Getting 6 possible URL parameters

Question

I am in the midst of writing a simple job board and I have the URL's filtering and working how I want, everything works - I just want to know if I can improve it, if there are any security concerns, and if it is horribly inefficient.

Take the following URL section - foo.com/in/london/at/google/do/design

I assign each section a variable and work out the location, company, and category.

Other use cases that work:

• Switching the order - foo.com/at/google/in/london/do/design
• Having less parameters - foo.com/in/london/at/google

My code to figure out all these variables is:

$regex = "[^a-zA-Z0-9,-]";
$a = isset($_GET["a"]) ? preg_replace("/".$regex."/", "", $_GET["a"]) : "";
$aa = isset($_GET["aa"]) ? preg_replace("/".$regex."/", "", $_GET["aa"]) : "";
$b = isset($_GET["b"]) ? preg_replace("/".$regex."/", "", $_GET["b"]) : "";
$bb = isset($_GET["bb"]) ? preg_replace("/".$regex."/", "", $_GET["bb"]) : "";
$c = isset($_GET["c"]) ? preg_replace("/".$regex."/", "", $_GET["c"]) : "";
$cc = isset($_GET["cc"]) ? preg_replace("/".$regex."/", "", $_GET["cc"]) : "";

if ($a == "in" && $aa != null) { $searchLocation = $aa; }
if ($b == "in" && $bb != null) { $searchLocation = $bb; }
if ($c == "in" && $cc != null) { $searchLocation = $cc; }

if ($a == "at" && $aa != null) { $searchCompany = $aa; }
if ($b == "at" && $bb != null) { $searchCompany = $bb; }
if ($c == "at" && $cc != null) { $searchCompany = $cc; }

if ($a == "do" && $aa != null) { $searchCategory = $aa; }
if ($b == "do" && $bb != null) { $searchCategory = $bb; }
if ($c == "do" && $cc != null) { $searchCategory = $cc; }

if ($a == "missinghtml") { $errorUrl = true; }

I'm looking at this thinking there must be a better to do this. Is this section secure? Any thoughts on this are much appreciated. Like I say it works, but can it be better?

Answer 1

Your regex could be improved:

$regex = "[^a-zA-Z0-9,-]";
$a = isset($_GET["a"]) ? preg_replace("/".$regex."/", "", $_GET["a"]) : "";

The character , shouldn't be in a URL, but _ can be.
You don't really need to add / to the beginning and end of your regex every time.
I see a lot of people do a-zA-Z when they could really just do a-z with a case insensitive search.

Becomes:

$regex = "/[^a-z0-9-_]/i"
          ^           ^^
$a = isset($_GET["a"]) ? preg_replace($regex, "", $_GET["a"]) : "";

---

Using if ($a == "in" && $aa != null) could probably be improved also as if $aa is not null, then it evaluates to true anyway:

if ($a == "in" && $aa)

---

With if ($a == "missinghtml") { $errorUrl = true; }, you should make it into an else if loop

like:

if ($a == "in" && $aa) { $searchLocation = $aa; }
else if ($a == "missinghtml") { $errorUrl = true; }

Answer 2

In addition to the changes already proposed by Quill I strongly hope you reconsider the approach you're taking here.

I assign each section a variable ...

You can strongly simplify your code by just extracting a group match out of the URL:

$regex = '$/(in|at|do)/([^/]+)/$i';

Matching this regex will get you your $search... as the second group to extract.

under the assertion that a search-identifier will always be followed by it's value and not another identifier you can thus extract your values from the URL as follows:

$URL // here's your URL
$locationRegex = '$/in/([^/]+)/$i';
$resultcontainer;
if(preg_match($locationRegex, $URL, $resultcontainer)) {
     $searchLocation = $resultcontainer[1];
}
// and similarly for the other variables

now we should put these two approaches together...

$url // here's your URL
$pattern = '$/(in|at|do)/([^/]+)/$gi';
$resultContainer;
if (preg_match($pattern, $url, $resultContainer)) {
    // at this point your result container contains up to 6 elements:
    for ($i = 0; $i < count($resulContainer) - 1; $i = $i + 2) {
       switch ($resultContainer[$i]) {
            case "in":
               $searchLocation = $resultContainer[$i + 1];
               break;
            case "at":
               $searchCompany = $resultContainer[$i + 1];
               break;
            case "do":
               $searchCategory = $resultContainer[$i + 1];
               break;
        }

This approach basically parses the URL tokens into "key-value-pairs" which might be better stored into a different array structure, but that depends on what you need to do with this later on ;)

Answer 3

Thanks so much for your combined suggestions, my code looks like this:

$regex = "/[^a-z0-9,-\/]/i";
$queryURL = $_SERVER['REQUEST_URI']."/";
$cleanQueryURL = preg_replace("".$regex."", "", $queryURL);
if(preg_match('/in\/(.+?)\//i', $cleanQueryURL, $resultcontainer)) {
    $searchLocation = $resultcontainer[1];
} else { $searchLocation = "all"; }
if(preg_match('/at\/(.+?)\//i', $cleanQueryURL, $resultcontainer)) {
    $searchCompany = $resultcontainer[1];
} else { $searchCompany = "all"; }
if(preg_match('/do\/(.+?)\//i', $cleanQueryURL, $resultcontainer)) {
    $searchCategory = $resultcontainer[1];
} else { $searchCategory = "all"; }

I had to modify the regex slightly, but I used this - https://regex101.com/r/iZ8pV4/1 - and got there in the end!

You guys rock :)