-
Notifications
You must be signed in to change notification settings - Fork 5.5k
chore: update browserslist #39054
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
Merged
+34
−32
Merged
chore: update browserslist #39054
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broadened process permissions may expose environment variables
Medium Severity
The policy change for
browserslist>baseline-browser-mappingfromprocess.exittoprocesssignificantly broadens permissions. The fullprocessobject includesprocess.env(environment variables often containing CI secrets, API keys),process.argv, and other sensitive capabilities. Other packages in this policy file use granular permissions likeprocess.exit,process.cwd, orprocess.stderr.write. Ifbaseline-browser-mappingonly needs to exit the process, the broaderprocesspermission grants unnecessary access that could be exploited if the package is compromised.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Granting unnecessary
process.envaccess to a build process dependency isn't ideal, and the fix looks simple:The
processentry can be removed by addingprocess.argvas an override (the following passesyarn lavamoat:autolocally):That said, the packages don't seem to be used in any way that could directly expose vulnerabilities, and they're both well maintained by reputable orgs. @davidmurdoch What do you think -- worth the trouble?
@naugtur It looks like policy generation came up with wider permissions than necessary here (top-level global object instead of nested capabilities). Is this expected behavior?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re "worth the trouble" - sha1-hulud has caught very reputable organizations off-guard. It's not whether we trust the org, it's whether we trust the contents of a package on npm ;)
Narrowing down these policies is a very responsible thing to do.
Policy generation is the best-effort estimation of what seems necessary for the package to work. Depending on how it accesses the globals we sometimes can figure out a subset of fields from the AST, but some AST shapes will cause us to defer to the whole global field. We can't generate a policy that's too narrow because it'd force devs to have to add overrides for many common and harmless globals.
The globals that should be preferred for narrowing down are things like
process,document, orchrome- the ones that are bags of mixed capabilities. Once we get past the basics of providing a modern version of lavamoat for node, we are planning to focus on the capabilities that need special treatment (at runtime and in policy review)Please continue to be vigilant about narrowing down policies for the most powerful capabilities. This is great work! 👏 👏 👏
Also, I'm impressed that AI can now understand implications of policy diffs.
Any ideas how to make it better?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can't be narrowed after all due to a
typeof process !== undefinedin the dependency. :(We could add relevant information or instructions to
AGENTS.md, but it seems like cursorbot and other models already have a pretty good understanding of what lavamoat does just from havingdocs/lavamoat-policy-review-process.mdin context.I asked which AST shapes would cause deferral to wider permissions here and got this response:
AST Shapes That Cause LavaMoat to Defer to Wider Permissions
LavaMoat's policy generator (lavamoat-tofu) tries to determine the most specific global access path by walking the AST's member expression chain. When it can't follow the chain, it defers to the parent/wider permission.
How the Analysis Works
For an identifier like
process, lavamoat-tofu:processMemberExpressionnodes['process', 'env', 'NODE_ENV']from that chain['process']The key function is
isNonComputedMemberLikeExpression:AST Shapes That Cause Deferral
1.
typeofCheck on Bare IdentifierAST:
Why it defers: The
processidentifier's parent isUnaryExpression, which failsisNonComputedMemberLikeExpression. Result:memberExpressions = [], so path =['process'].2. Bare Identifier in Logical/Binary Expressions
AST:
Why it defers: The left
processhasLogicalExpressionas parent. The rightprocess.envwould produce['process', 'env'], but the left one produces['process']. LavaMoat then reduces to the topmost (widest) path, keeping justprocess.3. Computed Property Access
AST:
Why it defers:
computed: truefails the check. The chain breaks, so path =['process'].4. Identifier Passed as Argument
AST:
Why it defers: Parent is
CallExpression, notMemberExpression. Path =['process'].5. Assignment to Variable (Aliasing)
AST for
const p = process:Why it defers: The
processreference is assigned top. LavaMoat seesprocesswith parentVariableDeclarator. Path =['process'].6. Destructuring
AST:
Why it defers: Same as aliasing -
processis referenced without property access. The destructuring analysis in lavamoat-tofu tries to expand this, but aRestElementor computed property in the pattern causes fallback to['process'].The Reduction Rule
After analyzing all references, lavamoat-tofu calls
reduceToTopmostApiCalls:// If array contains 'process' and 'process.env' just keep 'process'So even if some references produce
process.envand others produceprocess, the final policy contains justprocesssince it's the topmost (widest, most permissive) path that covers all accesses.Real-World Example:
baseline-browser-mappingv2.9.11The code contains:
This creates:
typeof process→ path['process'](Pattern 1)process && ...→ left side produces['process'](Pattern 2)process.env→ path['process', 'env']After reduction:
['process']wins because it's the topmost path.Summary Table
process.envMemberExpressionprocess.envtypeof processUnaryExpressionprocessprocess && xLogicalExpressionprocessprocess["env"]MemberExpression (computed)processfn(process)CallExpressionprocessconst p = processVariableDeclaratorprocessconst {env} = processVariableDeclaratorprocess