6
\$\begingroup\$

Overview:

I'm using the following page template and scripts for navigation. I use a single page that loads menus and context content, and then use ajax or GET variable to load main-div's content, by passing $p variable to a php switch. Browser history is handled whith pushState function.

Main page:

<!DOCTYPE html>
<html>
  <head>
  </head>
  <body>
    <div id="main-div"> 
<?php

if (isset($_GET['p'])) {
    $p = $_GET['p'];

} else {
    $p = "main";
}


require_once("assets/switch.php");

?>
    </div>

    <script>      
    // Define state on first load
    function defineState() {
            "use strict";
            window.history.replaceState("<?php echo $p ?>", "", "");
        }   
    window.onload = defineState;


    function navegacion(dashboard, history) {

       "use strict";
       var urlPath = dashboard;
       var xmlhttp;
       var ActiveXObject;

       if (window.XMLHttpRequest) {
           // code for IE7+, Firefox, Chrome, Opera, Safari
           xmlhttp = new XMLHttpRequest();
       } else {
           // code for IE6, IE5
           xmlhttp = new ActiveXObject("Microsoft.XMLHTTP");
       }
       xmlhttp.onreadystatechange = function() {
           if (xmlhttp.readyState === 4 && xmlhttp.status === 200) {

                window.scrollTo(0, 0);

                //document.title = "TITLE";

                document.getElementById("main-div").innerHTML = xmlhttp.responseText;

                if(history !== 1) {
                    window.history.pushState(dashboard, "", "index.php?p="+urlPath);
                }

           }
       };
       xmlhttp.open("POST","assets/ajaxapi.php", true);
       xmlhttp.setRequestHeader("Content-type","application/x-www-form-urlencoded");
       xmlhttp.send("dashboard="+dashboard);

    }

    // Allows for back and forward browswe buttons

    window.onpopstate = function(e){
    "use strict";
    if(e.state){
        navegacion(e.state, 1);
    }
    };  


    </script>

  </body>
</html>

ajaxapi.php:

<?php
if (isset($_POST['dashboard'])) {
    $p = $_POST['dashboard'];

} else {
    $p = NULL;
}

require_once("./switch.php");

Could there be any performance or error issues with this code, or how could this script be enhanced?

Thanks.

\$\endgroup\$

3 Answers 3

Reset to default
6
\$\begingroup\$

Security: XSS

You are open to reflected XSS via this payload:

p=foo","","");alert(1)//

Note that the injection takes place into a JavaScript context, which is worse than injections into HTML context because browser filters have a hard time catching it (if you try the payload with eg chrome, it will execute, while "</script><script>alert(1)</script> would not).

Standard HTML-encoding would solve your problem, but OWASP recommends escaping to prevent XSS in a JS context.

Misc

  • p isn't a great variable name. If it stands for page, just use $page instead (also for the GET index).
  • As your other variable and function names are in english, navegacion should be as well.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the heads up. If I had enough score I'd give you a +1 \$\endgroup\$
    – ILS
    Commented Feb 28, 2016 at 2:20
  • \$\begingroup\$ How does the payload get executed in this code? I'm having a hard time following the process \$\endgroup\$
    – ILS
    Commented Feb 28, 2016 at 10:06
  • \$\begingroup\$ @ILS The user input is echoed in a JS context here: window.history.replaceState("[USER_INPUT]", "", "");. So what the payload does is to exit the string context via ". It then completes the function call via ,"",""); so that the script doesn't die because of errors. It then injects the command it wants to execute - alert(1) in this example. Finally, it comments out the rest of the line via //. An alternative would be to fix the remaining characters - ", "", ""); - by using them, eg via window.history.replaceState(". \$\endgroup\$
    – tim
    Commented Feb 28, 2016 at 10:15
  • \$\begingroup\$ Hi @ILS, welcome to code review, and now you have enough rep to vote ;-) \$\endgroup\$
    – janos
    Commented Apr 17, 2016 at 7:26
5
\$\begingroup\$

Seems like a simple straightforward solution. Nice job using pushState for browser history. Here's a couple quick things I noticed.

I would put your index script in the head tag. Most browsers don't care where it is but it's still good practice and it makes reading your code easier.

In your ajaxapi.php file you've got the './' path prefix on your require. You don't have this elsewhere so best to be consistent with your paths. I've also run into problems with this prefix before when trying to adjust the php include path. It's not likely to be an issue for you but it does go to show that you'll have more success without it. Since you don't need it anyway, I'd drop it.

You don't need the 'var ActiveXObject;' in your 'navegacion' function. Doesn't look like it's being used. Also not sure what the purpose of 'var urlPath = dashboard;' is in the same function. I never use strict mode though so maybe it has something to do with that? I've never had any trouble on any browsers with ajax not in strict mode, so I would consider simplifying your code.

Depending on the size of the rest of the content in your template you may want to consider dropping the js in favor of a pure php solution. You've already got the code in your index file. The js is cool but if the browser is caching most of your content anyway, it might be better to simplify your application and just reload the index page each time. This would negate the need for your back button history management, especially if you used GET parameters instead of POST. Typically I use ajax for small content updates relative to the rest of the page, or if I want to preserve some other data. Without knowing more about your app though, it's hard to say what's the best tool for the job.

\$\endgroup\$
4
\$\begingroup\$ 
var xmlhttp;
var ActiveXObject;

if (window.XMLHttpRequest) {
  // code for IE7+, Firefox, Chrome, Opera, Safari
  xmlhttp = new XMLHttpRequest();
} else {
  // code for IE6, IE5
  xmlhttp = new ActiveXObject("Microsoft.XMLHTTP");
}

Wow, I haven't seen this code for quite some time now. Consider using a library to make it easy. jQuery.ajax is readily available. If you don't want to lug around a huge library, there's the native fetch function which more or less does the same. It's relatively new, so you might need a polyfill to make it available. Using these, it will cut your code into half.

I believe you're just retrieving content. POST isn't the right verb for this, but a GET.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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