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

util: refactor to use primordials in debuglog #38756

Closed
wants to merge 1 commit into from
Closed

util: refactor to use primordials in debuglog #38756

wants to merge 1 commit into from

Conversation

marsonya
Copy link
Member

Replace code that's vulnerable to Prototype Pollution with Primordials.

@github-actions github-actions bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels May 21, 2021
aduh95
aduh95 reviewed May 21, 2021
View reviewed changes
lib/internal/util/debuglog.js Outdated Show resolved Hide resolved
lib/internal/util/debuglog.js
.replace(/,/g, '$|^')
.toUpperCase();
debugEnv = StringPrototypeReplace(debugEnv, /[|\\{}()[\]^$+?.]/g, '\\$&');
debugEnv = StringPrototypeReplace(debugEnv, /\*/g, '.*');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should wait for V8 9.1 to land on master and use StringPrototypeReplaceAll here instead, but non blocking.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. StringPrototypeReplaceAll would be the ideal thing here.
Do we proceed with the changes in this PR or close it for now?

@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label May 21, 2021
@marsonya
util: refactor to use primordials in debuglog
c975b1d 
Replace code that's vulnerable to Prototype Pollution with Primordials.
@aduh95
Copy link
Contributor

aduh95 commented May 25, 2021

AFAICT, this part of code is run before any user code, so it's not at risk of prototype pollution. Maybe we could simply add a comment that states that prototype pollution is not a concern for this function. wdyt?

@aduh95 aduh95 mentioned this pull request May 29, 2021
Merged
@aduh95
Copy link
Contributor

aduh95 commented May 29, 2021

I've opened a concurrent PR (#38838) to implement my suggestion, adding the blocked label to make sure this doesn't land until @marsonya has time to review and decide which should we land.

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label May 29, 2021
@marsonya
Copy link
Member Author

I've opened a concurrent PR (#38838) to implement my suggestion, adding the blocked label to make sure this doesn't land until @marsonya has time to review and decide which should we land.

Thanks for implementing the new PR. Your suggestion is the right solution to go ahead with.
I am going ahead and closing this PR in favour of #38838.

@marsonya marsonya closed this May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@RaisinTen RaisinTen RaisinTen left review comments

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marsonya @aduh95 @jasnell @RaisinTen