feat(ext): perry-ext-node-forge — native node-forge PKI, working end-to-end from TypeScript - #7033
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis change adds a native RustCrypto-backed ChangesNative node-forge binding
Supply-chain soak and security tooling
Compiler, runtime, and archive updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
crates/perry-ext-node-forge/src/lib.rs (1)
587-657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the canonical
{ attributes: [...] }DN shape.Both tests feed bare arrays, yet the doc comment on Lines 141-145 calls
DnJson::Wrappedthe canonical shape produced bysetSubject/setIssuerandcertificateFromPem— i.e. exactly whatsignsees in production is the untested branch.💚 Suggested test
#[test] fn cert_spec_parses_wrapped_dn_shape() { let json = r#"{ "publicKey": { "pem": "x" }, "serialNumber": "01", "validity": { "notBefore": 1700000000000, "notAfter": 1800000000000 }, "subject": { "attributes": [{ "name": "commonName", "value": "example.com" }] }, "issuer": { "attributes": [{ "name": "commonName", "value": "Socket Security CA" }] } }"#; let spec = cert_spec_from_json(json).expect("parse"); assert_eq!(spec.subject[0].value, "example.com"); assert_eq!(spec.issuer[0].value, "Socket Security CA"); }🤖 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-ext-node-forge/src/lib.rs` around lines 587 - 657, Add a unit test alongside cert_spec_parses_sfw_shaped_json and ca_ext_shape_from_json that passes subject and issuer as objects containing an attributes array, then calls cert_spec_from_json and verifies both parsed DN values. Cover the canonical DnJson::Wrapped shape without changing the existing bare-array tests.crates/perry-ext-node-forge/tests/openssl_e2e.rs (1)
102-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed shared temp path collides across concurrent runs and is never cleaned up.
temp_dir()/perry_node_forge_e2e/{ca,leaf}.pemis a constant path; two concurrent runs (parallel CI jobs on one runner, or a local run alongside CI) overwrite each other's fixtures mid-verify. Include the pid/nanos in the directory name, or usetempfile::TempDirso it self-cleans.🤖 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-ext-node-forge/tests/openssl_e2e.rs` around lines 102 - 113, Update the temporary fixture setup in the OpenSSL end-to-end test to use a unique per-run directory, preferably via tempfile::TempDir so cleanup occurs automatically; otherwise include sufficient process/time uniqueness in the directory name. Keep ca.pem and leaf.pem creation within that isolated directory and preserve the existing verification flow.scripts/soak/external-tools.mts (1)
151-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "valid + expired bypass" predicate.
staleBypassesandpruneExpiredSoakBypassescarry byte-identical filter logic (validity, arithmetic, expiry). Any future rule change has to be made twice or the warn/prune paths silently diverge.♻️ Suggested shape
+function isClearedBypass(bypass: NonNullable<ToolPin['soakBypass']>, today: string): boolean { + if (!isValidIsoDate(bypass.published) || !isValidIsoDate(bypass.removable)) { + return false + } + // Wrong-arithmetic annotations are a hard checkPins failure, never prunable. + if (bypass.removable !== addDaysIso(bypass.published, SOAK_DAYS)) { + return false + } + return bypass.removable < today +}Then both functions reduce to a loop plus
isClearedBypass(bypass, today).🤖 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 `@scripts/soak/external-tools.mts` around lines 151 - 205, Extract the duplicated bypass validity, arithmetic, and expiry checks from staleBypasses and pruneExpiredSoakBypasses into a shared isClearedBypass predicate that accepts a bypass and today. Update both functions to use this predicate while preserving their existing name collection and deletion behavior..github/workflows/security-audit.yml (1)
67-113: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding explicit job-level
permissions:to the new jobs.
soak-gateandagent-scaninherit whatever the workflow-level block grants; the siblingcargo-deny/zizmorjobs and the zizmor ratchet note both treat a missing block as debt.contents: readis all either needs.🤖 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 @.github/workflows/security-audit.yml around lines 67 - 113, Add an explicit job-level permissions block with contents: read to both soak-gate and agent-scan, matching the least-privilege configuration used by the sibling audit jobs and preserving their existing steps unchanged..github/workflows/soak-autofix.yml (1)
68-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
git diff --quietignores untracked files.The stash below is
--include-untracked, so the script anticipates fixer-created files; this early exit would silently discard them.git status --porcelaincovers both.♻️ Proposed change
- if git diff --quiet; then + if [ -z "$(git status --porcelain)" ]; then echo "soak surfaces clean — nothing to fix" exit 0 fi🤖 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 @.github/workflows/soak-autofix.yml around lines 68 - 71, Update the clean-working-tree check in the soak autofix workflow to use git status --porcelain instead of git diff --quiet, so fixer-created untracked files are detected before exiting. Preserve the existing clean message and successful exit when the porcelain status is empty.crates/perry-hir/tests/node_forge_namespace_lowering.rs (1)
1-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this lowering coverage into cargo-test-visible unit tests.
This integration-suite path is not the preferred PR-visible coverage location. As per coding guidelines, “Prefer acceptance coverage in
cargo-test-visible unit tests because integration suites undercrates/*/tests/*.rsdo not run on every PR.”🤖 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/tests/node_forge_namespace_lowering.rs` around lines 1 - 105, Move the namespace-lowering tests and their helpers from the integration-test module into the appropriate cargo-test-visible unit-test module within the HIR implementation. Preserve coverage for generateKeyPair, createCertificate, and create, including the node-forge module assertion, while adapting imports and visibility to the unit-test location.Source: Coding guidelines
🤖 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 @.claude/skills/soak/SKILL.md:
- Line 17: Update the changelog fragment `6912-security-tooling-soak` to use the
Cargo configuration key `global-min-publish-age` instead of `min-publish-age`,
matching the key parsed by `soak.mts` and the skill table.
In @.github/workflows/security-audit.yml:
- Line 22: Update the checkout steps in both the security-audit and cargo-deny
jobs to disable credential persistence by setting persist-credentials to false,
matching the other jobs in the workflow; leave the pinned actions/checkout
references unchanged.
- Around line 128-137: Update the “Scan each skill” workflow step to read the
skillspector version from external-tools.json via jq instead of hardcoding the
Git ref, and invoke the repository’s locked uv project when available using its
documented locked-install/reproducibility flow. Ensure the scan still runs for
every skill while avoiding runtime resolution of an independently pinned
transitive dependency tree.
In @.github/workflows/soak-autofix.yml:
- Around line 89-94: Update the existing-branch path around the git fetch and
checkout commands to fetch the branch into the remote-tracking ref expected by
`git checkout -B "$BRANCH" "origin/$BRANCH"` (or instead base checkout directly
on `FETCH_HEAD`). Ensure the fetch provides sufficient history for the later
push to remain fast-forwardable, using an unshallow/deeper fetch or full
checkout history.
In `@changelog.d/7021-real-npm-cli-compile-gaps.md`:
- Around line 1-7: Update the changelog heading to state “five independent
blockers” instead of “four,” leaving the five listed blocker entries unchanged.
In `@crates/perry-ext-node-forge/Cargo.toml`:
- Around line 29-50: Move the perry-runtime dependency from [dev-dependencies]
into the normal [dependencies] section used to build the staticlib, preserving
the ["default", "stdlib"] feature set. Ensure cargo build -p
perry-ext-node-forge bundles the full-featured runtime without relying on
dev-dependency activation.
In `@crates/perry-ext-node-forge/src/crypto.rs`:
- Around line 342-359: Update the extension-building flow around
BasicConstraintsSpec and KeyUsageSpec so their parsed critical flags determine
the emitted Extension critical bit instead of relying on each type’s AsExtension
implementation. Apply the requested value in both the basic constraints and key
usage branches, preserving the existing extension contents and error
propagation.
- Around line 176-233: Make parse_name/build_name lossless for imported
certificate names: extend Attr or the surrounding representation to preserve
each attribute’s original ASN.1 value/tag and have build_name reuse it instead
of always encoding Utf8String. Update name_for to return the unmapped OID’s
dotted string, ensuring oid_for accepts it rather than producing "unknown";
preserve existing mapped-name behavior and attribute ordering.
In `@crates/perry-ext-node-forge/src/lib.rs`:
- Around line 483-488: Update wrap_dn_attributes to detect when the incoming
attrs_bits value already represents an object containing an attributes field,
and return/store that object unchanged instead of wrapping it again. Preserve
the current wrapper behavior for raw attribute arrays so setIssuer accepts both
subject and subject.attributes shapes without producing nested attributes.
- Around line 539-568: The FFI certificate-signing path must surface invalid
inputs instead of silently returning. In crates/perry-ext-node-forge/src/lib.rs
lines 539-568, update js_node_forge_cert_sign so every validation failure and
build_and_sign error is reported through perry_ffi (or, at minimum, logged),
rather than leaving signaturePem unset. In
crates/perry-ext-node-forge/src/lib.rs lines 203-213, change parse_time to
return Result<i64, String> and propagate missing or invalid
validity.notBefore/notAfter errors through cert_spec_from_json instead of
defaulting to the epoch.
In `@crates/perry-ext-node-forge/tests/openssl_e2e.rs`:
- Around line 36-39: Update the openssl_available handling in the e2e test so
missing OpenSSL fails by default, allowing the current early return only when
PERRY_SKIP_OPENSSL_E2E=1 is explicitly set. Add a cargo-test-visible unit test
in crypto.rs that independently asserts the certificate DN and
chain-verification invariants, ensuring acceptance coverage does not depend
solely on the integration suite.
- Around line 42-100: Update the certificate validity timestamps in the test
setup around `ca_spec` and `leaf_spec` to derive from `SystemTime::now()`
instead of fixed Unix values. Use a validity window covering approximately one
day before the current time through 365 days after it, applying the same dynamic
`not_before_unix` and `not_after_unix` treatment to both certificates while
preserving their existing specifications.
In `@crates/perry-hir/src/lower/expr_call/native_module.rs`:
- Around line 261-290: Update try_node_forge_namespace to validate the complete
node-forge member path against supported native-module paths before emitting
NativeMethodCall, rather than matching only the final identifier. Preserve
supported paths such as md.sha256.create and pki.rsa.generateKeyPair, while
letting unsupported paths like forge.md.md5.create fall through as unresolved.
In `@crates/perry-runtime/src/event_target.rs`:
- Around line 337-358: Root this_value and the converted message/name values
with RuntimeHandleScope in js_dom_exception_subclass_init; after each allocating
coercion, reload exception, message_ptr, and name_ptr before dereferencing or
storing them. In crates/perry-runtime/src/object/global_this/ctor_thunks.rs
lines 53-64, root the boxed pattern/flag strings through both coercions and
js_regexp_new, then reload their raw pointers immediately before use.
In `@crates/perry/src/commands/compile/optimized_libs/freshness.rs`:
- Around line 201-226: Update declared_feature_names to initialize names as an
empty BTreeSet when the manifest lacks a [features] table, then continue
collecting optional dependencies from regular and target-specific dependency
tables. Preserve returning None only for unreadable or invalid manifests, and
add a regression test covering a manifest that declares features solely through
optional dependencies.
In `@crates/perry/well_known_bindings.toml`:
- Around line 303-315: Remove the duplicate [bindings.node-forge] and
[bindings.node-forge.upstream] table headers in the node-forge binding
configuration, retaining a single header for each table while preserving all
associated fields.
In `@scripts/soak/external-tools.mts`:
- Around line 612-618: Update the environment setup immediately before exec sfw
in the shimmed package-manager flow so SFW_UNKNOWN_HOST_ACTION is only assigned
to ignore when the caller has not already provided a value. Preserve an explicit
caller-provided action, including block, and avoid unconditionally exporting
ignore for enterprise or CI invocations.
- Around line 268-271: Update the channel validation near the `channel`
extraction to reuse `installedToolchains` instead of the brittle
`dockerBody.includes` substring check. Adjust the `installedToolchains`
parsing/filtering so named channels such as `nightly-*` and `stable`, including
channels appearing as later arguments, are retained and correctly recognized.
In `@scripts/soak/soak.mts`:
- Around line 258-299: Update parseExcludeEntries to recognize valid zero-indent
list items under minimumReleaseAgeExclude and ensure any item whose indentation
differs from the established blockIndent is still emitted as a finding rather
than silently skipped. Preserve the existing block termination behavior for
non-item top-level content, and retain each entry’s name, line, and annotation
metadata so checkExcludeAnnotations/staleExcludes can validate it.
---
Nitpick comments:
In @.github/workflows/security-audit.yml:
- Around line 67-113: Add an explicit job-level permissions block with contents:
read to both soak-gate and agent-scan, matching the least-privilege
configuration used by the sibling audit jobs and preserving their existing steps
unchanged.
In @.github/workflows/soak-autofix.yml:
- Around line 68-71: Update the clean-working-tree check in the soak autofix
workflow to use git status --porcelain instead of git diff --quiet, so
fixer-created untracked files are detected before exiting. Preserve the existing
clean message and successful exit when the porcelain status is empty.
In `@crates/perry-ext-node-forge/src/lib.rs`:
- Around line 587-657: Add a unit test alongside
cert_spec_parses_sfw_shaped_json and ca_ext_shape_from_json that passes subject
and issuer as objects containing an attributes array, then calls
cert_spec_from_json and verifies both parsed DN values. Cover the canonical
DnJson::Wrapped shape without changing the existing bare-array tests.
In `@crates/perry-ext-node-forge/tests/openssl_e2e.rs`:
- Around line 102-113: Update the temporary fixture setup in the OpenSSL
end-to-end test to use a unique per-run directory, preferably via
tempfile::TempDir so cleanup occurs automatically; otherwise include sufficient
process/time uniqueness in the directory name. Keep ca.pem and leaf.pem creation
within that isolated directory and preserve the existing verification flow.
In `@crates/perry-hir/tests/node_forge_namespace_lowering.rs`:
- Around line 1-105: Move the namespace-lowering tests and their helpers from
the integration-test module into the appropriate cargo-test-visible unit-test
module within the HIR implementation. Preserve coverage for generateKeyPair,
createCertificate, and create, including the node-forge module assertion, while
adapting imports and visibility to the unit-test location.
In `@scripts/soak/external-tools.mts`:
- Around line 151-205: Extract the duplicated bypass validity, arithmetic, and
expiry checks from staleBypasses and pruneExpiredSoakBypasses into a shared
isClearedBypass predicate that accepts a bypass and today. Update both functions
to use this predicate while preserving their existing name collection and
deletion behavior.
🪄 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: 517f799d-727b-4f25-934b-9afcd306abb6
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (51)
.cargo/config.toml.claude/skills/soak/SKILL.md.github/dependabot.yml.github/workflows/security-audit.yml.github/workflows/soak-autofix.yml.github/workflows/zizmor.yml.github/zizmor.yml.npmrcCargo.tomlchangelog.d/6912-security-tooling-soak.mdchangelog.d/7021-real-npm-cli-compile-gaps.mdchangelog.d/7033-ext-node-forge.mdcrates/perry-api-manifest/src/entries.rscrates/perry-api-manifest/src/entries/part_2.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/native_table/utils_crypto.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-ext-node-forge/Cargo.tomlcrates/perry-ext-node-forge/src/crypto.rscrates/perry-ext-node-forge/src/lib.rscrates/perry-ext-node-forge/tests/openssl_e2e.rscrates/perry-ext-zlib/Cargo.tomlcrates/perry-ext-zlib/src/stream.rscrates/perry-hir/src/js_transform/local_natives.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/tests/node_forge_namespace_lowering.rscrates/perry-runtime/src/event_target.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/ctor_thunks.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/optimized_libs/driver.rscrates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/strip_dedup.rscrates/perry/well_known_bindings.tomlexternal-tools.jsonpackage.jsonscripts/soak/constants.mtsscripts/soak/external-tools.mtsscripts/soak/external-tools.test.mtsscripts/soak/paths.mtsscripts/soak/soak.mtsscripts/soak/soak.test.mtsscripts/soak/update-deps.mtsscripts/soak/update-deps.test.mtstools/package.jsontools/pnpm-workspace.yamltools/taze.config.mts
| if git fetch origin "$BRANCH"; then | ||
| git checkout -B "$BRANCH" "origin/$BRANCH" | ||
| else | ||
| echo "no existing $BRANCH on origin — creating it" | ||
| git checkout -B "$BRANCH" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
origin/$BRANCH likely won't exist after this fetch, so the "existing branch" path fails.
actions/checkout configures a single-branch refspec (+refs/heads/<ref>:refs/remotes/origin/<ref>) and a depth-1 clone. git fetch origin "$BRANCH" under that config succeeds but only writes FETCH_HEAD — the refs/remotes/origin/bot/soak-autofix ref is never created, so git checkout -B "$BRANCH" "origin/$BRANCH" errors out (and set -e fails the run) on every day where a bot branch already exists. Fetch an explicit refspec, or base off FETCH_HEAD.
🐛 Proposed fix
- if git fetch origin "$BRANCH"; then
- git checkout -B "$BRANCH" "origin/$BRANCH"
+ if git fetch --depth=1 origin "+refs/heads/$BRANCH:refs/remotes/origin/$BRANCH"; then
+ git checkout -B "$BRANCH" "refs/remotes/origin/$BRANCH"
elseA shallow base also makes the later git push non-fast-forwardable if the branch has more than one commit — consider --unshallow/--depth=100 on that fetch, or fetch-depth: 0 on the checkout step.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if git fetch origin "$BRANCH"; then | |
| git checkout -B "$BRANCH" "origin/$BRANCH" | |
| else | |
| echo "no existing $BRANCH on origin — creating it" | |
| git checkout -B "$BRANCH" | |
| fi | |
| if git fetch --depth=1 origin "+refs/heads/$BRANCH:refs/remotes/origin/$BRANCH"; then | |
| git checkout -B "$BRANCH" "refs/remotes/origin/$BRANCH" | |
| else | |
| echo "no existing $BRANCH on origin — creating it" | |
| git checkout -B "$BRANCH" | |
| fi |
🤖 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 @.github/workflows/soak-autofix.yml around lines 89 - 94, Update the
existing-branch path around the git fetch and checkout commands to fetch the
branch into the remote-tracking ref expected by `git checkout -B "$BRANCH"
"origin/$BRANCH"` (or instead base checkout directly on `FETCH_HEAD`). Ensure
the fetch provides sufficient history for the later push to remain
fast-forwardable, using an unshallow/deeper fetch or full checkout history.
| fn declared_feature_names(workspace_root: &Path, krate: &str) -> Option<BTreeSet<String>> { | ||
| let manifest_path = workspace_root.join("crates").join(krate).join("Cargo.toml"); | ||
| let manifest: toml::Value = toml::from_str(&fs::read_to_string(manifest_path).ok()?).ok()?; | ||
| let mut names: BTreeSet<String> = manifest | ||
| .get("features")? | ||
| .as_table()? | ||
| .keys() | ||
| .cloned() | ||
| .collect(); | ||
| let mut collect_optional = |deps: Option<&toml::Value>| { | ||
| let Some(table) = deps.and_then(|d| d.as_table()) else { | ||
| return; | ||
| }; | ||
| for (name, spec) in table { | ||
| if spec.get("optional").and_then(|o| o.as_bool()) == Some(true) { | ||
| names.insert(name.clone()); | ||
| } | ||
| } | ||
| }; | ||
| collect_optional(manifest.get("dependencies")); | ||
| if let Some(targets) = manifest.get("target").and_then(|t| t.as_table()) { | ||
| for target_spec in targets.values() { | ||
| collect_optional(target_spec.get("dependencies")); | ||
| } | ||
| } | ||
| Some(names) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file context =="
wc -l crates/perry/src/commands/compile/optimized_libs/freshness.rs
sed -n '160,260p' crates/perry/src/commands/compile/optimized_libs/freshness.rs
echo "== usages of declared_feature_names =="
rg -n "declared_feature_names|filter_features|features" crates/perry/src/commands/compile/optimized_libs/freshness.rs crates/perry/src/commands/compile -S || true
echo "== tests mentioning declared_feature_names =="
rg -n "declared_feature_names|optional.*feature|features" crates -S | head -200Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== regression tests around feature filtering =="
sed -n '620,710p' crates/perry/src/commands/compile/optimized_libs/tests.rs
echo "== build invocation around filtered features =="
sed -n '560,620p' crates/perry/src/commands/compile/optimized_libs/driver.rs
sed -n '720,760p' crates/perry/src/commands/compile/optimized_libs/driver.rs
echo "== static manifests lacking runtime/stdlib features =="
for f in crates/perry-runtime-static/Cargo.toml crates/perry-ext-fetch/Cargo.toml crates/perry-ext-http/Cargo.toml crates/perry-wasm-host/Cargo.toml; do
echo "--- $f"
sed -n '1,80p' "$f"
done
echo "== Rust optional dependency semantics probe if rustc exists =="
if command -v rustc >/dev/null 2>&1; then
rm -rf /tmp/optional_dep_probe
mkdir -p /tmp/optional_dep_probe
cat >/tmp/optional_dep_probe/Cargo.toml <<'EOF'
[package]
name = "optional_dep_probe"
version = "0.0.0"
edition = "2021"
[dependencies]
optional_dependency = { path = "./optional_dependency", optional = true }
EOF
cat >/tmp/optional_dep_probe/src/main.rs <<'EOF'
fn main() {
println!("{:?}", cfg!(feature = "optional_dependency"));
}
EOF
mkdir -p /tmp/optional_dep_probe/optional_dependency
cat >/tmp/optional_dep_probe/optional_dependency/Cargo.toml <<'EOF'
[package]
name = "optional_dependency"
version = "0.0.0"
edition = "2021"
EOF
mkdir -p /tmp/optional_dep_probe/optional_dependency/src
echo "" >/tmp/optional_dep_probe/optional_dependency/src/lib.rs
cd /tmp/optional_dep_probe
cargo check --no-default-features || true
cargo check --no-default-features --features optional_dependency || true
else
echo "rustc not available"
fiRepository: PerryTS/perry
Length of output: 16244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, textwrap, subprocess, tempfile, pathlib
toml_data = {
"package": {"name": "perry-runtime", "version": "0.0.0", "edition": "2021"},
"dependencies": {"my_dep": {"version": "0.1", "optional": True}},
}
manifest = json.dumps({"[dependencies]my_dep": "version = \"0.1\"\noptional = true", "package.name": "perry-runtime", "package.version": "0.0.0", "package.edition": "2021"}, indent=None)
print("features absent:", ".get('features') is None")
print("optional dependencies absent:", "no collect_optional(path)")
PY
# Read-only check: compute declared_feature_names behavior for manifests missing [features] but containing optional dependencies.
python3 - <<'PY'
import re
from pathlib import Path
source = Path("crates/perry/src/commands/compile/optimized_libs/freshness.rs").read_text()
func = re.search(r'fn declared_feature_names\([^)]*\)\s*-> Option<BTreeSet<String>>\s*\{(?P<body>.+?)\n}\n\n/// The `perry` binary', source, re.S).group('body')
print("declared_feature_names contains features required access:", ".get(\"features\")?")
print("declared_feature_names contains target optional deps:", ".get(\"target\").and_then(|t| t.as_table())")
# Simulate only the missing-feature return path without executing repo code.
def declared_names(features_present):
if not features_present:
# `manifest.get("features")?` short-circuits to None.
return None
# placeholder: would collect optional deps after features in real source.
return {"features_only"}
print("absent_features ->", declared_names(False))
print("present_features ->", declared_names(True))
PY
# Inspect whether crates/perry-runtime or perry-stdlib manifests normally declare features.
for crate in cratesRepository: PerryTS/perry
Length of output: 535
Do not require an explicit [features] table.
declared_feature_names returns None for manifests that expose features only through optional dependencies, so retain_workspace_declared_features skips filtering and can pass unknown crate features into cargo build --features…. Initialize names as empty when [features] is absent, then collect optional dependencies; add a regression test for that manifest shape.
Proposed fix
- let mut names: BTreeSet<String> = manifest
- .get("features")?
- .as_table()?
- .keys()
- .cloned()
- .collect();
+ let mut names: BTreeSet<String> = manifest
+ .get("features")
+ .and_then(|value| value.as_table())
+ .map(|features| features.keys().cloned().collect())
+ .unwrap_or_default();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn declared_feature_names(workspace_root: &Path, krate: &str) -> Option<BTreeSet<String>> { | |
| let manifest_path = workspace_root.join("crates").join(krate).join("Cargo.toml"); | |
| let manifest: toml::Value = toml::from_str(&fs::read_to_string(manifest_path).ok()?).ok()?; | |
| let mut names: BTreeSet<String> = manifest | |
| .get("features")? | |
| .as_table()? | |
| .keys() | |
| .cloned() | |
| .collect(); | |
| let mut collect_optional = |deps: Option<&toml::Value>| { | |
| let Some(table) = deps.and_then(|d| d.as_table()) else { | |
| return; | |
| }; | |
| for (name, spec) in table { | |
| if spec.get("optional").and_then(|o| o.as_bool()) == Some(true) { | |
| names.insert(name.clone()); | |
| } | |
| } | |
| }; | |
| collect_optional(manifest.get("dependencies")); | |
| if let Some(targets) = manifest.get("target").and_then(|t| t.as_table()) { | |
| for target_spec in targets.values() { | |
| collect_optional(target_spec.get("dependencies")); | |
| } | |
| } | |
| Some(names) | |
| fn declared_feature_names(workspace_root: &Path, krate: &str) -> Option<BTreeSet<String>> { | |
| let manifest_path = workspace_root.join("crates").join(krate).join("Cargo.toml"); | |
| let manifest: toml::Value = toml::from_str(&fs::read_to_string(manifest_path).ok()?).ok()?; | |
| let mut names: BTreeSet<String> = manifest | |
| .get("features") | |
| .and_then(|value| value.as_table()) | |
| .map(|features| features.keys().cloned().collect()) | |
| .unwrap_or_default(); | |
| let mut collect_optional = |deps: Option<&toml::Value>| { | |
| let Some(table) = deps.and_then(|d| d.as_table()) else { | |
| return; | |
| }; | |
| for (name, spec) in table { | |
| if spec.get("optional").and_then(|o| o.as_bool()) == Some(true) { | |
| names.insert(name.clone()); | |
| } | |
| } | |
| }; | |
| collect_optional(manifest.get("dependencies")); | |
| if let Some(targets) = manifest.get("target").and_then(|t| t.as_table()) { | |
| for target_spec in targets.values() { | |
| collect_optional(target_spec.get("dependencies")); | |
| } | |
| } | |
| Some(names) | |
| } |
🤖 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/src/commands/compile/optimized_libs/freshness.rs` around lines
201 - 226, Update declared_feature_names to initialize names as an empty
BTreeSet when the manifest lacks a [features] table, then continue collecting
optional dependencies from regular and target-specific dependency tables.
Preserve returning None only for unreadable or invalid manifests, and add a
regression test covering a manifest that declares features solely through
optional dependencies.
| [bindings.node-forge] | ||
| crate = "perry-ext-node-forge" | ||
| lib = "perry_ext_node_forge" | ||
| tracking = "#466" | ||
|
|
||
| # Upstream provenance pin (see PR #7031 / docs upstream-pins.md). | ||
| [bindings.node-forge.upstream] | ||
| version = "1.4.0" | ||
| sha256 = "bf9d7ca0d774235354697bd4b5e642af6505e7ce2066762c3b855138cf870820" | ||
| repo = "https://github.com/digitalbazaar/forge" | ||
| ref = "fa385f92440879601240020f158bed68e444e83a" | ||
| ported-at = "1.4.0" | ||
| date = "2026-07-30" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate TOML table headers.
The repeated bindings.node-forge and bindings.node-forge.upstream declarations make this TOML invalid. Keep one header for each table.
🤖 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/well_known_bindings.toml` around lines 303 - 315, Remove the
duplicate [bindings.node-forge] and [bindings.node-forge.upstream] table headers
in the node-forge binding configuration, retaining a single header for each
table while preserving all associated fields.
| const channel = /^channel\s*=\s*"([^"]+)"/m.exec(toolchainToml)?.[1] | ||
| if (channel && !dockerBody.includes(`toolchain install ${channel} `)) { | ||
| out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Channel check still uses the brittle substring match the msrv parse abandoned.
includes(\toolchain install ${channel} `)requires a trailing space, so a Dockerfile line ending inrustup toolchain install nightly-2026-07-04(or one with the channel as a second argument, exactly the case Lines 229-234 fixed) false-fails. ReuseinstalledToolchains`.
🐛 Proposed fix
- if (channel && !dockerBody.includes(`toolchain install ${channel} `)) {
+ if (channel && !installedToolchains.includes(channel)) {
out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`)
}Note installedToolchains currently filters to args starting with a digit; drop or widen that filter so nightly-*/stable channels survive.
🤖 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 `@scripts/soak/external-tools.mts` around lines 268 - 271, Update the channel
validation near the `channel` extraction to reuse `installedToolchains` instead
of the brittle `dockerBody.includes` substring check. Adjust the
`installedToolchains` parsing/filtering so named channels such as `nightly-*`
and `stable`, including channels appearing as later arguments, are retained and
correctly recognized.
| # Enterprise sfw defaults to BLOCK for non-registry hosts | ||
| # (SFW_UNKNOWN_HOST_ACTION, parsed by the enterprise config), which | ||
| # breaks ordinary dev flows the day a Socket key lands. Only the | ||
| # enterprise build reads the var — it is inert for the free tier — so | ||
| # setting it unconditionally is safe. | ||
| export SFW_UNKNOWN_HOST_ACTION=ignore | ||
| exec sfw '${cmd}' "$@" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unconditionally forcing SFW_UNKNOWN_HOST_ACTION=ignore disarms the enterprise firewall for non-registry hosts.
Every shimmed package manager invocation exports this, so the day a SOCKET_SECURITY_KEY lands the enterprise build's block-by-default posture is silently downgraded to allow — including in CI, which is the environment that most benefits from it. Make it an opt-in default the caller can override rather than a hard export.
🛡️ Proposed fix
-export SFW_UNKNOWN_HOST_ACTION=ignore
+# Dev ergonomics default only; a caller (or CI) can pin the stricter value.
+export SFW_UNKNOWN_HOST_ACTION="\${SFW_UNKNOWN_HOST_ACTION:-ignore}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Enterprise sfw defaults to BLOCK for non-registry hosts | |
| # (SFW_UNKNOWN_HOST_ACTION, parsed by the enterprise config), which | |
| # breaks ordinary dev flows the day a Socket key lands. Only the | |
| # enterprise build reads the var — it is inert for the free tier — so | |
| # setting it unconditionally is safe. | |
| export SFW_UNKNOWN_HOST_ACTION=ignore | |
| exec sfw '${cmd}' "$@" | |
| # Enterprise sfw defaults to BLOCK for non-registry hosts | |
| # (SFW_UNKNOWN_HOST_ACTION, parsed by the enterprise config), which | |
| # breaks ordinary dev flows the day a Socket key lands. Only the | |
| # enterprise build reads the var — it is inert for the free tier — so | |
| # setting it unconditionally is safe. | |
| # Dev ergonomics default only; a caller (or CI) can pin the stricter value. | |
| export SFW_UNKNOWN_HOST_ACTION="${SFW_UNKNOWN_HOST_ACTION:-ignore}" | |
| exec sfw '${cmd}' "$@" |
🤖 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 `@scripts/soak/external-tools.mts` around lines 612 - 618, Update the
environment setup immediately before exec sfw in the shimmed package-manager
flow so SFW_UNKNOWN_HOST_ACTION is only assigned to ignore when the caller has
not already provided a value. Preserve an explicit caller-provided action,
including block, and avoid unconditionally exporting ignore for enterprise or CI
invocations.
| export function parseExcludeEntries(body: string): ExcludeEntry[] { | ||
| const lines = body.split('\n') | ||
| const out: ExcludeEntry[] = [] | ||
| let inBlock = false | ||
| let blockIndent = 0 | ||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]! | ||
| // Tolerate a trailing comment on the key line — without it, a stray | ||
| // `minimumReleaseAgeExclude: # note` never opened the block and every | ||
| // entry beneath silently escaped validation. | ||
| if (/^minimumReleaseAgeExclude:\s*(?:#.*)?$/.test(line)) { | ||
| inBlock = true | ||
| blockIndent = -1 | ||
| continue | ||
| } | ||
| if (!inBlock) { | ||
| continue | ||
| } | ||
| const item = /^(\s+)-\s*['"]?([^'"#\s]+)['"]?\s*(?:#.*)?$/.exec(line) | ||
| if (!item) { | ||
| // Comments stay inside the block; anything else at column 0 ends it. | ||
| if (/^\S/.test(line)) { | ||
| inBlock = false | ||
| } | ||
| continue | ||
| } | ||
| if (blockIndent === -1) { | ||
| blockIndent = item[1]!.length | ||
| } | ||
| if (item[1]!.length !== blockIndent) { | ||
| continue | ||
| } | ||
| const prev = lines[i - 1]?.trim() ?? '' | ||
| const ann = ANNOTATION_RE.exec(prev) | ||
| out.push({ | ||
| name: item[2]!, | ||
| line: i + 1, | ||
| ...(ann ? { annotation: { published: ann[1]!, removable: ann[2]! } } : {}), | ||
| }) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Mis-indented/zero-indent excludes silently bypass the annotation gate.
Two related gaps in parseExcludeEntries:
- The item regex
(\s+)-requires at least one leading whitespace char, so a zero-indent YAML list (minimumReleaseAgeExclude:\n- 'x@1.0.0', valid YAML) is never matched — the block-ending check (/^\S/.test(line)) then treats that first item line as ending the block, and it's never validated. - Once
blockIndentis set from the first matched item, subsequent items with a different indent arecontinued (line 288) — not added toout, not treated as block-end. They silently disappear from validation instead of surfacing a finding.
Since checkExcludeAnnotations/staleExcludes only ever see out, a version-pinned, unannotated exclude entry formatted this way would silently pass npm run soak instead of being rejected — undermining the exact bypass-prevention this gate exists for.
🛡️ Suggested fix: surface indentation drift as a finding instead of silently dropping entries
const item = /^(\s+)-\s*['"]?([^'"#\s]+)['"]?\s*(?:#.*)?$/.exec(line)
if (!item) {
// Comments stay inside the block; anything else at column 0 ends it.
if (/^\S/.test(line)) {
inBlock = false
}
continue
}
if (blockIndent === -1) {
blockIndent = item[1]!.length
}
if (item[1]!.length !== blockIndent) {
+ // Don't silently drop a real item — an indent-drifted entry can
+ // still be a version-pinned bypass that needs annotation checking.
+ out.push({ name: item[2]!, line: i + 1 })
continue
}🤖 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 `@scripts/soak/soak.mts` around lines 258 - 299, Update parseExcludeEntries to
recognize valid zero-indent list items under minimumReleaseAgeExclude and ensure
any item whose indentation differs from the established blockIndent is still
emitted as a finding rather than silently skipped. Preserve the existing block
termination behavior for non-item top-level content, and retain each entry’s
name, line, and annotation metadata so checkExcludeAnnotations/staleExcludes can
validate it.
…S-MITM CA
Add `perry-ext-node-forge`, a native wrapper for the exact node-forge PKI
subset sfw's TLS-MITM CA uses, so apps stop AOT-compiling forge's pure-JS
bignum/ASN.1/crypto code:
- forge.pki.rsa.generateKeyPair({ bits }) -> native RSA keygen
- forge.pki.createCertificate() builder: settable publicKey / serialNumber
/ validity.notBefore|notAfter, setSubject / setIssuer / setExtensions
(basicConstraints, keyUsage, extKeyUsage, subjectAltName,
subjectKeyIdentifier), sign(privateKey, md.sha256)
- forge.pki.certificateFromPem / certificateToPem
- forge.pki.privateKeyFromPem / privateKeyToPem / publicKeyToPem
- forge.md.sha256.create()
Crypto core is pure Rust on RustCrypto crates already in the lockfile
(rsa, x509-cert, der, spki, pkcs1/8, sha2, const-oid, pem, time) - no
rcgen, no new heavy deps. Certs are signed sha256WithRSAEncryption; private
keys emit PKCS#1 "RSA PRIVATE KEY", public keys SPKI "PUBLIC KEY", matching
forge. The cert builder is a real perry JS object so sfw's plain field
assignments stay native; methods serialize it via JSON.stringify and sign
through the RustCrypto core. DN attribute order is preserved on the
parse<->build round-trip so a leaf's issuer DN matches the CA subject DN
byte-for-byte (required by `openssl verify`).
Wiring: well_known_bindings.toml row (js_node_forge_* / perry_ext_node_forge,
date-fns/lru-cache hyphenated convention), NATIVE_MODULES entry, codegen
NATIVE_MODULE_TABLE rows + matching API_MANIFEST entries, and the HIR
createCertificate->Certificate factory registration. CPU-only: not in
binding_needs_shared_tokio.
Acceptance: crate unit tests (PEM round-trips, builder JSON parsing, DN
round-trip) plus an openssl end-to-end test that builds a CA + leaf and
runs `openssl verify -CAfile ca.pem leaf.pem` (=> OK) and `openssl x509
-text` (SANs, EKU serverAuth, sha256WithRSAEncryption all present).
…ork end-to-end
The perry-ext-node-forge crypto core (added earlier this branch) was
inert from compiled TypeScript: forge's API is deeply nested
(forge.pki.rsa.generateKeyPair, forge.pki.createCertificate,
forge.md.sha256.create), so the call receiver is a Member CHAIN, not a
bare native-module Ident — no dispatch arm fired, and an intermediate
read like forge.pki hit the unimplemented-API gate (no node-forge
symbol 'pki') and deferred a throw.
Adds try_node_forge_namespace in perry-hir's call lowering: collapse any
member chain rooted at the node-forge default import to its last segment
(the method key the codegen NATIVE_MODULE_TABLE rows already use). Runs
before the generic namespace / module.Class.staticMethod dispatch, which
would otherwise claim the 2-level forge.pki.createCertificate() shape and
gate its forge.pki object-read.
Also fixes a wrapper fidelity gap: setSubject/setIssuer now store
node-forge's { attributes: [...] } DN shape (matching certificateFromPem),
so leaf.setIssuer(caCert.subject.attributes) — the sfw idiom — reads the
CA subject back and the leaf's issuer DN matches the CA. certificateToPem
accepts both the wrapped and bare-array shapes.
End-to-end: a CA+leaf generated by the COMPILED binary from real
forge.pki.* TypeScript (fixture with no node-forge installed, served by
the well-known table) now verifies with the openssl CLI
(leaf.pem: OK; issuer=CN=Socket Security CA), correct SANs / keyUsage /
extKeyUsage / sha256WithRSAEncryption. Adds an upstream provenance pin
(lock-step per PerryTS#7031) and 3 HIR flattening tests.
perry bin 753 pass; node-forge crate 6+1 (incl. openssl e2e); new HIR
tests 3/3. (3 pre-existing write-PIC codegen failures on this worktree
are unrelated — they fail identically on branches with none of this
work.)
567177b to
ba7bff8
Compare
# Conflicts: # crates/perry/well_known_bindings.toml
Serves node-forge's PKI subset (the surface Socket Firewall's TLS-MITM CA uses) from perry-native RustCrypto instead of AOT-compiling node-forge's large pure-JS bignum/x509 code. Verified end-to-end: a CA+leaf generated by the compiled binary from real
forge.pki.*TypeScript passesopenssl verify.Two pieces
1. The wrapper crate
perry-ext-node-forge(RustCrypto:rsa,x509-certbuilder+hazmat for exact extension control,der/spki/sha2/pem— all already inCargo.lock, norcgen). Covered:forge.pki.rsa.generateKeyPair({bits})forge.pki.createCertificate()+publicKey/serialNumber/validitycert.setSubject/setIssuer/setExtensionscert.sign(key, md.sha256)certificateFromPem/certificateToPem,privateKeyFromPem/privateKeyToPem(PKCS#1),publicKeyToPemforge.md.sha256.create()2. The HIR namespace-flattening that makes it actually dispatch. This is the load-bearing fix — without it the crate is inert. forge's API is deeply nested (
forge.pki.rsa.generateKeyPair), so the call receiver is aMemberchain, not the bare native-moduleIdentevery existing dispatch arm expects — and an intermediate read likeforge.pkiotherwise hit the unimplemented-API gate (nonode-forgesymbol namedpki) and deferred a throw. Newtry_node_forge_namespacecollapses any member chain rooted at the node-forge default import to its last segment (the method key the codegen table already uses), and runs before the genericmodule.Class.staticMethoddispatch so it wins the 2-levelforge.pki.createCertificate()shape.Fidelity fix caught by the openssl chain-verify
setSubject/setIssuernow store node-forge's{ attributes: [...] }DN shape (matchingcertificateFromPem), soleaf.setIssuer(caCert.subject.attributes)— the sfw idiom — reads the CA subject back and the leaf's issuer DN matches the CA byte-for-byte. Before this, the leaf issuer was empty and the chain didn't verify.openssl acceptance (from the compiled binary)
Wiring
New
crates/perry-ext-node-forge; codegen rows innative_table/utils_crypto.rs(instance methods gatedclass_filter: "Certificate";createCertificate → Certificatefactory typing injs_transform/local_natives.rs);node-forgein NATIVE_MODULES + manifest;[bindings.node-forge]with an upstream pin (lock-step per #7031). CPU-only (no shared-tokio).Validation
cargo test -p perry --bin perry: 753 passed; crate: 6 unit + 1 openssl e2e; 3 new HIR flattening tests (node_forge_namespace_lowering.rs).Should rebase on #7031 at merge (shared
well_known_bindings.toml/ NATIVE_MODULES). Thetry_node_forge_namespaceflattening generalizes to any deeply-namespaced native package if more are added later.Summary by CodeRabbit
New Features
Bug Fixes
RegExpcalls withoutnew.DOMExceptionsubclass behavior.Security