HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Framed external link redirect - vBulletin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
framedredirectvbulletinexternallink

Problem

I'm just looking for some review for my code. This is a script that checks for outbound links in posts in vBulletin forums, and if it is an external link it will redirect to a template which will load the external site in an iFrame that I also coded, inspired by themeforest and stumbleupon's way of showing external links.

This is my first PHP script on my own that is a little more advanced. I'm only use to the basics, so any critique is good critique, it's the only way I can learn.

I don't even know if I went about doing this correctly, but it seems to work how I'm wanting currently. Let me know what I can do differently.

To get an idea of this you can see it in action - Here

HTTPS links don't work, however, I still want them redirected with this so all outbound links on my site will derive from the same page, so I included a button for if a certain page won't load you can click that button and it will take you to the page.

$curdir = getcwd ();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');

if ($_GET) {
    foreach ($_GET as $key => $value) {
        if ($key=="l") {
            $url1 = $value;
        }
        else {
            $url2 = "&". $key. "=". $value;
        }
    }

    $out = strip_tags($url1. $url2);
}

$site = parse_url($_SERVER['HTTP_HOST']);
$site = $site[path];
$url = parse_url($out);

if ($url[scheme] == "http" OR "https" && $url[host]) {
    if ($site == $url[host]) {
        Header( "Location: ". $out );
    }
    else {
        eval('print_output("' . fetch_template('drc_redirect') . '");');
    }
}


I would like to add a few things to it still but first wanted to get some suggestions on my code.

Solution

Here's how I optimized your code:

$curdir = getcwd();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');

if (isset($_GET['l'])) {
    //format passed url
    $passed_url = $_GET['l'];
    unset($_GET['l']);
    if(count($_GET))
        $passed_url = $passed_url . '&' . http_build_query($_GET);

    //deals w/ the display if $passed_url is a valid URL
    if (filter_var($passed_url, FILTER_VALIDATE_URL)) {
        $site = parse_url($_SERVER['HTTP_HOST']);
        $site = $site['path'];
        $url = parse_url($passed_url);

        //the passed url is not an external link
        if ($site == $url['host']) {
            Header( "Location: ". $_SERVER['QUERY_STRING'] );
        }
        //the passed url is an external link
        else {
            eval('print_output("' . fetch_template('drc_redirect') . '");');
        }
    }
}


Below are the modifications that I did:

  • Using isset() for checking $_GET.



  • Using http_build_query() instead of looping through each element in $_GET.



  • Using filter_var() to check if the passed URL is valid.



Explanations in detail:

  • isset() allows for proper checking of the intended variable from $_GET. It also returns a boolean for the if condition, as opposed to just placing $_GET which is an array.



  • The looping of the $_GET variable will backfire if there are at least 2 elements in it. The else condition on the foreach loop overwrites the previous values for $url2. Now this can be corrected to having every $key/$value be concatenated everytime. We can utilize http_build_query() instead because it can get the job done, and there's no need to loop the $_GET array.



  • Finally, parse_url() sometimes is not able to return the scheme of a given url string. filter_var() along with the FILTER_VALIDATE_URL filter can validate URLs regardless whether the scheme was specified or not.

Code Snippets

$curdir = getcwd();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');

if (isset($_GET['l'])) {
    //format passed url
    $passed_url = $_GET['l'];
    unset($_GET['l']);
    if(count($_GET))
        $passed_url = $passed_url . '&' . http_build_query($_GET);

    //deals w/ the display if $passed_url is a valid URL
    if (filter_var($passed_url, FILTER_VALIDATE_URL)) {
        $site = parse_url($_SERVER['HTTP_HOST']);
        $site = $site['path'];
        $url = parse_url($passed_url);

        //the passed url is not an external link
        if ($site == $url['host']) {
            Header( "Location: ". $_SERVER['QUERY_STRING'] );
        }
        //the passed url is an external link
        else {
            eval('print_output("' . fetch_template('drc_redirect') . '");');
        }
    }
}

Context

StackExchange Code Review Q#51767, answer score: 2

Revisions (0)

No revisions yet.