Jacques1

You mustn't enclose the parameters in quotes. In the query above, there are actually no parameters at all. You're literally asking for the user whose name, password and type is a question mark. There are two other things: Why do you again check the username and password after you've retrieved the row? You know the values already, because you've selected the row based on the username and password. You really cannot store passwords as plaintext, even if this is just a school project, a hobby site or whatever.

I'm aware that the MySQL extension is deprecated. But many people still use it (as you can see in this forum), so they need a way to escape their data. And the right function for this is mysql_real_escape_string() as opposed to intval(). If you use a modern database extension, you shouldn't rely on manual escaping at all and instead pass the data through the parameters of a prepared statement.

There are some fundamental issues, including several security holes. Accepting any image type is not a good idea, because some of them are much more than just a harmless collection of pixels. For example, an SVG “image” is actually a full-blown XML document which can contain arbitrary XHTML content including scripts. If you let people upload such files, that's like allowing them to place arbitrary HTML files in your document root. This is not just a cross-site scripting vulnerability. You've basically turned your whole site into a JavaScript playground. Then you insert the raw content of $_SERVER['PHP_SELF'] into your document, probably assuming that it only contains a harmless file path. But Apache actually allows the client to append arbitrary content to the real path. So, for example, they could access your script with this URL: https://yoursite.com/upload.php/"><script>... some malicious JavaScript code ...</script> This gets inserted into the page, and the user ends up running the appended JavaScript code. You also have no image validation whatsoever. You accept anything as long as the user-provided MIME type starts with “image”. That means instead of uploading images, people might as well submit text or code or whatever. I'm not saying that validation helps (it doesn't), but it might be a good idea to at least do some basic checks like running the image through getimagesize(). Long story short: Escape everything, even if you think it's harmless. Don't make assumptions about whether or not a value can be used for an attack. Reject everything unless you know how to handle it. Which image types do you know? JPEG, PNG and GIF? Great, then accept those three types and reject everything else. Don't just accept the whole “image” media type. I don't even know all the subtypes in there. Do you? I also wouldn't store the images in the database unless you have a specific reason for that. It bloats the database, makes the images difficult to access and increases the response time. Why not just store the images as actual files?

Psycho has just explained this in every detail. You need an additional table which assigns SKUs to attributes.

This is a common misconception. Cross-site scripting which depends on the user input is called Reflected XSS and is just as dangerous as Persistent XSS (which you describe). It only requires a slightly different attack: While Persistent XSS is triggered when the victim visits the target page, Reflected XSS is triggered by getting the victim's browser to send the required input. In the case above, for example, the attacker only needs the victim to visit a page with some predefined JavaScript code. This code would create a form pointing to the target page, define the cushion-refilling parameter with malicious JavaScript code and then automatically submit the form. Now the victim has just made a POST request to the target page and ends up running the code in the cushion-refilling parameter.

The keys are already in the value attribute of the option. They're sent automatically. Or are you talking about the price, the title etc.? You do not need to send those. You already have them in your database and can look them up at any time, so all you need to send is the key.

It's unclear what you want, which is probably why nobody hasn't answered yet. What do you mean by “send info through a form”? Do you want to submit the selected options to another script? Then why not use an actual form? That's what they're for. Generating dynamic JavaScript code is actually a very bad idea, because you easily end up with cross-site scripting vulnerabilities or plain bugs. For example, if there's any single quote in your data, your whole script blows up with a syntax error. That's hardly a good solution. Instead, simply store the price in a data attribute in the option element itself. Or load the data with an Ajax request. Or JSON-encode the data, HTML-escape it and then put it into a hidden div element. But do not drop PHP values into a script element. That's really the worst of all approaches.

Hi, I've noticed that many members routinely recommend intval() for “sanitizing” user input. I think this is a very bad idea for a couple of reasons: PHP integers are stored in 32 bits or 64 bits depending on the platform. This is not enough to cover all MySQL integer types. For example, a 32-bit PHP integer can neither hold an INT UNSIGNED nor a BIGINT. And even a 64-bit PHP integer cannot hold a BIGINT UNSIGNED. That's obviously a problem and can lead to very nasty truncation bugs. Silently changing the user input is very confusing and potentially harmful. Let's say the user tries to delete a record, but the provided ID is not numeric. This is clearly an error. Either the user has entered a wrong value, or there's an application bug. In any case, the request cannot be processed safely and should be rejected. What the intval() does instead is turn the invalid input into a “random” ID and pass it on to the database system to delete the record. Bad idea! Many people already struggle to understand the difference between mysql_real_escape_string(), addslashes(), htmlentities(), filter_var() etc. Now we have yet another function in the ever-growing pool of “sanitize” functions. This doesn't really help. So I think intval() should never be used for data “sanitization”. Just use the appropriate escape function like mysql_real_escape_string().

