"use strict" doesn't work + "use strict" as naive security measure discussion
- Dominant language
- JavaScript
- Stars
- 776
- Forks
- 180
- PR merge metrics
- No merged PRs in 30d
Description
It would be great if we could use strict mode, however the code injector modifies the code making the `"use strict"` not the first line of code because there is stuff to reset the canvas. The following code should raise an error, but it doesn't:
``` javascript
// jshint globalstrict: true
"use strict";
var obj = Object.freeze({});
obj.potato = 42;
```
Same problem within functions because all the `LoopProtector` stuff goes in front of your code.
I see two possible solutions to the problem:
1. Use esprima or something to recognize the use of `"use strict"` and if it is used put it at the top of the code
2. `"use strict"` by default in everyone's code. This is a bad solution because it **WILL** break people's code.
Even though `"use strict"` by default will break some code, it is worth mentioning and discussing. In strict mode, [`this` is no longer "boxed"](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode#Securing_JavaScript), meaning that the old `var win = (function() {return this;})();` trick to access the window object stops working completely.
Of course, it is still fairly trivial to access the window object. As long as you can access the `Function` constructor or any other form of eval, you can do it, and there are many ways to do so which are harder to prevent against. However, it would make things harder for people.
Still, it is a far better way of "fixing" this thing that some perceive to be a problem than doing that silly thing that was discussed a while back in #547 where every function would be esprima'd to check if `this` is the window object.
Personally, I _really_ don't want you to make `"use strict"` default because I have programs like [BMS](https://www.khanacademy.org/computer-programming/bens-module-system/4728010161913856) which currently use the boxing trick and some people _actually use BMS_ and I don't want to tell everyone who I know uses it that their programs are broken in some arcane way and they need to use a new version (although I suppose that's something that should be expected with hacky stuff like BMS).
The trick, which would be broken by `"use strict"`, is used benignly in many programs as @DozenalFTW and I explained in #547, and as I explained in https://github.com/Khan/live-editor/issues/546#issuecomment-180098342, some of the problems commonly associated with accessing window (such as `window.location`) may be solvable in less violence-inducing ways that would actually fix the problem instead of just covering it up (like `"use strict"`, BabyHint, or that `this` nonsense would).
Because of these reasons (and more), I prefer not making `"use strict"` default, but I would like it to be possible for users to `"use strict"` anyway because it is useful.
A final reason for why using moving the `"use strict"` to the top would be better that I didn't think of until now is that there are other directives such as `"use asm"` which also don't get noticed. Obviously, all code shouldn't be in `"use asm"` mode, but if you are going to fix `"use asm"`, you might as well fix all directives in the exact same way, so the solution would be to move all directives to the top of whichever block of code they happen to be in.
Anyway, sorry for this long report. **TL;DR:** I want to `"use strict"` but due to code modifications you can't put it on top of your code, so if the user uses strict mode, can you move their `"use strict"` (and all other directives) up to the top of their code or functions?
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by tracing the code injector and its LoopProtector insertion points, then compare the existing handling of user code with the esprima-based parsing mentioned in the report. Done means directives such as "use strict" and "use asm" are recognized despite injected code and apply at the intended program or function scope; the issue names no file or test to run.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- devtools
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100