Is it good practice to echo PHP code into inline JS?

Question

I'm new to PHP and JS and I'm currently working on a gym management web app for our project in school. I'm adding a bit of QR functionality that sees if the user is eligible to enter the gym or not by checking if the user has paid. I want to know if it is good to escape PHP code in inline JS, or would it be better to get the id via ajax and store it into a js variable?

<script>
// ...
new QRCode(document.getElementById("qr-code"),
    "./functions/check_if_paid_qr.php?id=<?php echo $_SESSION["member_id"] ?>");
// ...
</script>

I'm using qrcode.js btw.

Answer 1

No, it's usually a bad practice.

The problem

Any time you echo something from PHP into JavaScript what's happening is that you're trying to generate valid JavaScript code. There is no generic guarantee that you do produce syntactically and semantically valid JavaScript code that also does what you want. It might be easier to guess whether individual instances of code are going to work but it's definitely not a guarantee. Take for example the following code

var someVariable = '<?php echo $someOtherVariable ?>';

This seems like it should work. And indeed it will if $someOtherVariable doesn't contain a newline or a single apostrophe. However, if it contains O'Brian or Hello\nWorld the generated JavaScript code would be invalid in either case:

Early terminating of a string literal leads to invalid code after it:

var someVariable = 'O'Brian';

Invalid multiline string:

var someVariable = 'Hello
World';

Looking at the code and determining whether the code is correct right now and will remain correct becomes very hard. What if the format of the data you're echoing changes? What if you get some data you didn't expect?

To generalise, the issue is that you don't have a complete JavaScript source code. The source code is only complete when a user visits the page and the backend produces it for them. Until then it's in limbo and it's unknown whether it will work.

Impeded code analysis

Not only is it hard for humans to determine how a code would behave, but automated tools that are there to help you might also suffer. Some examples

• Syntax highlighters may break because of the mix of the two languages. This is often the first line of defence against defective code. For example, look at the line that says 'O'Brian' - you'd see that the highlighting is inconsistent between 'O' and Brian';.
• Tools that analyse code for correctness like ESLint or Tern.js among others will not be able to analyse code that's not there. Is var someVariable = '<?php echo $someOtherVariable ?>")'; syntactically correct JavaScript? We, as humans, cannot say, an automated tool that merely follows some rules is completely unable to guess what the generated code would be.
• Tools that extract code metrics would similarly have a problem as they may not be able to parse the real JavaScript code produced.

Hard to test code

Automatic testing also suffers when you mix the two languages. You can test the code but you need to first need to boot up a PHP environment with enough data in order to generate you JavaScript code and then run tests on the JavaScript code. This is a full integration test with a lot of edge cases to cover and situations to account for. Unit test that focuses on only JavaScript and only PHP would be vastly simpler and you can make sure each fulfils their part of the contract first before checking how they work together.

Hard to debug

What all the above means is that when something happens that breaks JavaScript, you wouldn't be likely to know or even suspect. It's only going to break for some users only some of the time. How many would report it and how accurate the reports would be would vary but in my experience - don't expect much. So, if you'd know that something doesn't work is questionable to begin with. Moreover, even if you do find out that it doesn't work, you'd now have to track down which mixed JavaScript+PHP line is it. Unless there is a single one, you'd need to spend a non-zero time of investigation to find where it goes wrong. And another non-zero amount of time to find why. All that would likely happen after you've developed the application. Maybe a week, maybe a year. In the best case scenario it was you who wrote the code, so while it's still going to be quite hard, you might have some idea about where to start. However, you might have inherited this code. Or somebody else could have inherited it from you.

Bundling

Modern JavaScript is often passed through tools to produce a compact set of files from it. The bundling process will read the JavaScript source and produce a minified version of it. This can suffer if the JavaScript source is incomplete as the compilation happens before any user has ever interacted with the site.

Answer 2

The real problem here is that you're providing a public API to query whether some arbitrary member has paid.

You can take that URL, change the member ID, and retrieve what should be private information.