Now he has a cross-site scripting vulnerability through the cushion-refilling parameter. The user input must be escaped, so using htmlentities() was actually the right idea. He just applied it to the wrong data: $message = 'Cushion Refilling Service: <strong>' . htmlspecialchars($_POST['cushion-refilling'], ENT_QUOTES, 'UTF-8') . "</strong>\n\n"; Note that you want htmlspecialchars(), not htmlentities(). The htmlentities() function converts all characters for which there's a named HTML entity. This is absolutely useless and a waste of resources. On the other hand, htmlspecialchars() only converts the characters which actually have a special meaning like “<” or “>”. Also note that you must specify the character encoding. How is PHP supposed to convert characters if it doesn't even know how they look like?

You can't write ... some_column LIKE '...' OR LIKE '...' ... This may make sense in colloquial language, but to a computer, this is gibberish. The boolean OR operator can only be applied to two boolean expressions. But “LIKE ...” is no expression at all, it's just a syntax fragment. A syntactically valid query would look like this: ... some_column LIKE '...' OR some_column LIKE '...' ...

Not sure what you mean. PHP parses the URL parameters and sets the $_GET superglobal before the script runs. What you do within the script is your business. If you want to update $_GET with some new value, you'd have to do that yourself (but of course you shouldn't and just use the correct parameters from the beginning).

The approach itself is already wrong. First of all, binding the attempts to the IP address is nonsensical, as mac_gyver already said. One IP does not equal one user. This is a widespread belief, but it's false nonetheless. You're actually helping attackers while punishing legitimate users. An attacker has access to hundreds and thousands of different IP addresses, be it legally through the use of proxies or Tor, be it illegally through botnets. That means they can easily switch to a new address after 3 attempts and start over without having to solve any CAPTCHA. On the other hand, a legitimate user may share her address with many other people due to a proxy, a VPN, a public access point or whatever. That means they may constantly have to solve CAPTCHAs just because somebody with the same IP address entered a wrong password on your site. The next problem is that you've completely forgotten about simultaneous requests. You save the current number of attempts, time passes, and then let the user log in if that number is below the limit. But what if there have been 100 other attempts in the meantime? Then you accept them all based on the obsolete counter value. So an attacker can circumvent your limit simply by sending many simultaneous requests. You again help attackers while punishing legitimate users who dutifully make only one request at a time. To fix this problem, you need to increment the counter and get the new value in one atomic operation. And then you check if that number is 3 or less. So instead of counting the failed log-in attempts, it's more like taking a number: The requests who got 1, 2 or 3 are allowed to submit the credentials directly, everybody else also has to solve a CAPTCHA. Implementing this may be tricky in SQLite, because it's a very simple database system which lacks most advanced features. In MySQL, you would do this with a user variable or LAST_INSERT_ID(): // update the counter and save the new value in a single atomic operation UPDATE users SET login_attempts = (@current_attempt := login_attempts + 1) WHERE user_id = 123 ; // get the updated value SELECT @current_attempt;

This doesn't require an extra variable. Just set the value to null when needed, e. g., $stmt = $database->prepare(' ... some query ... '); $stmt->execute(array( 'fname' => isset($_POST['fname']) ? $_POST['fname'] : null ));

Of course the parameters aren't read from the URL. They simply aren't there. What you have is a single URL parameter called “rawEncodedLink”. This parameter contains a bunch of URL-encoded characters. Since they're encoded, they do not have any special meaning for the URL. It's just data. The original “&” is now a literal character. It seems you want something entirely different. If you want to add actual parameters to the URL, you must not encode the “&” character. Use http_build_query() to create a proper list of URL parameters.

