fix(security): allow dynamic stdlib member access by default; gate U006 under --lockdown (#5263)#5265
Merged
Merged
Conversation
…06 under --lockdown (#5263) The #503 dynamic-stdlib-dispatch refusal (`error[U006]`) was always-on and over-broad: it blocked legitimate packages that do dynamic member access over the linked fs namespace — graceful-fs stores its retry queue on `fs[Symbol.for('graceful-fs.queue')]`, fs-extra wraps the known `fs[method]` functions. Dynamic `fs[name]` over a *linked* namespace can only select among the methods Perry already statically linked (dynamic selection of a known set, not arbitrary code), so it is safe to allow by default. Part 1 — gate the refusal under lockdown instead of always-on: - Default `refuse_dynamic_stdlib_dispatch` is now false (allow). The thread-local default in perry-hir matches. - `--lockdown` (the supply-chain gate, #496) re-arms the refusal; the install into the HIR thread-locals now runs after the lockdown ladder is fully resolved. - An explicit `perry.allowDynamicStdlibDispatch: false` / `PERRY_ALLOW_DYNAMIC_STDLIB=0` still re-arms just this check (last wins). Part 2 — make dynamic access actually work at runtime: - String-keyed reads via the codegen NativeModuleRef fast-path (`js_native_module_property_by_name`) now consult the module-keyed override side-table, so `fs[name] = v` round-trips on reads (it already stored, but the read path skipped the table). - The top-level `fs` namespace object is now cached so a user-set symbol property (`fs[Symbol.for(...)] = queue`, keyed by object pointer in SYMBOL_PROPERTIES) round-trips — otherwise each NativeModuleRef minted a fresh object and the write was lost. Docs: dynamic-dispatch.md + lockdown.md updated to the new default + rationale. Tests: new default_allows_dynamic_stdlib_member_access HIR test; the existing refuses_* tests pin the armed gate and still pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThe PR flips ChangesDynamic stdlib dispatch: default-allow + lockdown re-arm
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-hir/src/ir/constants.rs (1)
226-228:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale documentation: default is now
false, nottrue.The doc comment says "Default is true (refusal active)" but this PR changes the default to
false. Update to match the new behavior.📝 Proposed fix
-/// `#503`: enable (true) or disable (false) the dynamic-stdlib-dispatch -/// refusal pass. Default is true (refusal active). The compile driver -/// calls this once with the resolved configuration before kicking off -/// HIR lowering for the build. +/// `#503`: enable (true) or disable (false) the dynamic-stdlib-dispatch +/// refusal pass. Default is false (allow, per `#5263`). The compile driver +/// calls this once with the resolved configuration before kicking off +/// HIR lowering for the build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/ir/constants.rs` around lines 226 - 228, Update the doc comment for the dynamic-stdlib-dispatch refusal pass flag to reflect the new default value. Change the text that currently states "Default is true (refusal active)" to indicate that the default is now false instead, ensuring the documentation accurately matches the actual default behavior implemented in this PR.
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/native_module.rs (1)
3253-3253: ⚡ Quick winAvoid allocating an override key on the common no-override path.
This fast path now runs before every built-in native-module property resolution;
native_namespace_prop_override_getcurrently formats a new key even when the override map is empty. Add an empty-map guard inside the helper to keep the default path allocation-free.♻️ Proposed helper tweak
pub(crate) fn native_namespace_prop_override_get(module: &str, prop: &str) -> Option<f64> { NATIVE_NAMESPACE_PROP_OVERRIDES.with(|m| { - m.borrow() - .get(&format!("{module}\0{prop}")) + let m = m.borrow(); + if m.is_empty() { + return None; + } + m.get(&format!("{module}\0{prop}")) .map(|bits| f64::from_bits(*bits)) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/native_module.rs` at line 3253, The native_namespace_prop_override_get function currently allocates and formats a key even when the override map is empty, which happens on every property resolution call. Add an early return guard at the beginning of the native_namespace_prop_override_get helper function to check if the override map is empty and return None immediately if it is, avoiding the unnecessary key allocation in the common case where no overrides exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-hir/src/ir/constants.rs`:
- Around line 159-164: The documentation comment in
crates/perry-hir/src/lower/expr_member.rs at lines 2373-2374 states that the
refusal pass is "on by default", but this conflicts with the new default
behavior where REFUSE_DYNAMIC_STDLIB_DISPATCH is initialized to false in
constants.rs. Update the comment in expr_member.rs to accurately reflect that
the refusal pass is now disabled by default, not enabled by default, to keep the
cross-file documentation consistent.
In `@crates/perry-runtime/src/object/native_module.rs`:
- Line 3148: The fs.default cache entry is missing the bidirectional mappings
found in other default module entries. Either add fs.default to the
cjs_default_base_module map (mapping fs.default to fs) and to the
cjs_default_namespace_name map (mapping fs to fs.default) to match the pattern
used for dns.default, child_process.default, and other cached default modules,
or remove the fs.default entry from the cache if it is not actually emitted by
any lowering path. Choose the option that aligns with the actual runtime
behavior for fs.default requests.
In `@docs/src/cli/dynamic-dispatch.md`:
- Around line 21-23: The current documentation text incorrectly groups string
and symbol writes together, stating both are "stored on the namespace," which
misrepresents the actual mechanisms. Revise the sentence to clearly distinguish
between the two storage approaches: string-keyed reads (fs[name] operations) are
handled by the override side-table, while symbol-keyed operations use namespace
caching for persistence. Replace the overly broad statement with more precise
language that explains these are two separate mechanisms, not a single namespace
storage approach.
---
Outside diff comments:
In `@crates/perry-hir/src/ir/constants.rs`:
- Around line 226-228: Update the doc comment for the dynamic-stdlib-dispatch
refusal pass flag to reflect the new default value. Change the text that
currently states "Default is true (refusal active)" to indicate that the default
is now false instead, ensuring the documentation accurately matches the actual
default behavior implemented in this PR.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module.rs`:
- Line 3253: The native_namespace_prop_override_get function currently allocates
and formats a key even when the override map is empty, which happens on every
property resolution call. Add an early return guard at the beginning of the
native_namespace_prop_override_get helper function to check if the override map
is empty and return None immediately if it is, avoiding the unnecessary key
allocation in the common case where no overrides exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22ff7541-5b45-4060-a1c6-17bb960e3a84
📒 Files selected for processing (7)
crates/perry-hir/src/ir/constants.rscrates/perry-hir/tests/dynamic_stdlib_dispatch.rscrates/perry-runtime/src/object/native_module.rscrates/perry/src/commands/compile/host_config.rscrates/perry/src/commands/compile/types.rsdocs/src/cli/dynamic-dispatch.mddocs/src/cli/lockdown.md
…ead fs.default cache entry, clarify string-key storage docs - expr_member.rs: refusal pass is OFF by default since #5263 (re-armed under lockdown/opt-out) - native_module.rs: remove dead `fs.default` namespace-cache arm (referenced nowhere else; no cjs_default_* mappings emit it; the `fs` arm backs symbol-key persistence). fs[symbol] + fs[name] verified still working; dynamic_stdlib tests 24/24. - docs/dynamic-dispatch.md: clarify string keys persist via module-keyed override table, symbol keys on cached namespace object
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #5263 (corpus Pattern B — blocks graceful-fs + fs-extra).
Problem
The
#503dynamic-stdlib-dispatch refusal (error[U006]) was always-on and over-broad. Minimal repro:It blocked legitimate packages: graceful-fs stores its retry queue on
fs[Symbol.for('graceful-fs.queue')]; fs-extra wraps the knownfs[method]functions.Safety rationale
Dynamic
fs[name]over a linked namespace can only reach the methods Perry already statically linked — dynamic selection among a known set, not a way to reach arbitrary code. The dispatch-by-string obfuscation it could hide is only meaningful alongside the arbitrary-code surfaces--lockdownalready forbids (eval/Function,child_process, native archives). So the refusal belongs with the lockdown gate, not always-on. Default-allow is safe.Change
Part 1 — gate U006 under lockdown (default-allow)
refuse_dynamic_stdlib_dispatchis nowfalse(allow); the perry-hir thread-local default matches.--lockdown/perry.lockdown: true/PERRY_LOCKDOWN=1re-arms the refusal. The HIR thread-local install now runs after the lockdown ladder is fully resolved.perry.allowDynamicStdlibDispatch: false/PERRY_ALLOW_DYNAMIC_STDLIB=0still re-arms just this check (last wins), without the rest of lockdown.Part 2 — runtime dynamic-access support
NativeModuleReffast-path (js_native_module_property_by_name) now consults the module-keyed override side-table, sofs[name] = vround-trips on reads (writes already stored viaNATIVE_NAMESPACE_PROP_OVERRIDES; the read path skipped it).fsnamespace object is now cached, so a user-set symbol property (fs[Symbol.for(...)] = queue, keyed by object pointer inSYMBOL_PROPERTIES, GC-scanned) round-trips. Without caching, eachNativeModuleRefminted a fresh object and the write was lost.(Optional notice recording of allowed sites was skipped to avoid per-build noise on graceful-fs/fs-extra-heavy graphs; the access is documented as safe-by-default.)
Test evidence
Default build:
Gate still refuses when armed:
graceful-fs / fs-extra (corpus, cwd=corpus)
drivers/graceful_fs_.tserror[U006](compile)TypeError: Object prototype may only be an Object or null: undefined(graceful-fsclone.js{ __proto__: getPrototypeOf(obj) }/Object.create— separate codegen/runtime issue, not fixed here)drivers/fs_extra_.tserror[U006](compile)The dynamic
fs[gracefulQueue]symbol access (graceful-fs.js:43/86) is exactly what this PR enables — verified working in isolation.Tests
default_allows_dynamic_stdlib_member_accessHIR test (graceful-fs + fs-extra shapes).refuses_*tests pin the armed gate (set_refuse_dynamic_stdlib_dispatch(true)) and still pass:cargo test -p perry-hir --test dynamic_stdlib_dispatch→ 24 passed.cargo test -p perry-hir --lib→ 154 passed;cargo test -p perry --bins→ 577 passed.perry-hirintegration testcheerio_call_rewritefails to compile on clean HEAD (missingbyte_offsetinExpr::Callliterals) — not touched by this PR.Version / changelog
Per CONTRIBUTING, version bump (
Cargo.toml/Current Version) andCHANGELOG.mdentry are left for the maintainer to fold in at merge. NoCargo.lockchurn.Summary by CodeRabbit
Release Notes
--lockdownor when explicitly re-enabled via configuration/env.fsand ensured user overrides are applied via the fast property-read path.