Sanitizing whitespace in generated URL parameters

Question

I'm having an issue with a PHP page that generates an HTML document. On the page, there is a form that has a <select> box, and each <option> inside has a value with the URL parameters to use when changing the window location when the form is "submitted". The issue is that I noticed is that one of the parameters is a name, and when that name has a space in it, it breaks the window location because the space remains in the URL.

I've tried simply doing a str_replace() on the string before it generates the <option> tag, and when I view the <option> in Firefox' inspector, it DOES contain a %20 instead of a space, but when I look at the URL bar after clicking the <option>, it still retains the space instead of the %20. Can anyone tell me what's wrong with the following snippet?

print("<form name=sel1>");
print("<select size=10 style='width:200;font-family:courier,monospace;
        font-weight:bold;color:black;' ");
print("onchange=\"location = this.options[this.selectedIndex].value;\">");
for($i = 0; $i < count($allGroups); $i++)
{
    print("<option value='groups.php?action=");
    if($advancedPermissions)
    {
        if($modGroups)
        {
            print("edit");
        }
        else
        {
            print("view");
        }
    }
    else
    {
        print("edit");
    }
    print("&group_id=");
    print(str_replace(" ", "%20", $allGroups[$i][0])."'>");

    print($allGroups[$i][0]);
    if($allGroups[$i][2] != 'Y')
    {
        print(" - inactive");
    }
}

print("</select></form>");

The relevant lines are the line with location = and the line just after the group_id parameter is added, where the str_replace() is done.

I do the str_replace() on just the value, not the display text, so it will show normally to the user, but contain the %20 character for when it is passed to the window location, but regardless, it either ignores it, or something else is happening I am not aware of.

Use PHP Functions first trim() and then str_replace(" " , "-",$location) , try it ! — Sanmeet, Jun 15 '21 at 18:11
@Sanmeet I can't replace it with dashes, as some group IDs have dashes in them. It has to be the %20. But I also have to then remove that %20 if it doesn't do that automatically when the PHP page receives that data, so I can match the group_id parameter to the info coming from the DB. — Darin Beaudreau, Jun 15 '21 at 18:26
1. Get rid of that `str_replace()` and use either `urlencode()` or `rawurlencode()`. 2. The URL bar lies, and will almost always show you the decoded, human-friendly version of a URL. 3. The truly correct answer is `http_build_query()` as in miken's example, but it depends on how motivated you are to clean up some of that mess vs throwing your hands in the air and saying "well _I_ didn't write it...". — Sammitch, Jun 15 '21 at 19:05
@Sanmitch No, I'm definitely gonna try out http_build_query, but I was referring to his suggestion of rewriting the entire page to fit his preferred format. It would be nice, but I don't have the time or the reason. — Darin Beaudreau, Jun 15 '21 at 19:10

miken32 · Accepted Answer · 2021-06-15T20:39:47.607

This code is a whole mess of bad practices. First, separation of code (PHP) and presentation (HTML) is essential for making sense of things. You should be doing logic in a separate code block at the very least, if not a separate file. Definitely not in the middle of an HTML element. Exiting PHP and using alternative syntax and short echo tags make this separation much clearer.

You should be building HTTP query strings using the http_build_query() function, which will handle escaping issues like the one you're having, and you should always escape HTML for output using htmlspecialchars().

print is not commonly used, but note that it's a language construct and not a function. Parentheses are not needed, and rarely used.

Inline CSS and JavaScript declarations are very 20th century and should be avoided wherever possible.

Here's how I would start to rework this code...

<?php
// assuming $allGroups is created in a loop
// the following code could be incorporated into that loop
$options = [];
foreach ($allGroups as $group) {
    $action = ($advancedPermissions && !$modGroups) ? "view" : "edit";
    $group_id = $group[0];
    $url = "groups.php?" . http_build_query(compact("action", "group_id"));
    $options[$url] = $group[0] . ($group[4] !== "Y" ? " - inactive" : "");
}


?>
<!doctype html>
<html>
<head>
    <style>
        #location_select {
            width: 200px; font-family: monospace; font-weight: bold; color: black;
        }
    </style>
</head>
<body>
    <form name=sel1>
        <select id="location_select" size="10">
    <?php foreach ($options as $value => $option): ?>
            <option value="<?= htmlspecialchars($value) ?>">
                <?= htmlspecialchars($option) ?>
            </option>
    <?php endforeach ?>
        </select>
    </form>

    <script>
        document
            .getElementById("location_selector")
            .addEventListener("change", function() {
                window.location = this.options[this.selectedIndex].value;
            });
    </script>
</body>
</html>

But if you are looking for the quick fix:

for($i = 0; $i < count($allGroups); $i++)
{
    $action = ($advancedPermissions && !$modGroups) ? "view" : "edit";
    $group_id = $allGroups[$i][0];
    $url = "groups.php?" . http_build_query(compact("action", "group_id"));
    print("<option value='$url'>");
    print($allGroups[$i][0]);
    if($allGroups[$i][2] != 'Y')
    {
        print(" - inactive");
    }
    print("</option>");
}

Well while your comments about the design are nice, this is legacy code I inherited, and I am just trying to implement a new feature. I don't have time to be refactoring the entire website. The page needs to be generated dynamically because what's on the page depends on DB data and URL parameters. I'll give http_build_query and htmlspecialchars a try. — Darin Beaudreau, Jun 15 '21 at 18:40
Well the minimal fix would be taking the first 3 lines of my loop and using it at the top of your loop to properly build the URL. I've updated my answer to include that. `htmlspecialchars()` is unlikely to make a difference with the specific problem you're having, but is obviously recommended by best practices. — miken32, Jun 15 '21 at 19:52
Turns out that the space in the group ID wasn't the problem... I noticed I had a logical condition which causes the page to not construct the info view, and I couldn't remember for what purpose I put that there, but basically, any group ID with a space in it would cause it to stop page construction. Not sure why I did that, but... oh well, it works now. Thanks! — Darin Beaudreau, Jun 16 '21 at 13:28

Sanitizing whitespace in generated URL parameters

1 Answers1