You need to check if the parameter even exists: $fname = isset($_POST['fname']) ? $_POST['fname'] : null; However, this whole practice of putting external input into separate variables doesn't make a lot of sense. You already have a variable with the value: $_POST['fname']. What's the point of creating this extra $fname variable? Only to save 10 characters when writing it down? That's not really a valid reason in the times of auto-completion.

This is wrong on several levels. First of all, it's conceptually wrong to apply one particular escaping function to all input. The PHP core developers tried a similar concept in the early days of the language (see Magic Quotes), and it ended in a disaster. As long as you indeed use the values in a query, it may seem convenient that they're already “pre-escaped”. But what if you want to use them in a different context like your HTML document? Then you need to revert the previous escaping, retrieve the original value and finally apply the correct escaping technique to this value. That's twice as much work. And should you ever forget to remove the SQL-escaping, you'll end up with the infamous backslash bug: All quotes and blackslashes will be preceded by an actual backslash character. This really doesn't look good on a website. Another problem is that your pre-escaping will never cover all input. OK, so you've escaped the entries of $_POST. But what about $_GET? And $_COOKIE? And $_REQUEST? And $_FILES? And $_SERVER? And other values you read from external sources like your database or a file or another website? Those are all just as dangerous as POST parameters, but you keep them as they are. This is the perfect setting for a security vulnerability: If you become used to just dropping variables into query strings and then encounter a value which is not escaped yet, you're likely to forget the escaping. Now you have a full-blown SQL injection vulnerability. Apart from the conceptual aspect, the code simply doesn't make sense. I can vaguely recognize the idea behind it, but almost every line is wrong in some sense. Why do you check if $_GET is set? This superglobal is always set. And what do the URL parameters have to do with POST parameters, anyway? What's the matter with the loops? When you've just found out that $_POST is not an array, why do you then try to iterate over its elements as if was an array? And so on. Last but not least, the mysql_* functions are hopelessly outdated and will be removed in one of the next PHP releases. We have much better alternatives (PDO and MySQLi) since more than 10 years, and we've given up manual SQL-escaping in favor of prepared statements a long time ago. You're riding a dead horse. Long story short: This is not a good idea. Use PDO instead of the old MySQL extension. Use prepared statements instead of manual SQL-escaping. And always escape data for its specific context. Blanket escaping doesn't work.

PHP cannot set HTTP headers when there already was output (like HTML). Since sessions usually need to set a session cookie in the headers, you're getting this error message. It's generally very poor practice to mix PHP and HTML. This is actually one of the more extreme cases, because you're randomly switching between large blocks of markup and large blocks of application logic. This is almost guaranteed to cause trouble, and it also kills readability and maintainability. I couldn't figure out what this script even does until I read through the entire PHPSQLHTMLCSSJavaScript mixture. So this is a fundamental problem. You need to start seperating the different aspects of your application. Keep your CSS styles and your JavaScript code in external files. Remove all the inline stuff and throw away ancient style elements like font. Keep the HTML markup out of the application logic. Do all the heavy lifting at the very top of the script and put all markup to the very bottom. Or even better, use a template engine like Twig or Smarty to keep your markup in external files as well. This will make the code much cleaner and automatically fix your header problems.

Your database design is broken. Stuffing multiple values in a single attribute is a classical mistake and a common source for problems. In the relational model (which SQL is based on), attributes must be atomic. That is, there's only a single value, not a list, not a complex structure. As soon as you start abusing strings for lists, the data is no longer accessible to the database system. Yes, you may kinda sorta get it working with string operations, but those are all just hack. The fact is the database system cannot do its job. Read up on normalization (in particular the first normal form) and fix the problem with an association table. The query itself will then be trivial.

I don't see the point of this thread. I mean, Michael Feathers obviously refers to a certain idea when he talks about “Legacy Code”. It's not about legacy code in the literal sense. However, you don't seem to be interested in the idea at all. You just take the words literally, come up with all kinds of random associations and then try to apply Feathers' definition to your own (mis)interpretation of the words. Well, yes, you can do that. But how is this useful? What's your goal? Of course a fresh project doesn't suddenly become “old and ugly” if I remove the tests. And obsolete code doesn't suddenly become fresh if only I add some tests. That should be obvious. So what are we even talking about?

