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 ?>", "", "");
}

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");
}
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.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.

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.
• Thanks for the heads up. If I had enough score I'd give you a +1
– ILS
Feb 28 '16 at 2:20
• How does the payload get executed in this code? I'm having a hard time following the process
– ILS
Feb 28 '16 at 10:06
• @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(".
– tim
Feb 28 '16 at 10:15
• Hi @ILS, welcome to code review, and now you have enough rep to vote ;-) Apr 17 '16 at 7:26

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.

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.