Link shortening script

Question

I wrote this script for a link shortener just for fun.

Is it safe, or is it very easy to inject SQL have other security shortcomings? Also, do you have any tips against MySQL injection?

I don't know how to make it better. I don't know, but it tells me that this is mostly code, and I should add some details but I don't know what to write!

 <?
    $user     = check_user();
    $uid      = $user["id"];
    $uid2     = $_SESSION['userid'];
    $link1    = $_POST['link1'];
    $link2    = $_POST['link2'];
    $flink    = $_POST['url'];
    $action   = $_POST['action'];
    $action1  = $_POST['action1'];
    $action2  = $_POST['action2'];
    $faction  = $_POST['faction'];
    $adgrade  = $_POST['adgrade'];
    $monetize = $_POST['monetize'];
    // $re = '/^(?:https?\:\/\/)?[a-z0-9\-_]+\.[a-z0-9\-_\.\/%&=\?\+]+$/mi';
    $re2      = '/^[a-zA-Z]+[:\/\/]+[A-Za-z0-9\-_]+\\.+[A-Za-z0-9\.\/%&=\?\-_]+$/i';
    if (isset($link1) && isset($link2) && isset($flink) && !empty($link1) && !empty($link2) && !empty($flink)) {
        if (!preg_match("~^(?:f|ht)tps?://~i", $link1)) {
            $link1 = "http://" . $link1;
        }
        if (!preg_match("~^(?:f|ht)tps?://~i", $link2)) {
            $link2 = "http://" . $link2;
        }
        if (!preg_match("~^(?:f|ht)tps?://~i", $flink)) {
            $flink = "http://" . $flink;
        }
    }
    if (isset($link1) && isset($link2) && isset($flink) && !empty($link1) && !empty($link2) && !empty($flink)) {
        if (preg_match($re2, $flink, $matches, PREG_OFFSET_CAPTURE, 0) && preg_match($re2, $link1, $matches, PREG_OFFSET_CAPTURE, 0) && preg_match($re2, $link2, $matches, PREG_OFFSET_CAPTURE, 0)) {
            if (isset($action) && isset($action1) && isset($action2) && isset($faction)) {
                //$id = substr(md5(time().$flink), 0, 5);
                $id = substr(str_shuffle(str_repeat("aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStTuUvVwWxYyYzZ", 5)), 0, 5);
                if ($conn->query("INSERT INTO `" . $database . "`.`link` (`id`, `link1`,`link2`,`action`,`action1`,`action2`,`faction`, `flink`,`adonly`,`userid`,`adgrade`,`monetize`) VALUES ('" . $id . "','" . $link1 . "','" . $link2 . "','" . $action . "','" . $action1 . "','" . $action2 . "','" . $faction . "', '" . $flink . "','false','" . $uid . "','" . $adgrade . "','" . $monetize . "');")) {
                    $html = 'Your STU-Link is <a class="test" href="https://example.com/' . $id . '">https://example.com/' . $id . '</a>';
                } else {
                    $error_sth_wrong = "Something went wrong!";
                }
            } else {
                $error_sl_actions = "Please select all actions!";
            }
            mysqli_close($db);
        } else {
            $error_iv_urls = "Invalid url(s)!";
        }
    } else {
        $error_iv_urls2 = "Invalid url(s)!";
    }
?>

Answer 1

The are a number of problems here.

• You do a poor job of validating user input before working with it. I would strongly suggest you look at using PHP's filter_input() or filter_input_array() to provide basic data validation and error handling. You can use these to easily validate URL formats for example, or provide you own custom validation rule. You probably need to further sanitize some of your input against cross-site scripting vectors depending on how you are using the data added to your database in your application.
• You should be leveraging prepared statements for inserting your data to help mitigate against sql injection.
• You have a lot of conditional nesting complexity. You should consider adopting a coding style where you look for early exits and design away nested conditionals to whatever extent is possible.

For example:

if (empty($link1) || empty($link2) || empty($flink)) {
    // error and exit
}

// continue with rest of code, now without nesting.

This minimizes your number of code paths making your application less complex, easier to debug, and easier to maintain. Actively try to design away else conditions, as they are usually avoidable.

• Note also, in my example above that I reduced conditionals to only use emtpy() instead of isset() and empty(). empty() would fail for same conditions but is also more specific than isset(), so it is redundant to have both. Just use the more specific criteria.
• You repeat yourself unnecessarily. Look to factor things like you link re-writing logic into a function to minimize repeated code. By the way, I am not sure exactly why you use preg_match() and concatenation here. Won't that break your links? Perhaps preg_replace() instead?
• It appears to me that you a falling into classic programming anti-pattern by naming variables like $somename1, $somename2, etc. Naming like this is a possible code smell that you should be thinking about using arrays or other data structures to describe these logically-grouped values.
• Your lines of code are too long. It makes it hard to read your code.
• Variable names seem to be somewhat poorly named (not very meaningful in some cases).
• Why are you closing $db, when you are working with $conn? There is obviously context missing from example, but this seem like an error.
• It is unclear from this code example how you are validating that user making the request should be allowed to modify your database in this manner. Perhaps that is in check_user()? But typically, at a minimum any form-posting like this would have cross-site request forgery (CSRF) mitigation (i.e. session token) if not some form of user authorization.