You've obviously missed my reply. You don't need a COUNT(*) query at all. Just use SQL_CALC_FOUND_ROWS. Regarding your database class, I suggest you throw it away and switch to plain PDO. The class is way too trivial to be useful, and most of the time, it actually stands in the way (as you can see). With PDO, this whole task is a matter of 5 minutes: $images_stmt = $database->prepare(' SELECT SQL_CALC_FOUND_ROWS ... FROM ... '); $images_stmt->execute(array( ... )); $total_images = $database->query('SELECT FOUND_ROWS()')->fetchColumn(); foreach ($images_stmt as $image) { ... }

This is extremely inefficient, because you load the entire table into PHP only to count the rows and then throw the result set away. The correct way to count rows is indeed a COUNT(*) query. You just need to actually fetch the result as explained by Psycho. In your case, however, you don't need an extra query to count the rows. You can do it in the main query with the SQL_CALC_FOUND_ROWS option. This makes MySQL remember the total number of rows which the query would return if there was no LIMIT clause. You can then get this number with FOUND_ROWS().

No, man5 is using the OFFSET syntax.

A lot of the code doesn't really make sense. First of all, you don't seem to understand what exceptions are for and how they work: Catching instances of Exception means that you're catching any exception which could possibly happen, including those you may not even know. This obviously makes no sense. If PHP throws a, say, TotallyOutOfMemory exception, it's probably a good idea to not interfere with it, because there's no way you could handle this. Your “error handling” consists of printing the error message on the screen and then continuing the script. Do you really want everybody to see your database errors in the live system? Do you realize that the default behaviour of exceptions is much smarter than this? By default, exceptions either print the message on the screen (suitable for development) or write it to an error log (suitable for the live system) depending on your PHP configuration. And do you really want to continue the script after the query you need has just blown up? So the try-catch block is not only useless, it's actually harmful. You should remove it. In general, don't catch exceptions unless you can actually handle them. Just let them bubble up so that either a higher level or PHP itself can take care of the problem. If you do catch an exception, always catch a specific class like PDOException. I'm also not sure why you're using a prepared statement. The point of a prepared statement is to pass dynamic values to the query in a secure fashion or speed up repeated execution of the same query. You have neither. So why not just call query() or exec() like before? That depends on what exactly you mean by “updated”. Do you want to check for actual data changes? Why? If the data simply didn't have to be changed, what's wrong with that? Or do you want to know how many rows have been “touched” by the update? That's somewhat more useful.

The two URLs are frame sources. When you render the cURL response in your browser, the browser tries to load the frame content from /left_main.php and /right_main.php respectively. Those relative URLs is resolved according to the current host, which is localhost. And you obviously don't have such scripts on your localhost. You simply passed the wrong URL to cURL. You don't need the URL of some page with frames on it, you need to URL of the actual data you want. So if the mails are on, say, right_main.php, then you pass that URL to cURL.

There are several other issues: Do you actually store the admin credentials as plaintext in your script? Never store plaintext passwords, always hash them with bcrypt. Your sessions are vulnerable to sessions fixation, because you keep reusing the same ID. If an attacker manages to set a custom session cookie in the victim's browser, they just need to wait for the victim to log-in. Now they know the ID of a fully authenticated session. To prevent this, the ID must be regenerated in the log-in procedure. Your queries are vulnerable to SQL injection attacks. You've randomly added mysqli_real_escape_string() to one method, but you don't seem to understand what that does and how to use it. Manual escaping is obsolete, anyway. Learn to use prepared statements. You establish a new database connection for every query. This is a performance killer and simply bad design. Establish one connection, store it in an attribute and then use it for all queries. The mixture of PHP and HTML makes the code very hard to read and leads to many problems like the infamous “headers already sent” error. In modern code, you separate the application logic from the presentation. Put all the PHP stuff on top of the script and all the HTML to the bottom. Many people also use a dedicated template engine like Twig. Your class is becoming a god object. Use separate objects for separate functionalities. For example, user authentication has absolutely nothing to do with listing servers, so there's no reason to put both into the same class. Proper names would also help (a “handler” could be pretty much anything). Table layouts were OK in the 90s, but nowadays, we use CSS. You declare the document as XHTML, but then you use plain HTML, making the markup invalid in either flavor. Is there any reason why you want XHTML? This is a rather exotic XML-based implementation of HTML and requires special treatment. For example, you cannot use the usual text/html content type. You need application/xhtml+xml, which in turn cannot be handled by older browsers. I'd just use plain HTML.