It doesn't matter that you're providing the member ID from the server side in PHP: you're using it to produce a URL in JavaScript which is completely visible to the browser and therefore to the end-user, and then having it use an API that must by definition be accessible to both of those entities.

Don't do this.

Use some session token instead such that you can query the payment status for the current user only.

Answer 3

It's ok to do that, but you need to be careful of code injection. If the user can modify the content of the variable, they can inject arbitrary script or tags into the page, which can be used as an avenue for cross site scripting/XSS attack.

To avoid such issues, you can escape the injected value using a json_encode() and then by htmlspecialchars().

For bigger content, rather than injecting to JS code directly, I usually recommend injecting to a non-displayed tag instead, for example:

<script type="json" id="mydata"><?= htmlspecialchars(json_encode(mydata)) ?></script>

You can then parse that from JS like so:

var mydata = JSON.parse($("#mydata").text());
// var mydata = JSON.parse(document.getElementById("mydata").innerText);

If you can be 100% sure that your injected values won't contain JS or HTML special characters, then you can inject directly.

Answer 4

Not directly about being good practice or not, but there is a "right" way to do it with JSON encoding that I didn't see mentioned here yet.

<script>
var myCoolVar = <?= json_encode($myCoolVar, JSON_HEX_TAG) ?>;
</script>

