Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just developed a URL shortener.

index.php:

<?
    $sql_host = "...";
    $sql_db = "...";
    $sql_user = "...";
    $sql_pass = "...";

    $conn_error = "Could not connect to database";

    $con = mysqli_connect($sql_host, $sql_user, $sql_pass, $sql_db) or die($conn_error);

    $hash = htmlspecialchars($_GET["l"]);
    $hash = mysqli_real_escape_string($con, $hash);

    if (!(empty($hash))) {
        $query = "SELECT * FROM `links` WHERE hash = '$hash'";
        $row = mysqli_query($con, $query) or die($conn_error);
        if (mysqli_num_rows($row)) {
            $_ROW = mysqli_fetch_assoc($row);
            $url = $_ROW["link"];
            header("Location: $url");
            exit;
        } else {
            die("link not available");
        }
    }
?>
<html>
    <head>
        <title>gus URL Shortener</title>
    </head>
    <body>
        Type the link to be shortened:<br>
        <form action="short.php" method="post">
        <input type="text" name="url"><input type="submit" value="Shorten">
        </form>
    </body>
</html> 

The form redirects to short.php where the link is generated:

<html>
    <?
        $sql_host = "...";
        $sql_db = "...";
        $sql_user = "...";
        $sql_pass = "...";

        $conn_error = "Could not connect to database";

        $con = mysqli_connect($sql_host, $sql_user, $sql_pass, $sql_db) or die($conn_error);

        $url = $_POST["url"];
        $url = mysqli_real_escape_string($con, $url);
        $hash = hash("crc32", "$url");

        if (!(empty($url))) {
            if ((substr($url, 0, 7) == "http://") or (substr($url, 0, 8) == "https://")) {
                $query = "INSERT INTO `links` (hash, link)
                          VALUES ('$hash', '$url')";
                mysqli_query($con, $query) or die($conn_error);
            } else {
                die("Your link needs to start with http:// or https://");
            }
        }
    ?>
    <head>
        <title>gus URL Shortener</title>
    </head>
    <body>
        Your Link:
        <? echo "<a href='http://gus.netii.net/?l=$hash'>gus.netii.net/?l=$hash</a>";?>
    </body>
</html>

Sorry it's not commented but I think it's pretty straightforward. I've been thinking how could I do it so the link doesn't have the "?l=..." like other shorteners have.

UPDATE

Now gus.netii.net is up and running safely, like @CodeX suggested, also without the "?l=" thing after the url to GET with php ! Thanks to all and share it please.

share|improve this question
1  
url rewrite might work with.htaccess –  CodeX Jul 21 at 15:17
    
BTW is your hash meant to be a random hash? –  CodeX Jul 21 at 16:46
    
yep, random but unique –  matheussilvapb Jul 21 at 16:47
1  
google.com for example will always be db85f073 using crc32 –  CodeX Jul 21 at 16:49
1  
This is also good reading - blog.codinghorror.com/url-shortening-hashes-in-practice –  CodeX Jul 21 at 16:56

2 Answers 2

up vote 10 down vote accepted

Ok, so the main problem with your code is that it is vulnerable to SQL injection you can fix that by using prepared statements - http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

If you want to remove the ?l= you can look into Mod Rewrite - http://www.sitepoint.com/guide-url-rewriting/ and have the URL generate as gus.netii.net/$hash

I took the MySQLi Code straight from PHP.net to use in this example

index.php

$sql_host = "";
$sql_db = "";
$sql_user = "";
$sql_pass = "";

$mysqli = new mysqli($sql_host, $sql_user, $sql_pass, $sql_db);
if ($mysqli->connect_errno) {
    echo "Failed to connect to MySQL: (" . $mysqli->connect_errno . ") " . $mysqli->connect_error;
}

if(isset($_GET['l'])) {
    $hash = htmlspecialchars($_GET["l"]);
}

if (!empty($hash)) {
    if (!($stmt = $mysqli->prepare("SELECT link FROM links WHERE hash = ?"))) {
            echo "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
    }

    if (!$stmt->bind_param("s", $hash)) {
        echo "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
    }

    if (!$stmt->execute()) {
        echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
    }

    if ($stmt->execute()) {
        $stmt->bind_result($url);
        $stmt->fetch();
        header("Location: $url");
    } else {
        echo "link not available";
    }
}

short.php

$sql_host = "";
$sql_db = "";
$sql_user = "";
$sql_pass = "";

$mysqli = new mysqli($sql_host, $sql_user, $sql_pass, $sql_db);
if ($mysqli->connect_errno) {
    echo "Failed to connect to MySQL: (" . $mysqli->connect_errno . ") " . $mysqli->connect_error;
}

$url = $_POST["url"];
$hash = hash("crc32", "$url");

if (!empty($url)) {
    if ((substr($url, 0, 7) == "http://") or (substr($url, 0, 8) == "https://")) {
        if (!($stmt = $mysqli->prepare("INSERT INTO links(hash, link) VALUES (?,?)"))) {
            echo "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
        }
        if (!$stmt->bind_param("ss", $hash, $url)) {
            echo "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
        }
        if (!$stmt->execute()) {
            echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
        }
        echo "<a href='http://gus.netii.net/?l=".$hash."'>gus.netii.net/?l=".$hash."</a>";
    } 
    else {
        echo "Your link needs to start with http:// or https://";
    }
}
share|improve this answer
    
what $stmt stands for ? –  matheussilvapb Jul 21 at 22:55
4  
@matheussilvapb $stmt usually is a shorthand term for "statement", which is meant to represent a prepared statement. –  Alex L Jul 21 at 23:21

Apart from fixing the SQL injection with prepared statements as shown by CodeX you should/could do

  • Move your SQL data and possibly also the connecting to the database into a config.php
  • You didn't specify the table layout so make sure to properly set the unique property for fields
  • Consider using a different database more suited for the task. SQL databases are for relational data. You don't really have that so you could use a NoSQL system such as Redis for speed improvements.
  • Do an more through checking whether it is a valid url. E.g. only a limited character set and no succeeding periods in the hostname, ... You can use filter_var() for this
  • Not use a hash but an incrementally rising ID to save URL space. Easily doable in redis (INCR), and to shorten the length of the ID it could be represented in baseX.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.