Jump to content

Jacques1

Members
  • Posts

    4,207
  • Joined

  • Last visited

  • Days Won

    209

Everything posted by Jacques1

  1. Open the network tab of your browser to see what is and what isn't being sent from your form page to the target script. There are definitely problems: The HTML syntax is screwed up, because you're using the self-closing notation on the opening form tag (i. e. <form />). This is not only nonsensical (you don't want to immediately close the form), it's actually invalid in and will send the browser into error recovery mode – which may or may not work out. I strongly recommend you get rid of those old XHTML mannerisms like self-closing tags, use plain old HTML and validate the markup before jumping to any PHP code. The Mozilla Developer Network also has a great HTML crash course. There's no validation or error handling of any kind. You just hope for the best. As you can see, that's a huge problem when there actually are errors. You need to add proper feedback for you and your users(!). You're using short PHP tags (i. e. <?) all over the place, which will be a problem in the future (they're turned off on many systems). The second point is particularly important: Don't assume that everything will always work out as expected. It won't. You need to validate every step so that you have a chance to detect and fix problems: <?php /* ---- the receiving script ---- */ // First validation step: Was the request even a POST request? Maybe somebody accidentally just opened the script. if ($_SERVER['REQUEST_METHOD'] == 'POST') { // Second validation step: Did we receive all the expected parameters? $missing_params = []; $required_params = [ 'amount', 'comments', ]; foreach ($required_params as $param) { if (!isset($_POST[$param])) { $missing_params[] = $param; } } if ($missing_params) { echo 'Missing parameters: '.implode(', ', $missing_params); } // Add further validation if necessary (e. g. number parameters should in fact consist of digits) // *Now* you're ready to process the input }
  2. … which doesn't mean anything. When less experienced programmers think they're done, they've finished half of the job at best. When it takes three(!) people and an entire page to convince you of a simple fact and get to the real problem, then, yes, you might get a snarky comment. You'll make life a lot easier for yourself and everybody else if you just state the problem and accept help. Something like: “I've tried your array approach, but I don't know how to insert array elements into a string. Please explain.” The whole thing would be done in 5 minutes. There's a lot more to say, but you're clearly not ready for any information at this point. Feel free to resume the thread when you are.
  3. You still need the HTML-escaping. For that matter, the “simple” code you had before was just plain wrong.
  4. Be more specific. What do you understand, and what exaclty don't you understand?
  5. I assume the “payments” involve actual money? Then this isn't the kind of task you can handle with copypasta from the interwebs. The approach already irritates me. If you cannot – excuse my French – write a friggin' link script after somebody has explained it to you, how on earth are you going to implement a safe payment process? There are two options: Install a mainstream shop application which supports virtual products and can be customized enough to fit your needs. Hire(!) an actual(!) programmer to implement a custom solution. This will be expensive and risky, because there are a lot of people who also try the copypasta route and like to disppear when you need them the most.
  6. You realize this is a PHP forum, right? The reason why I point this out is because you seem to use the site exclusively for lots of off-topic threads which aren't going anywhere. Either nobody has an answer, or you never respond. Of course you have every right to keep trying, but maybe you should look for a more specialized community.
  7. *sigh* You realize that $stream_data is an array containing all the stream data, right? This array is all you ever need. Whenever you want to use a particular stream attribute, you just access it within the array: echo 'The display name is '.$stream_data['DisplayName']; // in reality, the variable must be HTML-escaped See? You can use the array elements directly. It's neither necessary nor sensible to copy them into separate variables. If there's some other reason for why you think you need separate variables, please explain that specific reason. Right now, it seems you just don't understand arrays.
  8. KISS You've produced a lot of extra code without any obvious benefit. For example, how is 'home'.$FILE_EXTENTION.'' better than a simple 'home.php'? It doesn't make the code any clearer (to the contrary), and the chances of ever having to change the file extension to something other than “.php” are pretty slim. And what's up with the empty string at the end?
  9. In your previous post, you weren't sure if the permission checks should also include the role. I'm simply saying that you should make business decisions like this before the implementation.
  10. It's not just the SQL queries. Your entire code is riddled with bugs that can lead to all kinds of syntax errors or injection vulnerabilties. For example, you're making the same mistake with your HTML markup. I strongly recommend you put your code aside and make sure you get a basic understanding of code correctness and security before going on: Never insert dynamic data into code contexts (SQL, HTML, shell commands, ...) without careful preparation. If you just drop a PHP variable into an SQL query or HTML document, even a completely harmless string like “O'Donnell” or “x<y” can cause havok, because it screws up the syntax of the surrounding context. And as you've seen, this bug can also be exploited to purposely inject code fragments. You need to either completely separate the data from the code (see prepared statements for SQL queries) or escape the data so that it won't interfere with the code (see HTML-escaping for HTML contexts). Never trust external input without careful validation. Whenever you pull data out of a source like $_GET or $_POST or $_SESSION or your database, it can be badly broken or malicious. So before you make any assumptions about this data, check it. Note that I used the broader term “external input” as opposed to “user input”, because even a source which isn't directly under the user's control (like the session or database) might be manipulated or simply buggy. Unfortunately, this is easier said than done, and PHP itself doesn't support you in any way. So if you try to implement this on a case-by-case basis, you're likely to screw up. You need a systematic approach with strict coding rules, the right tools and multiple layers of security: In general, all SQL queries should be entirely static (i. e. not involve any string concatenation). If you need to include dynamic data, use a prepared statement. Concatenation is only valid when you have to build a complex query from multiple SQL fragements. Use an HTML template engine with auto-escaping (like Twig) instead of assembling the HTML markup directly in your script. Put your JavaScript code and CSS rules into separate files instead of throwing everything onto one big pile of PHPSQLHTMLJavaScriptCSS. Chaos is a security risk. Adopt modern security mechanisms like Content Security Policy.
  11. The home page and error page path (first and third case) are lacking the PAGES_BASEDIR prefix. You're relying on relative paths instead, which can go wrong. Other than that, it's OK.
  12. I have full confidence in your ability to do that yourself. This isn't programming, it's search-and-replace.
  13. Isn't that the exact same code you had before? I'm talking about first checking if the page parameter isn't set if (!isset($_GET['page'])) { // default page } else if (in_array(...) ...) { // selected page } else { // error }
  14. You should rephrase the code for clarity: whitelist = ... if no page parameter: show home page else if page is in whitelist and page exists ...: show page else: show error
  15. The queries don't really make sense. Your first query always yields all existing permissions regardless of the user (which is potentially dangerous), because for some reason you're using left joins and put the user ID check into a join condition rather than the WHERE clause. To get the actual permissions, you need SELECT DISTINCT -- note DISTINCT: the user may have the same permission from multiple roles permissions.permission FROM -- note: no need to join with roles table permissions JOIN role_permission_mappings ON role_permission_mappings.permission_id = permissions.permission_id JOIN user_role_mappings ON user_role_mappings.role_id = role_permission_mappings.role_id WHERE user_role_mappings.user_id = 3 ; The second query also needs an inner join. You're already kicking out the rows added by the left join through the WHERE condition. As to the last question: The hard-coded role check doesn't make sense. In fact, it will be extremely confusing if a user cannot edit news when they've been explicitly been allowed to (but don't happen to be an admin). It also means that code changes are required to introduce new roles (e. g. super admin). In any case, you should definitely think your permission model through before jumping to the implementation. Some parts of the concept don't seem to be clear yet.
  16. You still haven't explained why you think you need an extra case for user permissions when this could be handled just fine with the generic role permissions. The user permissions of Joe Blow can be implemented as the permissions of the user-specific role “Joe Blow”. This will simplify both the database schema and future queries a lot. Right now, you'd have to do a UNION of both permission tables whenever you need to calculate the effective permissions. As to the employee permissions, I would generally avoid assigning individual permissions to employees. It makes a lot more sense to have a generic role like “Walmart salesman” and only add employee-specific permissions when they're actually needed.
  17. Are you sure that the PHP code before the operator is executed? What you describe sounds more like the entire file is interpreted as HTML. Open the page source code in the browser to check, don't just look at the rendered content.
  18. What on earth is the point of the JSON encoding anyway? All it does right now is produce a lot of problems and extra code.
  19. You simultaneously assume that the request body uses custom JSON encoding: json_decode(file_get_contents("php://input")) and classical URL encoding: $_POST["captcha"] It can't both be true. I recommend you get rid of the weird JSON stuff (as I already told you last time) and use plain old form parameters.
  20. I'm talking about your form which must in some way include the CAPTCHA. And again: You'll save yourself a lot of trouble now and in the future if you just use a known CAPTCHA library. Google has copypastable code and detailed instructions.
  21. You cannot randomly put an if statement into your code. Like every language, PHP has a specific grammar. You have to either create a separate statement with the if check and the setCellValue() call. Or you use the ternary operator, which yields a conditional expression: ... ->setCellValue('F', $i, $row['prod_sell_override'] ?: $row['prod_trade'] * 100 / 70) ... Read: If $row['prod_sell_override'] is truthy (i. e. neither empty nor 0), use it as the cell value, otherwise use $row['prod_trade'].
  22. So where's the actual CAPTCHA? All you've shown is the CAPTCHA check. By the way, home-made CAPTCHAs are typically not very effective. Consider using a professional solution like Google's reCAPTCHA.
  23. You want a whitelist, not a blacklist. That is, you explicitly list all permitted pages. Anything else should result in a 404 error code: <?php const PAGES_BASEDIR = __DIR__.'/pages/'; const PAGES = [ 'home', 'about', ]; if (isset($_GET['page']) && in_array($_GET['page'], PAGES, true) && is_readable(PAGES_BASEDIR.$_GET['page'].'.php')) { require PAGES_BASEDIR.$_GET['page'].'.php'; } else { http_response_code(404); // display error page echo 'Page does not exist'; }
  24. Look at the output. It's not just empty text, the entire rows are missing. This means your script probably fails at some point, but you've not managed to set up proper error handling. Turn your error reporting all the way up and either display the errors (during development) or log them (in production). Unless the ODBC throws exceptions automatically, you also need to do this manually. Don't just assume that every function call succeeds. It also helps to run the query alone (in the console or in phpmyadmin). Does it yield the correct result set? Finally, I'm irritated that each fragment of your alleged code contains syntax problems, even though you claimed it worked fine before the data type change. So did you change the code as well?
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.