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.

Sign up for GitHub

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

Jump to bottom

src: throw on process.env symbol usage #9446

Merged
merged 1 commit into from
Nov 7, 2016
Merged

src: throw on process.env symbol usage #9446

merged 1 commit into from
Nov 7, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 3, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

This commit causes process.env to throw when a symbol is used as either a key or a value.

Fixes: #9429

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 3, 2016
@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 3, 2016
@addaleax addaleax added the process Issues and PRs related to the process subsystem. label Nov 3, 2016
bnoordhuis
bnoordhuis approved these changes Nov 3, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a request.

test/parallel/test-process-env-symbols.js
// Verify that assigning a symbol value throws.
assert.throws(() => {
process.env.foo = symbol;
}, /^TypeError: Cannot convert a Symbol value to a string$/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a symbol in process.env test?

@fhinkel
Copy link
Member

fhinkel commented Nov 3, 2016

LGTM.

@AnnaMag, this is based on your change to use the v8::setHandler(). Very cool.

jasnell
jasnell approved these changes Nov 3, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a more specific error message but this LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 4, 2016

Added requested test. CI: https://ci.nodejs.org/job/node-test-pull-request/4778/

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 4, 2016

The first CI run was green minus related failures on Windows. Pushed a second commit with Windows only fixes. Second CI run passed on Windows: https://ci.nodejs.org/job/node-test-commit/5915/

The failures on the second run seem to be unrelated, as the second commit only touched Windows code. PTAL at the second commit.

@danbev
Copy link
Contributor

danbev commented Nov 4, 2016

LGTM

@cjihrig
src: throw on process.env symbol usage
1aa595e 
This commit causes process.env to throw when a symbol is used as
either a key or a value.

Fixes: nodejs#9429
PR-URL: nodejs#9446
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@cjihrig cjihrig merged commit 1aa595e into nodejs:master Nov 7, 2016
@cjihrig cjihrig deleted the symbols branch November 7, 2016 16:24
addaleax added a commit that referenced this pull request Nov 20, 2016
@addaleax
src: allow getting Symbols on process.env
3295a7f 
1aa595e introduced a `throw` for accessing `Symbol` properties of
`process.env`. However, this breaks `util.inspect(process)` and
things like `Object.prototype.toString.call(process.env)`, so this
patch changes the behaviour for the getter to just always return
`undefined`.

Ref: #9446
Fixes: #9641
PR-URL: #9631
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@cjihrig @italoacasas
src: throw on process.env symbol usage
bda4c2b 
This commit causes process.env to throw when a symbol is used as
either a key or a value.

Fixes: nodejs#9429
PR-URL: nodejs#9446
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@addaleax @italoacasas
src: allow getting Symbols on process.env
03c79e9 
1aa595e introduced a `throw` for accessing `Symbol` properties of
`process.env`. However, this breaks `util.inspect(process)` and
things like `Object.prototype.toString.call(process.env)`, so this
patch changes the behaviour for the getter to just always return
`undefined`.

Ref: nodejs#9446
Fixes: nodejs#9641
PR-URL: nodejs#9631
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@TimothyGu TimothyGu mentioned this pull request Mar 7, 2017
Closed
3 tasks
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@JungMinu JungMinu JungMinu approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @fhinkel @danbev @bnoordhuis @jasnell @JungMinu @addaleax @nodejs-github-bot