Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node -c rejects BOM before #!, but node accepts it #27767

Closed
bakkot opened this issue May 19, 2019 · 5 comments · Fixed by #27768
Closed

node -c rejects BOM before #!, but node accepts it #27767

bakkot opened this issue May 19, 2019 · 5 comments · Fixed by #27768
Labels
confirmed-bug Issues with confirmed bugs. discuss Issues opened for discussions and feedbacks. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.

Comments

@bakkot
Copy link

bakkot commented May 19, 2019

Version: v12.1.0

$ node -e 'require("fs").writeFileSync("test.js", `\u{FEFF}#!`, "utf8")'
$ node -c test.js
(function (exports, require, module, __filename, __dirname) { #!
                                                              ^
SyntaxError: Invalid or unexpected token
$ echo $?
1
$ node test.js
$ echo $?
0

The bug is that check_syntax.js call stripShebang before stripBOM, but the actual loader calls stripBOM before stripShebag.

It's not totally clear which behavior is desired. The original bug which lead to the introduction of stripBOM actually had a script with a BOM preceding the #!, though the test included in that commit doesn't. But unix systems generally require the #! to be the first two bytes for it to be parsed as an interpreter directive, which means it cannot be preceded by a BOM.

@devsnek
Copy link
Member

devsnek commented May 19, 2019

i'd expect a failure if both are present. whether we fail because BOM is invalid js or because a shebang is invalid js seems less relevant. to do this i'd combine them into a stripShebangOrBOM function which operates on the first few bytes of the file all at once so we can't accidentally strip one and then the other.

@bakkot
Copy link
Author

bakkot commented May 19, 2019

(Strictly speaking the BOM is valid - it's parsed as whitespace.)

@devsnek
Copy link
Member

devsnek commented May 19, 2019

that's interesting. i'd still say our logic is wrong here. BOM and shebang only have meaning as the first bytes of the file.

@devsnek devsnek added confirmed-bug Issues with confirmed bugs. discuss Issues opened for discussions and feedbacks. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. labels May 19, 2019
@devsnek
Copy link
Member

devsnek commented May 19, 2019

I made this, if the logic seems alright I'll PR it. 429bf4a

@bakkot
Copy link
Author

bakkot commented May 19, 2019

That allows two BOMs: here and here.

You should probably just make a PR and this sort of code review can go there, though.

devsnek added a commit to devsnek/node that referenced this issue May 26, 2019
Fixes nodejs#27767

PR-URL: nodejs#27768
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 pushed a commit to aduh95/node that referenced this issue Jan 6, 2020
Fixes nodejs#27767

PR-URL: nodejs#27768
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Jan 8, 2020
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Jan 8, 2020
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. discuss Issues opened for discussions and feedbacks. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants