33

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.

9
  • 1
    If you can reasonably expect member_id to always be an integer then <?php echo (int)$_SESSION["member_id"] ?> is advisable. In general you cannot trust the contents of a variable to produce JS compliant syntax without implementing various guardrails for anticipated data. – MonkeyZeus Feb 8 at 16:19
  • 1
    The question title says, "...into inline JS?" but the body says, "...in inline JS..." You mean "into", right? – Panzercrisis Feb 8 at 20:46
  • 2
    @hanshenrik I have a vague memory that there's some (rarely used) unicode that's valid in json but not javascript, making it not a strict subset... – Izkata Feb 9 at 15:18
  • 2
    @Izkata wow, i did some digging, that used to be correct, but is now outdated: prior to ECMAScript5/ES5 (released 2009, over 10 years ago), "\u2028\u2029" was legal JSON but illegal JS; but as of ES5/modern web browsers, that's legal (and have the same meaning) in JavaScript as well; there's another difference between json and js though, they have slightly different interpretation of [{"__proto__": []}], apparently. TIL! – hanshenrik Feb 9 at 16:25
  • 2
    Does this answer your question? Is it considered bad practice to have PHP in your JavaScript – August Janse Mar 10 at 14:08
72

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.

6
  • 12
    FWIW the correct way to do it is var someVariable = <?=json_encode($someOtherVariable,JSON_THROW_ON_ERROR);?>; , json_encode() will take care of any encoding that needs to be done (or throw an exception if the data literally cannot be represented as json, for example certain non-utf8-compatible binary data) – hanshenrik Feb 9 at 13:00
  • 2
    Another way to pass values to JS without an AJAX round-trip is by using data-attributes. I don't find setting a JS global object with PHP that wrong. In fact, some frameworks I've seen (but cannot recall) take a limited configuration from global JS vars to make it easy to set them from server-side. – Margaret Bloom Feb 9 at 20:56
  • @MargaretBloom Setting a global object via PHP in a script-Tag makes it hard to set a strict Content-Security-Policy. Other than data-attributes I've also seen meta-Tags for page-global information like CSRF-Tokens or a <script type="application/json" id="some-id">…</script> block that is parsed by the script. – cg909 Feb 10 at 17:02
  • Even if there are no apostrophes or newlines in the PHP variable, your example will likely not give what you expect. If the contents of the PHP variable is somestuff, the JavaScript variable will be somestuff"), which seems unlikely to be the desired outcome. – Janus Bahs Jacquet Mar 10 at 9:17
  • @JanusBahsJacquet thanks for spotting this. It was a typo - I did intend to write proper code but somehow ended up with an extra "). The code below was supposed to be the output from PHP. I've edited to remove the confusion. – VLAZ Mar 10 at 14:32
32

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.

6
  • 1
    Another solution is to assign a UUID or hash to every customer, and use that one when a public ID is needed. A UUID is not as easy to guess as a sequential ID. – md2perpe Feb 10 at 16:29
  • @md2perpe I'd class that more as a workaround for a fundamentally insecure API - security by obfuscation at the very most - but yes that is technically possible and will reduce the attack vector slightly. Though not really. – user384771 Feb 12 at 0:01
  • 1
    As I understand the situation, the gym customer will have a QR code and the staff will scan it. A session token, as you suggest, will then not be useful. – md2perpe Feb 12 at 9:30
  • Then some other mechanism by which a secure token is generated, not just a raw query against member IDs. – user384771 Feb 13 at 1:10
  • 1
    That's why I suggested UUID. It's not easy to guess a UUID. There are of course other types of "random" IDs that can be assigned to a customer. Do you have a suggestion? – md2perpe Feb 13 at 9:02
15

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.

1
  • In addition, (btw this answers the OP's question most directly) PHP is a loosely-typed language. While the convention might be that $SESSION['member_id'] is an integer, the PHP language makes no such guarantee. It is probably safe to echo an int directly as JavaScript, but only if you can guarantee it is an int. And you can't in this case, so htmlspecialchars(...) it is. – Greg Burghardt Feb 8 at 13:35
7

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".

2
  • Frankly, if you want something available on the window object, refer to it as window.thing, as in window.thing = "value";, rather than var thing = "value";. The former is much clearer in intent. That said, avoid globals generally (not that there aren't appropriate uses for them). – Andrew Ray Feb 9 at 19:45
  • @AndrewRay that's fair, my case running into this was making sure closure compiler wouldn't rename my global variables but then they had to be tied to the window and then I learned about the const/let/var thing – Brian Leishman Feb 9 at 20:27
2

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.

2

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.

2

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.

1

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.

0

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.

4
  • 1
    Really... Getting a downvote for this. I am certain I answered the OP's question correctly. Why the hate? – mizstereo Feb 9 at 12:26
  • 1
    I guess some other people are certain you didn't answer the question correctly. I doubt they hate you though. – npostavs Feb 9 at 20:04
  • 4
    Welcome to Stack Overflow! Your answer is helpful in pointing out that the proposed public API would leak data on other members (something others did not always notice). I've given you an upvote for this, as it's important. Downvotes may be a signal your answer could more clearly & directly answer the OP's question "It is good to escape PHP code in inline JS, or should they get the ID via AJAX and store it into a JS variable"? I absolutely encourage your wider thinking and consideration of PHP/ JS integration and API issues, as this has real value. Cheers! – Thomas W Feb 9 at 23:44
  • 1
    Why do you assume that check_if_paid_qr.php is accessed in the same session as the QR code is produced (the code that therealtsuki showed)? As I understand it, the QR code is produced in one session, but read in another (where there might not even be a session cookie). – md2perpe Feb 12 at 22:31

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