-
Posts
4,207 -
Joined
-
Last visited
-
Days Won
209
Everything posted by Jacques1
-
The whole approach is suicidal. When you don't stop the script, it happily keeps running and will in fact render the entire page (or take any action the user has requested). This obviously defeats the purpose of authentication checks. The only reason why your code seemingly “works” on your other sites is that a successful redirect makes a standard browser discard the original content and jump to the new page. Appearently the header() call has failed this time (I assume you have prior output), so the problem which has always existed now becomes visible. Long story short: Always stop the script if authentication failed. A redirect alone doesn't do anything but advise the client to visit another URL. Check if there's output prior to the header() call. This is not allowed. Turn error reporting all the way up and make sure you actually see the errors (you only do this on your development machine, of course!)
-
It's difficult to understand what you want, and the whole context sounds rather odd. Do you want to assign IDs to URLs so that you can hide the real URL behind something like https://yoursite.com/download.php?file_id=123 ? In that case, I recommend you store all your URLs in a proper database. The table would look something like this: files - id CHAR(whatever-length-your-ids-have) PRIMARY KEY - url TEXT NOT NULL - title TEXT NOT NULL The ID must be randomly generated as soon as you store the URL. Whenever a user sends you an ID, you look up the corresponding URL in the database and redirect the user to the URL (assuming it's actually an external URL, not a local file). If you don't want to install a full-blown database system, there's always SQLite which stores the entire database in a single file.
-
This isn't the Obfuscated Perl Contest.
-
I don't see why the function should be artificially restricted to regular files. Checking the existence of folders as well is a very natural use case, and it would be rather confusing for the function to fail in that case. If, for whatever strange reason, there may be empty filenames, I'd check exactly that.
-
Parsing CSV file + appending data to another file by unique ID key
Jacques1 replied to slim_swan's topic in PHP Coding Help
I would do this in three steps: First read the entire content of data.csv into an associative array where the ID is the key and the array of all other fields is the value. For example, the row “123;foo;bar” would become the mapping “123” => [“foo”, “bar”] Loop through each of the other CSV files line by line, look up the ID in the data.csv array and append the corresponding array to the current row. Write each extended row into a new CSV file. The first step would look something like this: <?php const CSV_DELIM = ';'; const CSV_MAX_ROW_LENGTH = 0; // 0 means "no limit" $data = []; $data_handle = fopen(__DIR__.'/data.csv', 'r'); $data_columns = fgetcsv($data_handle, CSV_MAX_ROW_LENGTH, CSV_DELIM); while ($data_row = fgetcsv($data_handle, CSV_MAX_ROW_LENGTH, CSV_DELIM)) { $id = $data_row[0]; $data[$id] = array_slice($data_row, 1); } var_dump($data); -
I don't think you'll get a lot of feedback when there's no documentation anywhere. Even the code examples on GitHub don't work due to typos and wrong array keys (looks like “user”, “pass”, “name” and “pref” should actually be “username”, “password”, “dbname” and “prefix”). You offer an Italian ebook, but that's just empty (and Italian). Your code is wide open to SQL injection attacks through identifiers: $db->apply([ 'test' => [ 'x' => $db::TEXT, ], ]); $db->insert('test', [ "x) SELECT CONCAT('The MySQL version is: ', VERSION()) -- " => "dummy value", "x" => "dummy value", ]); This puts your exact MySQL version (or any other sensitive data) into the column: INSERT INTO `prefix_test` (x) SELECT CONCAT('The MySQL version is: ', VERSION()) -- ,x) VALUES (:x) SELECT CONCAT('The MySQL version is: ', VERSION()) -- ,:x) You're also using emulated prepared statements (this is a PDO default) and a SET NAMES query, which can make the values vulnerable to injection attacks as well. Emulation means that PDO doesn't actually use prepared statements, it merely auto-escapes all values. When you use SET NAMES, you silently change the connection encoding without notifying PDO, potentially breaking the escape mechanism altogether. The latter can be fixed by turning off emulation and using the DSN charset attribute instead of SET NAMES. The former is tricky, because you have no concept for safely dealing with identifiers (they're just dumped straight into the query). One possible approach would be to wrap all identifiers in backticks while escaping all backticks within the identifiers (through doubling). Whether this actually works in all cases is an open question, though.
-
You have to move the return true behind the loop that that it's only executed if all files exist. Or use a boolean variable for clarity: $filenames = ["index.html", "areas.html", "test.html", "example.xls"]; $all_files_exist = true; foreach ($filenames as $filename) { if(!file_exists($this->skeleton_dir.$filename)) { $all_files_exist = false; break; } } return $all_files_exist;
-
There are a couple of excellent PDO tutorials which explain everything you need to know: (The only proper) PDO tutorial PDO Tutorial for MySQL Developers Sticking to the mysql_* functions is not an option, because they've been removed entirely from the current PHP version. As soon as the server is updated, you'll have to write all the code again. Besides that, the old MySQL extension is infamous for security issues.
-
There's no need for an additional join. Simply check if the number of matching key/value pairs is 2 (so that both conditions are fulfilled): SELECT {$wpdb->woocommerce_order_items}.order_item_id, {$wpdb->woocommerce_order_items}.order_id, $wpdb->users.ID AS user_id, $wpdb->users.user_nicename FROM {$wpdb->postmeta} INNER JOIN {$wpdb->users} ON {$wpdb->postmeta}.meta_value = {$wpdb->users}.ID INNER JOIN {$wpdb->woocommerce_order_items} ON $wpdb->postmeta.post_id = {$wpdb->woocommerce_order_items}.order_id INNER JOIN {$wpdb->woocommerce_order_itemmeta} ON {$wpdb->woocommerce_order_items}.order_item_id = {$wpdb->woocommerce_order_itemmeta}.order_item_id WHERE {$wpdb->postmeta}.meta_key = '_customer_user' AND ( ({$wpdb->woocommerce_order_itemmeta}.meta_key = '_wcs_migrated_subscription_status' AND {$wpdb->woocommerce_order_itemmeta}.meta_value = 'active') OR ({$wpdb->woocommerce_order_itemmeta}.meta_key = '_product_id' AND {$wpdb->woocommerce_order_itemmeta}.meta_value = '20') ) GROUP BY {$wpdb->woocommerce_order_items}.order_item_id, {$wpdb->woocommerce_order_items}.order_id, $wpdb->users.ID, $wpdb->users.user_nicename HAVING COUNT(*) = 2
-
How could the meta value possibly be both "active" and "20" at the same time? I think what you actually mean is this: AND ( ($wpdb->woocommerce_order_itemmeta.meta_key = '_wcs_migrated_subscription_status' AND $wpdb->woocommerce_order_itemmeta.meta_value = 'active') OR ($wpdb->woocommerce_order_itemmeta.meta_key = '_product_id' AND $wpdb->woocommerce_order_itemmeta.meta_value = '20') )
-
session_start() creates new session on page reload
Jacques1 replied to runnerjp's topic in PHP Coding Help
The browser doesn't even send the session cookie, so PHP has no chance of resuming the session. Does your browser block cookies from the domain of your application? Have you tried a different browser? -
session_start() creates new session on page reload
Jacques1 replied to runnerjp's topic in PHP Coding Help
The code doesn't display anything, so what exactly makes you think that sessions aren't resumed? Execute this script twice and show us each response: <?php session_start(); echo 'Cookies:<br>'; var_dump($_COOKIE); echo '<br>'; echo 'Session:<br>'; var_dump($_SESSION); $_SESSION['test-8165'] = 'test'; Also check the PHP error log. -
You must exclude the actual HTML markup from the html_escape() call. Actually, I'd avoid <br> and use a semantic element like an unordered list instead. This gives your markup a meaningful structure and makes it easier to read for machines (search engine bots, maybe screen readers for visually impaired users etc.). echo '<ul>'; foreach ($songStmt as $song) { echo '<li>'.html_escape($song['artist'] . " - " . $song['title'], 'UTF-8').'</li>'; } echo '</ul>'; If you don't like the bullet points, you can hide them with CSS.
-
Make sure to HTML-escape all outgoing data. Otherwise the same problem that happened with your query may happen again with your HTML. <?php /** * HTML-escapes a string so that it can safely be included in an HTML document * * @param string $raw_input the string which should be escaped * @param string $encoding the character encoding of the input string * * @return string the encoded string */ function html_escape($raw_input, $encoding) { return htmlspecialchars($raw_input, ENT_QUOTES | ENT_SUBSTITUTE, $encoding); } echo html_escape($song['artist'].' - '.$song['title'], 'UTF-8');
-
There's no point in assigning IDs to the pages and including them in some main script. Simply create one standalone script for each page. If you have common functionalities which are needed on multiple pages, use PHP functions. If you have a common layout, use templates.
-
Putting the pages into a database is not an option if they contain PHP code (you should stop calling that “static”, by the way). There's a huge risk that an attacker finds a way to manipulate the pages, either through an SQL injection attack or your admin interface. If that happens, the attacker can execute arbitrary PHP code on your server, which is the absolute last thing you want. Including the pages into a main script also isn't a good idea, because this means there will be implicit dependencies all over the place. The pages will pull their variables seemingly out of nowhere (from the main script, actually), which makes the code error-prone, hard to read and just ugly. I'd go with a separate script for each page and a template engine like Twig. Template engines are perfect for creating “almost static” pages and embedding different contents into a common layout.
-
Your query may be vulnerable to SQL injection attacks through the $company_email variable. Use a prepared statement instead. Also, this line-by-line format is very bad for automatic parsing. Who generates the e-mails? Is there a chance to send the data in a proper machine-readable format like JSON, XML etc.? This would make the parsing much easier and more robust.
-
Your MySQL passwords are stored in an ancient, insecure format which is no longer supported. You need to keep MySQL up-to-date as well and then generate new passwords (since the old one may have been compromised).
-
And let's not forget CSRF attacks. You can check permissions all day long: If anybody can “tunnel” request through your admin accounts, permissions are meaningless. So first fix the SQL injection vulnerabilities (this should be done by now), then solve the CSRF issues with the Synchronizer Token Pattern and finally take care of authentication and authorization. When in doubt, open a new topic.
-
array_push() appends a value to an array, so there are necessarily two arguments (or more). If you have less, your code makes no sense. It seems what you actually want to do is store a value at a specific index, which has nothing to do with array_push(): $array[$location][$i] = $connection->sheets[0]["cells"][$i][2];
-
This is safe from SQL injection attacks, because now you're actually using a prepared statement. All values are passed to the statement parameters, which means they cannot alter the query structure. Injections only happen when you insert PHP strings straight into the query. You should get rid of the $mysqli->errno stuff, though. This will leak information about your server and is also completely unnecessary if you use mysqli_report() (since mysqli will automatically report all errors). PDO is superior to mysqli in every aspect, so this is indeed the recommended interface. But if you somehow like mysqli more, you can of course stick to it.
-
This isn't a prepared statement, it's a collection of SQL injection vulnerabilities. And appearently you've managed to perform an injection attack against your own site. The PDO manual explains prepared statements in great detail, including multiple examples. I recommend you read that. It also seems you're randomly mixing mysqli functions with PDO features. You cannot do that. They're two entirely different database interfaces which are not compatible in any way.
-
You're inserting $_POST['search'] straight into the query string, which means that a single quote will terminate the SQL string after the LIKE keyword and change the entire query. It's a self-inflicted SQL injection. What's much worse than your current bug is that anybody can manipulate the query and fetch sensitive data (passwords, e-mail addresses etc.) or even take over the server. To fix this vulnerability, stop inserting PHP string into queries. The whole point of prepared statements is that you use a constant query with placeholders: <?php const DB_HOST = 'localhost'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; const DB_CHARSET = 'UTF8'; /* * Set up the database connection * - the character encoding *must* be defined in the DSN string, not with a SET NAMES query * - be sure to turn off "emulated prepared statements" to prevent SQL injection attacks * - turn on exceptions so that you don't have to manually check for errors */ $dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET; $databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [ PDO::ATTR_EMULATE_PREPARES => false, // activate real prepared statements PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, // activate exceptions PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, // fetch associative arrays by default (this isn't critical) ]); $songStmt = $databaseConnection->prepare(' SELECT artist, -- always select *concrete* columns title FROM songs WHERE artist LIKE :artist_search_term OR title LIKE :title_search_term LIMIT 100 '); $songStmt->execute([ 'artist_search_term' => $_POST['search'], 'title_search_term' => $_POST['search'], ]); foreach ($songStmt as $song) { var_dump($song); }
-
Edge starting new session on header(Location:...)
Jacques1 replied to jcanker's topic in PHP Coding Help
So it's obviously not a PHP problem or session issue but rather a browser misconfiguration. I know nothing about Windows 10, but maybe somebody else will chime in. Otherwise try a Windows forum. -
I strongly recommend that you learn how arrays work, because this is fundamental for any programming language, not just PHP. If you still can't get it to work, post your code and explain the problem.