Note that <?= is syntatic sugar for <?php echo (it is not short tags, <?, that's a whole different thing, <?= is safe to use)

The important thing here is that JSON_HEX_TAG flag passed to json_encode on the PHP side of things. If your variable contains something like </script> then this will break your script tag within your html, but that flag will escape the brackets < and > with their HTML entity equivalents making sure that ending script tags don't appear in your script.

Also, another random note, but I would be inclined to use "let" or "const" here because that's how modern javascript usually looks, but keep in mind that "let" and "const" don't assign the variable to the window object within JS, so if you access your variables with window['myCoolVar'], then you'll have to declare it with "var".

Answer 5

If you already have the data while rendering server-side, it makes sense to use it rather than throwing it away and then looking it up again upon a second client-side request. But there are some reasons - not necessarily stronger ones - to prefer doing it client-side.

Content-Security-Policy, which exists to prevent injection attacks, is easier to manage tightly if all of your JavaScript is served as static files (and then makes request for dynamic data). Dynamic inline script-tags are possible but require you to calculate hashes or nonces and set headers for each request (or essentially give up on using CSP). It's good to be security-conscious when working with logins etc..

I tend to find testing more difficult when there is this kind of interaction between server and client. In order to split the system into smaller units for testing, your test tools must understand the interface used at the split. You'd have to read and build files instead of HTTP calls - certainly possible, but something I think most test tools are not as good at.

If your server always renders the same pages for each visitor (with any differences managed client-side) you can pre-render the pages and save on server costs. Assuming that the additional calls to fetch that data client-side don't eat up the savings. It depends.

If it's for a school project, I might not be concerned about these things.

Answer 6

Your code is somehow fine for some small projects, you don't need to put an extra call for retrieving what is need to be provided and it's available in the first place. Your PHP expressions would be rendered and the client won't see the code, if you see the rendered source code in the browser it would look like the following code:

</script>
...
new QRCode(document.getElementById("qr-code"), "./functions/check_if_paid_qr.php?id=421512");
</script>

But there are few important points, And what you need to take care of it are:

• Reducing redundant/extra network interaction between client and server.
• Reducing the number of DB Queries ( Database is bottle-neck )
• Keeping your code, as much as possible Simple, organized, readable, easy to understand, and maintainable
• Avoid exposing unnecessary data
• Prevent unauthorized action by not exposing unnecessary APIs to public users
• When you are accepting input from the client and you are going to show it somewhere later to avoid Cross Site Scripting/XSS attack, escape the input by htmlspecialchars() or htmlentities().
• When you are accepting input from the client and you are going to execute a Query on DB based on the provided input, You MUST DO Some action to prevent SQL Injection. Read More Here
• You need to take care of the variables that you are going to echo in your code, you need to make sure the format and data are valid before passing it as a parameter into your JS functions. Check Vlad Answer

Update 1: Thanks to @Lie Ryan, I've added some new points based on his answer.

Update 2: Thanks to @VLAZ, I've added some new points based on his answer.

Answer 7

Its good practice if you do it correctly and avoid it where convenient. The notion that you should avoid at all costs is absurd. If your JavaScript is just going to send a HTTP request to your PHP script for loading the page, then absolutely you should use PHP to add data into the webpage for the JavaScript.

Cookies are sent upon every request from a webpage. thus, you do not need to include an id for the QR code request, so you can skip the PHP altogether (which is the best thing to do when possible):

<script>
...
new QRCode(document.getElementById("qr-code"), "./functions/check_if_paid_qr.php");
</script>

The XMLHttpRequest to ./functions/check_if_paid_qr.php will automatically include the same cookies as this current request (assuming the cookies are not modified by JavaScript), so the check_if_paid_qr.php will have the same session and, thus, you can access $_SESSION["member_id"] directly from check_if_paid_qr.php.

If you really did need to include $_SESSION["member_id"] in the HTTP request, then here's the best solution. Contrary to what VLAZ said, its very possible, very easy, and very resilient to write JavaScript from PHP so long as you separate the modified bits of JavaScript into tiny manageable isolated sections with one and only one purpose. Basically, follow these rules when writing JS with PHP:

1. Never write JS expressions, block statements, arrays, or objects with PHP
2. Only write JS primitives with PHP (e.x. strings, numbers, and booleans)
3. Never allow writing multiple possible types. If something might be a string or a number, pass an additional boolean to the JavaScript indicating which type it should be so that the JS can perform additional validation/converison.

Your initial code had a problem with #2 in that it wrote neither a string nor a number nor a boolean to JS. Instead, it appended onto a JavaScript string. You should pass an independent primitive to JS like in the example below.

<script>
...
var checkIfPaidURI = <?php
    echo json_encode(rawurlencode($_SESSION["member_id"]));
?>;
new QRCode(document.getElementById("qr-code"), "./functions/check_if_paid_qr.php?id=" + decodeURI(checkIfPaidURI));
</script>

As another example, the way one would pass an object to JavaScript is by passing a URL encoded string to JavaScript's decodeURI then JSON.parse. Thus, the PHP writes a string to JS, and JS proceeds to convert that string into a safe object.

For goodness sake, please ignore Brian Leishman and don't use json_encode alone by itself. Doing json_encode('</script><script>eval(atob(`YWxlcnQoJ2hhY2tlZCcp`))</script>') doesn't escape anything, so it would lead to injection of a potentially malicious script.

Answer 8

As I understand the scenario, the QR code is produced in one session and scanned in another. The gym customer will carry the QR code and it will be scanned by staff when he wants to enter. The idea is to let the QR code contain a URL .../check_if_paid_qr.php?id=<customer identifier> which the staff will be sent to after scanning the QR code, and that page will show whether the customer has paid his fee.

If my understanding is correct, suggestions that you should just read the member ID from the session instead of injecting it via the URL will not work. There will be no session, at least not one with the customer's ID, when the QR code is scanned.

In this scenario, I see some problems:

• The QR code can be decoded by a customer who then can access the URL, and if the customer identifier is just a sequential number he can easily try other numbers to find someone who has paid. He can then recreate a QR code for that customer and use that to enter the gym. Therefore use some other type of identifier, e.g. a UUID, where it's not as easy to guess a working ID.
• All URLs that the customers can find, e.g. check_if_paid_qr.php, need to be really safe for SQL injections.

Also, I suggest that you use a PHP library for creating QR codes instead of a JavaScript library.

Answer 9

Props to OP for aiming to have good practice in coding. As to answering your question, @Jack Giffin shares the same sentiment as I do in accessing the same session ID in check_if_paid_qr.php as opposed to just injecting the session ID via PHP in the script.

Also, try to avoid exposing public APIs (your QR code API, as it would be seen on the browsers then-rendered source code) because it can lead to users easily using the API for their own benefit and reveal important data.