fix: replace DefinePlugin undefined member access with undefined#21040
Conversation
When a member chain accessed a property that was not defined on a
DefinePlugin object, the whole object was inlined (e.g. `({...}).MISSING`),
leaking the entire definition. Resolve such accesses to `undefined` via the
expressionMemberChain hook instead. Closes #15559.
Replace EXPECTED_ANY with the precise CodeValue type for the resolved member-chain value.
Shorten to the project's one/two-line comment style and stop over-claiming that dotted-key object definitions are covered.
Key the undefined-member resolution on the chain root so dotted object keys (e.g. `"config.env"`) are covered, and tap callMemberChain so calling an undefined member replaces only the callee instead of inlining the object: `x.MISSING()` stays a throwing call and `x.MISSING?.()` short-circuits. Optional and computed access are covered too. Refs #15559.
🦋 Changeset detectedLatest commit: f250764 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is packaged and the instant preview is available (f8537a2). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@f8537a2
yarn add -D webpack@https://pkg.pr.new/webpack@f8537a2
pnpm add -D webpack@https://pkg.pr.new/webpack@f8537a2 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21040 +/- ##
===========================================
+ Coverage 37.94% 91.66% +53.71%
===========================================
Files 423 573 +150
Lines 48945 59912 +10967
Branches 13475 16169 +2694
===========================================
+ Hits 18574 54917 +36343
+ Misses 30371 4995 -25376
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a DefinePlugin behavior where accessing an undefined property on an object definition could inline the entire object (potentially leaking secrets), by resolving unknown member-chain accesses to undefined via JavascriptParser’s expressionMemberChain/callMemberChain hooks (issue #15559).
Changes:
- Add
expressionMemberChain/callMemberChainhandling inDefinePluginto substitute unknown object-member accesses withundefined(and avoid inlining the whole object). - Add config-case coverage for unknown member access (including dotted keys, optional chaining, and calls).
- Add a changeset entry for a patch release note.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/DefinePlugin.js |
Implements undefined-member-chain handling for object definitions and refactors object-definition detection. |
test/configCases/plugins/define-plugin/webpack.config.js |
Extends DefinePlugin test configuration with nested and dotted object keys to reproduce the issue. |
test/configCases/plugins/define-plugin/index.js |
Adds assertions covering unknown member access, optional chaining, and call-chain behavior. |
.changeset/define-plugin-undefined-member.md |
Documents the behavioral fix as a patch change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // a leaf with members left is a real property access on a value | ||
| if (!isObjectDefinition(value)) return false; | ||
| const member = members[i]; | ||
| const nextPath = `${path}.${member}`; | ||
| if (member in value) { | ||
| value = value[member]; | ||
| } else if (nextPath in definitions) { | ||
| // defined via a dotted sibling key, e.g. `OBJECT.SUB2` | ||
| value = definitions[nextPath]; | ||
| } else { |
There was a problem hiding this comment.
For the definitions-map lookup I switched to Object.prototype.hasOwnProperty.call(...) as suggested.
For member in value I kept the in operator on purpose: a define like OBJECT: { SUB: 1 } means OBJECT is that object literal, so inherited members must keep working — OBJECT.toString() should evaluate to the object's toString, not undefined. Switching to an own-property check would rewrite OBJECT.toString/OBJECT.hasOwnProperty to undefined and break those calls. Genuinely-absent properties (typos, missing secrets) are not on Object.prototype, so in already returns false for them and the object is not inlined. Accessing an actual inherited member inherently requires materializing the object, so there's no extra leak beyond what that access implies.
Generated by Claude Code
| parser.hooks.expressionMemberChain | ||
| .for(chainRoot) | ||
| .tap(PLUGIN_NAME, (expr, members) => { | ||
| if (!isUndefinedMemberAccess(members)) return; | ||
| addValueDependency(key); | ||
| return toConstantDependency(parser, "undefined")(expr); | ||
| }); |
There was a problem hiding this comment.
Good catch. isUndefinedMemberAccess now collects every define key it consults (including dotted siblings resolved via definitions[nextPath]), and both the expressionMemberChain and callMemberChain handlers call addValueDependency for each before substituting — so changing OBJECT.SUB2 invalidates modules whose output depended on it. Fixed in 372f6e2 (covers the callMemberChain case in the sibling comment too).
Generated by Claude Code
| parser.hooks.callMemberChain | ||
| .for(chainRoot) | ||
| .tap(PLUGIN_NAME, (expr, members) => { | ||
| if (!isUndefinedMemberAccess(members)) return; | ||
| addValueDependency(key); | ||
| toConstantDependency( | ||
| parser, | ||
| "undefined" | ||
| )(/** @type {Expression} */ (expr.callee)); | ||
| parser.walkExpressions(expr.arguments); | ||
| return true; |
…access
`import.meta` is a single MetaProperty chain root, not two members, so
`key.split(".")` mis-rooted it on `import`. Detect the `import.meta`
prefix when computing the chain root so undefined member access on an
`import.meta.*` object define resolves to `undefined` instead of inlining
the object. Refs #15559.
…alChaining The bundle emits `?.` syntax, which Node 10/12 cannot parse, so move the optional-chaining and optional-call assertions into separate config cases guarded by a supportsOptionalChaining test.filter. The main define-plugin case keeps only non-optional syntax so it still runs on all Node versions.
… access When an undefined member access resolves through a dotted sibling define (e.g. `OBJECT.SUB2.b` via `definitions["OBJECT.SUB2"]`), record a value dependency on every consulted key so the persistent cache invalidates when the sibling definition changes. Use an own-property check for the definitions-map lookup.
| /** @type {CodeValue} */ | ||
| let value = /** @type {CodeValue} */ (obj); | ||
| let path = key; | ||
| for (let i = chainPrefix.length; i < members.length; i++) { | ||
| // a leaf with members left is a real property access on a value | ||
| if (!isObjectDefinition(value)) return false; |
There was a problem hiding this comment.
Keeping in here intentionally. A define like OBJECT: { SUB: 1 } means OBJECT is that object literal, so inherited members must keep working: today OBJECT.SUB.toString() compiles to ({"a":1}).toString() and returns "[object Object]". Switching to hasOwnProperty would rewrite the callee to undefined, so OBJECT.SUB.toString() becomes undefined() and throws — a regression versus current behavior. Genuinely-absent properties (typos / missing secrets) are not on Object.prototype, so in already returns false for them and the object is not inlined. Accessing a real inherited member inherently requires materializing the object, so in does not leak anything beyond what that access already implies. (The definitions-map lookup does use hasOwnProperty, since config keys are own properties.)
Generated by Claude Code
| // a leaf with members left is a real property access on a value | ||
| if (!isObjectDefinition(value)) return false; | ||
| const member = members[i]; | ||
| const nextPath = `${path}.${member}`; | ||
| if (member in value) { |
There was a problem hiding this comment.
This is consistent with how the plugin already works: every instance processes its own this.definitions throughout (walkDefinitions, the expression/typeof/evaluate hooks, etc.), and the hooks.definitions waterfall is exposed for external observers rather than consumed internally. So an instance that only defines OBJECT has never resolved a sibling OBJECT.SUB2 contributed by a different instance — the member-chain lookup just follows that same per-instance scope. Making member-chain resolution operate on the merged compilation-wide definitions would be a larger, cross-cutting change (the registration side keys handlers per instance too), so I'd prefer to keep it consistent here and treat cross-instance object-key merging as a separate enhancement if it's wanted. Happy to open a follow-up if you'd like it addressed.
Generated by Claude Code
| const createHash = require("./util/createHash"); | ||
|
|
||
| /** @typedef {import("estree").Expression} Expression */ | ||
| /** @typedef {import("estree").CallExpression} CallExpression */ |
| const createHash = require("./util/createHash"); | ||
|
|
||
| /** @typedef {import("estree").Expression} Expression */ | ||
| /** @typedef {import("estree").CallExpression} CallExpression */ |
| /** | ||
| * Whether a value is a nested definition (plain object/array) to recurse into. | ||
| * @param {CodeValue} code code value | ||
| * @returns {code is Definitions} true for a plain object or array | ||
| */ | ||
| const isObjectDefinition = (code) => | ||
| Boolean(code) && | ||
| typeof code === "object" && | ||
| !(code instanceof RuntimeValue) && | ||
| !(code instanceof RegExp); |
…lution Adds a case for `OBJECT.SUB1.toString` so the walk-through branch and the fully-resolved return are exercised, and asserts inherited members stay reachable (the `in` behavior).
Types CoverageCoverage after merging claude/verify-issue-15562-zISLD into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary
DefinePlugininlined the entire object definition when code accessed a property that was not defined on it (e.g.OBJECT.UNKNOWNemitted({...}).UNKNOWN), which bloated output and could leak unrelated/secret values from the same object.This resolves such undefined member accesses to
undefinedinstead, via theexpressionMemberChainandcallMemberChainparser hooks, keyed by the chain root so it also covers:"config.env": { ... }→config.env.MISSING);a?.b,a?.["b"]);a.MISSING()stays a throwing call anda.MISSING?.()short-circuits, neither inlining the object; call arguments are still substituted;import.meta.*object keys.A value dependency is recorded for every consulted define key (including dotted siblings such as
OBJECT.SUB2) so persistent caching invalidates correctly.Closes #15559.
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes:
test/configCases/plugins/define-plugin— undefined member access and non-optional calls (runs on all Node versions);test/configCases/plugins/define-plugin-optional-chaining— optional chaining / optional calls, gated by thesupportsOptionalChainingtest filter (the emitted?.is not parseable on Node 10/12);test/configCases/plugins/define-plugin-import-meta—import.meta.*object key in an ESM build.Does this PR introduce a breaking change?
No. Direct undefined member access resolved to
undefinedat runtime before (via the inlined object) and still does. The only difference for the common case is the emitted code (no object inlined). One edge-case nuance: a chain that read further through an undefined member (e.g.OBJECT.SUB1.UNKNOWN.deep) previously threw aTypeErrorat runtime and now evaluates toundefined.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
This change was implemented with the assistance of Claude (Claude Code). AI was used to investigate the relevant
JavascriptParserhooks, implement the fix inlib/DefinePlugin.js, write the test cases, and address review feedback. All output was reviewed and verified locally by running the affected test suites (ConfigTestCases) andtsc/lint.