Problem with syntax in PHP str_replace

kerry14 · July 20, 2021, 4:10pm

Hi I am trying out some code from the internet for a secure login, one of the functions seems to contain a syntax error in a line that reads -

$url = str_replace("'", ''', $url);

The whole function is -

function esc_url($url) {
 
    if ('' == $url) {
        return $url;
    }
 
    $url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\\x80-\\xff]|i', '', $url);
 
    $strip = array('%0d', '%0a', '%0D', '%0A');
    $url = (string) $url;
 
    $count = 1;
    while ($count) {
        $url = str_replace($strip, '', $url, $count);
    }
 
    $url = str_replace(';//', '://', $url);
 
    $url = htmlentities($url);
 
    $url = str_replace('&amp;', '&', $url);
	
    $url = str_replace("'", ''', $url);
 
    if ($url[0] !== '/') {
        // We're only interested in relative links from $_SERVER['PHP_SELF']
        return '';
    } else {
        return $url;
    }
}

I have to admit I don’t yet understand the full code, but there is definitely a syntax error in the line mentioned. If I can sort that out I can continue with my understanding and learning - thanks guys

Gandalf · July 20, 2021, 4:52pm

What are you trying to do there? You can’t wrap a single quote in a pair of single quotes.

kerry14 · July 20, 2021, 5:32pm

To be honest, I am not completely sure either As I said It is a secure login script I found on the net. The desription of the function they give is -

This next function sanitizes the output from the PHP_SELF server variable. It is a modification of a function of the same name used by the WordPress Content Management System.

I can comment out the line and the whole script works OK. But I am not sure if that will create another problem. It seems they are trying to replace single quotes in the url with something but I am not sure with what or why. I was hoping someone had some advice or comments.

droopsnoot · July 20, 2021, 6:13pm

Perhaps they are trying to replace single-quotes with double-quotes, and the line should read

$url = str_replace("'", '"', $url);

but if it’s a URL, would either character be valid there?

benanamen · July 20, 2021, 8:15pm

The fact that the code uses $_SERVER[‘PHP_SELF’] tells me that the “tutorial” is either old or poorly written. Best to show us the link to it.

trying out some code from the internet

More often than not you will run across old, insecure, or poorly written code just grabbing something at random from the internet. Be careful and understand the code before you implement it, especially for a “Secure"Login” code.

m_hutley · July 20, 2021, 11:40pm

Well, let’s at least walk you through what it DOES. You can then figure out whether it’s a good idea to use it (though as benanamen points out… probably not.)

function esc_url($url) {

Define a function, esc_url, that takes one parameter, $url.

   if ('' == $url) {
        return $url;
    }

If the URL was empty, return that empty string; we’re done here.

$url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\\x80-\\xff]|i', '', $url);
Get rid of any characters that are not:
alphabet characters,
the numbers 0-9,
the symbols -~+_.?#=!&;,/:%@$|*’()
the latin-1 characters corresponding to the hex-value range 80-FF. This is often called the “Extended ASCII Characters”.
(the |i indicates that the search should be case-insensitive, hence not needing to specify both a-z and A-Z.)

$strip = array('%0d', '%0a', '%0D', '%0A');
    $url = (string) $url;

Setting up some variables for later use.

$count = 1;
    while ($count) {
        $url = str_replace($strip, '', $url, $count);
    }

Recursively strip out anything in the $strip array from the URL. Repeat until you didnt remove anything.

For example, one of the elements of $strip is “%0d”. If, however, I wrote my url to be “%0%0dd”, running str_replace once would remove the inner %0d, replace it with a “”, and cause the string to just be “%0d”.

$url = str_replace(';//', '://', $url);
Change the semicolon to a colon in the URL’s protocol definition.

$url = htmlentities($url);
For some reason, we now convert special characters in the url to html entities.

$url = str_replace('&', '&', $url);
Remember the last line? Yeah, undo it for ampersands.

