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

fix(ses): Add Node.js domain hazard mitigation #850

Merged
merged 1 commit into from
Sep 9, 2021
Merged

fix(ses): Add Node.js domain hazard mitigation #850

merged 1 commit into from
Sep 9, 2021

Conversation

kriskowal
Copy link
Member

Fixes #126

@kriskowal
Copy link
Member Author

ht @dominictarr for investigating the technique applied here.

@kriskowal kriskowal requested a review from erights July 22, 2021 22:46
@kriskowal kriskowal self-assigned this Jul 22, 2021
@kriskowal kriskowal added audit-least-authority confinement Pertaining to confinement of guest programs. security blocker Required to make security claims on all expressly supported platforms labels Jul 22, 2021
@kriskowal
Copy link
Member Author

Looks like this is going to have to sit around until we can remove support for RESM, and this will be a breaking change.

@kriskowal kriskowal marked this pull request as draft July 22, 2021 23:40
erights
erights approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM -- I hope we can merge this for real soon!

@erights
Copy link
Contributor

erights commented Jul 23, 2021

At https://github.com/endojs/endo/pull/850/checks?check_run_id=3138995713
why would this fail by failing the sloppy check?

@kriskowal
Copy link
Member Author

At https://github.com/endojs/endo/pull/850/checks?check_run_id=3138995713
why would this fail by failing the sloppy check?

This is an easy fix for a problem I’ve seen before. Instead of using process as a free variable, I need to use globalThis.process. That keeps Parcel from noticing the dependency on process and stops it from including a Node.js shim for that global which has the unintended side-effect of turning the bundle into a sloppy mode bundle.

The error with the -r esm packaging test is not solvable. We can’t land this until all our dependees can migrate off RESM.

@kriskowal kriskowal mentioned this pull request Aug 3, 2021
Closed
@kriskowal kriskowal marked this pull request as ready for review September 2, 2021 23:02
erights
erights approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Please add a section to lockdown-options.md describing this option. While you're there, please check that it does still document all options.

LGTM

packages/ses/error-codes/SES_NO_DOMAINS.md
@@ -0,0 +1,16 @@
# SES failed to lockdown, Node.js domains have been initialized (SES_NO_DOMAINS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This text should also mention domainTaming and link to an explanation.

packages/ses/src/lockdown-shim.js Outdated
@@ -139,6 +140,7 @@ export const repairIntrinsics = (
overrideTaming = 'moderate',
overrideDebug = [],
stackFiltering = 'concise',
domainTaming = 'unsafe', // To become 'safe' by default in next-breaking-release.
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO in this comment text?

packages/ses/src/tame-domains.js
globalThis.process,
'domain',
);
if (domainDescriptor !== undefined && domainDescriptor.get !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the get test?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an important distinction and I’ve added a comment to the effect. Node.js initializes process with a process.domain === null and replaces this with a get/set pair when domain initializes.

@kriskowal kriskowal merged commit 8d083d2 into master Sep 9, 2021
@kriskowal kriskowal deleted the 126 branch September 9, 2021 02:42
AleksandM
AleksandM reviewed Apr 30, 2024
View reviewed changes
Copy link

@AleksandM AleksandM left a comment

Choose a reason for hiding this comment

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

XXX METAMASK

@AleksandM
Copy link

Looks like this is going to have to sit around until we can remove support for RESM, and this will be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AleksandM AleksandM AleksandM left review comments

@erights erights erights approved these changes

Assignees

@kriskowal kriskowal

Labels
audit-least-authority confinement Pertaining to confinement of guest programs. security blocker Required to make security claims on all expressly supported platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node.js adds a property called domain to promises
3 participants
@kriskowal @erights @AleksandM