Sanitizing whitespace in generated URL parameters

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.

Answer

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>");
}

Advertisement

Answer