$url = str_replace("'", ''', $url);
This was probably supposed to replace single quotes with double quotes, as droop points out. But they flubbed it up.

 if ($url[0] !== '/') {
        // We're only interested in relative links from $_SERVER['PHP_SELF']
        return '';
    } else {
        return $url;
    }

If the first character of the URL is a /, it’s trying to refer to the root folder of the domain we’re currently on. The script doesnt want to play with those, so it returns the empty string if it finds one. If it starts with anything else, the URL is relative to the current script’s directory, so we return the URL.

This isn’t actually correct; a URL that begins with, say, http://, is not relative to the current page; they’re absolute URL’s. But the script doesnt seem to mind those, for whatever reason.

kerry14 · July 21, 2021, 8:29am

Wow ! thanks, so much for such assistance - especially m_hutly

and to benanamen

https://getcodify.com/create-secure-login-script-php-mysql/

Yes the code was published a while ago but I do not know if it has been updated at any point. It is not my intention to use it live in its current form but I was hoping to get it working and through research update each element to be properly secure. I don’t like the fact that is uses javascript, particularly for encryption.

I really want to develop a secure login, basic but secure. There are so many aspects to consider as well as individual methods and opinions. I did not want to draw the obvious and deserved negative comments a question such as ‘How do I write a secure login system?’ since I know that’s a short question with a long answer. So I thought I would get something going and work my way through asking for assistance only when necessary.

Any advice for a starting point would be great, opinion based is fine for me at this stage.

Maybe if I get it started as a project, I could share it here and people could contribute and I could help build a resource that others could use or refer to, what do you guys think, would that be appropriate or useful for this site and members?

Anyway, help so far has been awesome, thanks again guys.

kerry14 · July 21, 2021, 8:36am

Cheers, thats all I could think maybe, it works like that I guess, but like you say is what it does needed

All I can think is they are trying to prevent some kind of redirection or calling from another location for hacking attempts - maybe?

mabismad · July 21, 2021, 6:12pm

The code at the linked to page looks like it was pulled together from elsewhere (the registration and login code are different programming styles.) The line in question was probably attempting to convert any single-quotes to their html entity value (which should have been done via the htmlentities call, with an appropriate flag, a few lines prior to this line of code), which when it got published and then copied to the getcodeify… site resulted in the html entity for a single-quote looking like an actual single-quote.

The following is a laundry list of practices that the linked to code should/should-not be doing -

Don’t use defined constants for database connection credentials. This limits your code to ONE database connection.
Any variable/defined constant should be uniquely named as to what it is for. For the database credentials, the name should include ‘db’ or similar.
Use ‘require’ for things that your code must have for it to work.
There’s no consistent or useful error handing for database statements that can fail - connection, query, prepare, and execute. The easiest way of adding error handling for all the database statements, without adding code at each statement (which will also let you remove the existing error handling logic), is to use exceptions for database statement errors and in most cases let php catch and handle the exception, where php will use its error settings to control what happens with the actual error information (database statement errors will ‘automatically’ get displayed/logged the same as php errors.) The exception to this rule is when inserting/updating user submitted data. In this case, your code should catch the exception, detect if the error number is for something that your code is designed to handle, setup and display a message telling the user what was wrong with the data. For all other error numbers, just re-throw the exception and let php handle it.
Switch to the much simpler and more consistent PDO database extension.
If you build the sql query statement in a php variable, it makes debugging easier. This also separates the sql syntax as much as possible from the php code, reducing mistakes, and also lets you see the common php code being used for queries that are candidates for reduction by extending the database extension you are using with useful methods.
Other than trimming user submitted data, mainly for detecting if all white-space characters were entered, do NOT modify user submitted data, as this changes the meaning of the data. Validate the data and only use it if it is valid. If it is not valid, tell the user what was wrong with it and let them correct it.
Don’t waste time attempting to ‘sanitize’ data to try to make it safe. Instead, use data safely in whatever context it is being used in - html, sql, mail header, …
The Author of this script apparently missed the point that external data can be anything and cannot be trusted. You MUST validate data on the server before using it.
A great point about BOOLEAN values is you can directly test them in conditional statements. This code has a number of loose tests against a true value that are just needless typing.
This code is using preg_replace on values being fetched from a query. Simply amazing. See the above points about not modifying data and using data safely in context (at the point where it is being used.)
The only piece of user data that should be stored in a session variable upon successful login is the user’s id (auto-increment primary index.) Then, query to get any other user data on each page request. This will allow the user data to be edited and the change will take effect on the very next page request.
The code is filled with copying variables to other variables for nothing. Programming is not an exercise to see how much unnecessary typing you can do.
The whole esc_url function is pointless. To get a form to submit to the same page, just leave the entire action=’…’ attribute out of the form tag. Don’t even attempt to echo PHP_SELF/REQUEST_URI values.
A session can be used for more than just remembering who the logged in user is. The logout code should only unset the session variable(s) that the login code was responsible for.
The logout code should detect if a post method form was submitted before doing anything. Attempting to delete cookies is pointless. All this does is set cookie’s time in the past so that the browser won’t send it with requests. The cookie still exists on the client until it is physically deleted on the client computer.
This code has some of the form and the form processing on separate pages. This results in a poor user experience, is less secure, takes more code, and is redirecting all over the place (which opens a site up to phishing). Put the form and form processing on the same page. This will simplify the code and allow you to re-populate appropriate form fields with their existing values so that the user doesn’t need to keep reentering the same things over and over.
The only redirect in any post method form processing should be upon successful completion of the form processing code and it should be to the exact same url of the current page to cause a get request for that page. If you want to display a one-time success message, store it in a session variable, then test, display, and unset the session variable at the appropriate location in the html document.
Validation/user error messages should be stored in an array, using the field name as the array index. This will let you test and perform dependent validation steps on inputs and will let you output error messages adjacent to the inputs they go with. Only the text of the message should be stored in the array. Any markup/styling should be handled at the point of displaying the error.
The database MUST enforce uniqueness. Any column that must contain unique values must be defined as a unique index. This will cause an error upon inserting/updating duplicate values. To avoid repetitive logic, you can and should use this in your code to detect if duplicate values where entered by simply trying to insert the data, rather then first trying to use a select query to find if data already exist.
Any dynamic value that is output on a web page should have htmlentities applied to it, right before being output.

benanamen · July 21, 2021, 6:48pm

Excellent reply @mabismad.

OP, as you can see from the laundry list kindly provided by @mabismad, there are many issues with the “tutorial” you are working with just as I suspected. As you learn more you will get to know the red flags to look out for when reading someone else’s code.

While this code is good for learning things not to do, might I suggest starting learning what to do. A login application is a bit higher on the learning scale when just starting out but is a good exercise nonetheless. I think I myself started the same way.

There are many components to a login/registration script. I would suggest mastering each component, then you will be in a better place to put it all together.

Just some of the basic components…

Forms
Form handling
Error checking and handling
Form Validation (Notice I didn’t say Sanitation)

Database
Database Normalization (Part of DB Design & Architecture)
DB Connection
SQL

Authentication

One of the very good tutorials we often refer people to is this one on PDO. This will get you going in the right direction.

kerry14 · July 22, 2021, 12:51pm

Well, what can I say, thanks so much for the time and effort you have all put in to assist it is much appreciated and at least gives me a direction to take. The only item that concerns me is :-

I am used to mysqli so, taken from an example, mysqli connection =

 <?php
$servername = "localhost";
$username = "username";
$password = "password";

// Create connection
$conn = mysqli_connect($servername, $username, $password);

// Check connection
if (!$conn) {
  die("Connection failed: " . mysqli_connect_error());
}
echo "Connected successfully";
?>

Whereas the PDO equivalent is =

 <?php
$servername = "localhost";
$username = "username";
$password = "password";

try {
  $conn = new PDO("mysql:host=$servername;dbname=myDB", $username, $password);
  // set the PDO error mode to exception
  $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  echo "Connected successfully";
} catch(PDOException $e) {
  echo "Connection failed: " . $e->getMessage();
}
?>

This is almost like a new language to me

As I understand it PDO is really only useful if I want to connect to other databases and offers no additional security or ease of programming. OOP is apparently simpler, once you know it. I think for me procedural to oop is a big enough step and I am not convinced I need PDO - but I am not an expert.

m_hutley · July 22, 2021, 2:02pm

It’s more along the lines of flexibility, and probably longevity.

If MySQLI is killed off as a module, or… I dont know, MySQL stops being developed (yeah, not likely), then to change your script to a new database engine, all you have to do is change the 1 command (the connection string), as opposed to changing every mysqli_query call in your entire site.

mabismad · July 22, 2021, 4:02pm

The PDO extension is simpler than the mysqli extension, taking less statements to accomplish most tasks, especially when dealing with prepared queries. Regardless of how a query gets executed, the PDO statements testing/using the result from the query are the same. For mysqli, you must use two different sets of statements for a non-prepared and a prepared query. The mysqli extension also has a number of statements that are non-portable between servers because they are dependent on the mysqlnd driver being used, which you cannot guarantee unless you build and manage your servers. One of the things that got added, after the fact to mysqli, to make it more useful, like PDO already had, was the fetch_all() method/function, but they set the default fetch mode, incorrectly, to numeric, because whoever did this didn’t understand that the fetch mode refers to the rows of data, not the numerical array that the rows are being put into.

Don’t output the raw database errors on a live web page. A connection error contains the database hostname/ip address, the database username, if you are using a password or not, and web server path information. All a hacker needs to do to trigger a database connection error is to flood the server with requests to scripts that use the database to consume all the available connections. Use exceptions for database statement errors, for both mysqli and PDO.