Repository navigation
fix(checker): a moved secret's aliases are moved too - #642
Conversation
Work in progress: the sole-reference rule for secret(v), its negative cases, the reverted-rule pin, the docs and the record.
Work in progress: simplify the sole-reference rule, refuse a second name made through `??`, regenerate the goldens.
Work in progress: judge each secret(v) site in a function of its own, so the loop over them keeps no allocation past a pass.
# Conflicts: # docs/security/README.md # src/codes.ts # tests/self/goldens/checked-self.txt
# Conflicts: # src/codes.ts # src/secret.ts # tests/self/goldens/checked-self.txt
# Conflicts: # tests/self/goldens/checked-self.txt
amritk
left a comment
There was a problem hiding this comment.
Review round 1 at db5a03e. Three independent reviewers read the diff, and a separate verifier then compiled a probe or read the code to confirm each finding below. Seven findings were confirmed. Eleven others were refuted and won't be raised again.
Blocking
Ok(raw)/Err(raw)leave a second reference that the rule doesn't see. See the inline comment onsrc/secret.ts:1612.
Low (fix in the same push)
makeKey().orReturn()is now falsely refused, contradicting "every program that passed before still passes". Seesrc/secret.ts:1736.- The copy hint names an undefined
vfor an argument that isn't a plain name. Seesrc/secret.ts:1825. - No negative pins a statement with several declarators, though both shapes are refused today. CLAUDE.md asks for one per shape:
const a = raw, b = 1, andconst k = secret(raw), alias = raw. - The record cites a check string that doesn't exist. See
docs/security/secret.md:71. - The record claims a recursion property with no test pinning it. See
docs/security/secret.md:52. docs/AI.md:1512still says only "secret(local)moves the local too". It is the whole-language rules page and should state the new sole-reference condition. This file is added to the stage's scope for this fix.
Please address all seven in one push. Merge main again first if it has moved, and re-run the full gate, reading each exit status.
Generated by Claude Code
Review round 1. A `Result` now carries its value as the rule sees it: `Ok(raw)` and `Err(raw)` leave a second name, as a ternary arm does, and `r.orReturn()`, `r.expect(m)` and `r.unwrapOr(d)` answer what `r` carries in its `Ok`. So `const r = Ok(raw); secret(raw)` is refused, and `secret(make().orReturn())` is accepted when `make` returns `Ok` of a fresh array, as the last release accepted it. A local carried to a `secret(...)` through `unwrapOr`'s default is refused, since the move does not reach it. The identifier builtins that keep their argument are named: `Ok` and `Err`. The copy the diagnostic suggests no longer names a `v` that does not exist. Refs: docs/LANGUAGE.md#secrets-nishsecret, docs/security/secret.md Tests: tests/cases/reject_secret_alias_ok, reject_secret_alias_err, reject_secret_alias_unwrap_or, reject_secret_alias_declarators, reject_secret_alias_declarator_moved, reject_secret_alias_mutual
amritk
left a comment
There was a problem hiding this comment.
Review round 2 at d15f5a8. Three fresh reviewers read the diff, and each finding was then verified with probes compiled against this head.
No soundness hole was found: every route probed (exposeWith, expose, orReturn/expect through a helper or a local, Err through a callee, methods returning or keeping this, generics, constructors, and receivers with no recorded type) is refused. All round-1 findings are fixed. Four findings were confirmed, all low or medium and none blocking:
- [medium] The PR body describes an older head. Its first section becomes the squash commit and the changelog entry. Please update the following:
- The Testing section names head
eeada0b, with counts taken at that head. Re-run the gate on the final head and report those numbers. - "What changed" says the revert check "compiles all 17 cases". It now compiles 22 of 23;
declarator_movedis exempt. - The Negatives table lists 14 rows. It omits
declarators,declarator_moved,mutual,ok,errandunwrap_or. - The closing note says
docs/AI.md"is left for its owner", but this PR now updates it, and the Docs list omits it. - The changelog paragraph before
---doesn't mention thatOk/Errwrapping andorReturn/expect/unwrapOrare followed. A release-note reader should learn thatOk(raw)counts as a second reference.
- The Testing section names head
- [low] The
Result-unwrap routes have no negative. Seesrc/secret.ts:1574. - [low] The record claims two over-refusals that no test pins. See
docs/security/secret.md:61. - [low]
reject_secret_alias_popdoesn't depend on the alias it sets up. See its line 7.
Please address all four in one push, and re-run the full gate on the final head, reading each exit status.
Generated by Claude Code
…e's over-refusals Review round 2. `secret(wrap(raw).orReturn())`, `secret(wrap(raw).expect(m))` and the same through a local `Result` holding `raw` each get a negative. So do the two places the rule refuses a safe program: a `Result` local copied into another, and a `let` given another local after the move. `reject_secret_alias_pop` now pins what it tests, that a method of a value never answers a fresh value, without an alias it did not need. Tests: tests/cases/reject_secret_alias_unwrap_helper, reject_secret_alias_unwrap_local, reject_secret_alias_result_copy, reject_secret_alias_reassigned_after, reject_secret_alias_pop
amritk
left a comment
There was a problem hiding this comment.
Review round 3 at 0fb115c. Fresh reviewers read the code shards, and a verifier then compiled and ran probes against this head.
Blocking:
- An assignment used as a value (
alias = raw = [...],(raw = [...])as an initialiser, an argument or a field value) leaves a second reference. The checker accepts it, and the second name reads the wiped key. Seesrc/secret.ts:1516.
Everything else in the code and test shards held. All 27 negatives exercise their own routes, and the mutation pin's case list and exemption are sound.
The PR body is still the one from round 2 finding 1. Please rewrite it on the final head after this fix, with the gate's exact numbers, and read it back. Round 4 runs on that head, docs and PR body included.
Generated by Claude Code
…array it stores Review round 3. An assignment answers the array it stores, so `alias = raw = [1, 2, 3]`, `const alias = (raw = [...])`, `xs.push(raw = [...])`, `h.k = (raw = [...])` and `keep(h, raw = [...])` each gave the moved array a second name the rule did not see. An assignment to the local is now harmless only where its value is dropped, by a statement or a `for` header; anywhere else its value is judged as the local itself is. The compound assignments take no array, so they are no road. Tests: tests/cases/reject_secret_alias_assign_chain, reject_secret_alias_assign_init, reject_secret_alias_assign_push, reject_secret_alias_assign_field, reject_secret_alias_assign_kept, reject_secret_alias_assign_helper
amritk
left a comment
There was a problem hiding this comment.
Review round 4 at 8b29eb3. Fresh reviewers read the code shards, and a verifier compiled probes at this head.
The src/secret.ts shard found no soundness hole. It traced assignment-as-value in every parent, the for-header exception, all Result wrappers, array methods, interface dispatch and the crash paths. The round 3 fix holds: all seven chained-assignment probes are refused. CI is green, 19/19.
Two low coverage findings were confirmed; every probe was refused, and nothing is pinned:
- Six more parents of an assignment used as a value: array element, record field, ternary arm,
??arm,Ok(...), kept constructor argument. Seesrc/secret.ts:1528. - An assignment as a local's source (
raw = (alias = [...])), the only way to reach thejudgedefault branch. Seesrc/secret.ts:1787.
Please add these negatives and put them in the pin. In the same push, rewrite the PR body (round 2 finding 1), including the gate's exact numbers on that head. Round 5 reviews all three shards, docs and body included, on that head. If it comes back clean, this merges.
Generated by Claude Code
… an assignment as a local's source Review round 4. An assignment to the local used as an array literal's element, a record literal's field, a ternary arm, a `??` arm, the argument of `Ok` or a kept constructor argument is refused, and so is a local given an assignment as its value (`raw = (alias = [1, 2, 3])`). Each now has a negative in the mutation pin. Tests: tests/cases/reject_secret_alias_assign_parents, reject_secret_alias_assign_constructor, reject_secret_alias_assign_source
# Conflicts: # tests/self/goldens/checked-self.txt
# Conflicts: # src/codes.ts # tests/self/goldens/checked-self.txt
amritk
left a comment
There was a problem hiding this comment.
Review round 5b @ 71d51fa: how the rule interacts with #644 (task captures and spawn forms). One confirmed finding, low severity (coverage).
1. The argument forms of spawn are a way in with no negative of their own. Each of these is refused only by NL2512, through PointerParamFacts.captured on a generic std method of ThreadScope. No case pins either route, and the mutation pin would not notice if that route regressed. The only spawn case, reject_secret_alias_capture_spawn, is the arrow form, which is judged by the "inside an arrow" rule instead.
const first = (b: u8[]): i32 => toI32(b[0]);
// ...
const raw: u8[] = [7, 2, 3];
const out: i32[] = [0];
{
using s = scope();
s.spawn(first, raw, out, 0); // `ThreadScope.spawn<u8[], i32, first>` keeps `arg`
}
const key: Secret<u8[]> = secret(raw); // NL2512 is the only errorThe same holds for the form without a destination: s.spawn(sum, raw) with a pure sum: (b: u8[]) => void reaches ThreadScope.spawnWith<u8[], void, sum>, and NL2512 is again the only error. Per CLAUDE.md, "a checker rule covers every way in", so each route needs its own negative. Add a case for each form (or one case with both, if the .err and the pin can tell them apart), put them in the reject_secret_alias_* set the mutation pin reverts, and mention the route in docs/security/secret.md next to the arrow capture.
Checked and found sound:
s.spawn(first, raw, out, 0)withsecret(raw)inside the open scope: refused by NL2512 and by #644's scope rule.rawas a spawn destination: refused by #644's destination rule.secret(raw)before the spawn: NL2445.- A
secret(...)site inside a task arrow's body, with aconstalias: NL2512. - #644 adds no task handle or join that
FreshJudgecould take as fresh. Rngreturns nou8[].- The renumbering to NL2512 is consistent, and #644's own NL2506 is untouched.
Generated by Claude Code
Review round 5. `s.spawn(entry, raw, dst, at)` and `s.spawn(entry, raw)` hand `raw` to the scope, which keeps it until the task has run, so `secret(raw)` after the block is refused through `ThreadScope.spawn`'s and `spawnWith`'s kept parameter. Each form now has a negative in the mutation pin, and the record names the route. Tests: tests/cases/reject_secret_alias_spawn_arg, reject_secret_alias_spawn_with
secret(v)now takes only a value nothing else holds. Movingvinto aSecretused to forget every other name for the same bytes:const alias = raw; const key = secret(raw)compiled,aliasread the key outsideexpose, and afterwipe(key)it read zeros. The checker now refusesvwhenever something else still holds it, says where that second reference was made, and suggests a fresh value or a copy:A literal,
null, and anewwhose constructor keeps nothisare fresh. A ternary is fresh when both arms are. A call is fresh when its callee's answer is, judged over the callee's own body by the same rule, through every depth of calls. A local is fresh when it is the only reference to its value: everything assigned to it is fresh and none of it is another local, it is not afor...ofbinding, and no other use of it leaves a second name.These each leave a second name:
??;Ok(raw)orErr(raw), since theResultholds it;alias = raw = [1, 2, 3],xs.push(raw = [...])); an assignment whose value is dropped, as a statement or in aforheader, is fine;r.orReturn(),r.expect(m)andr.unwrapOr(d)answer whatrcarries in itsOk, so a function that returnsOkof a fresh array still givessecreta fresh value through them, as the last release accepted.secret(c ? a : b)now moves the local in either arm, and refuses a parameter there as NL2447 does elsewhere. All ofstd/compiles unchanged.The rule refuses a few safe programs, each with a compile error that names the reference, never with a missed read:
Resultlocal copied into another;letgiven another local's array after the move.Refs: docs/LANGUAGE.md#secrets-nishsecret, docs/security/secret.md
Tests: tests/cases/reject_secret_alias_*
Part of #625.
Approach
Option (b), the ownership rule:
secret(e)acceptseonly when it is the sole reference to its value, and anything else is a compile error that names the second reference. The alternative, an alias-aware move, would have to find every name that may hold the value (fields, elements, callee-kept parameters, captures) and make each one unreadable across functions. A missed name there is a missed read. Here a missed shape is a refusal.What changed
src/secret.ts:checkSoleReferencesjudges eachsecret(...)site, whichcollectFlowcollects, in a body the ownership walk accepted. So NL2447 and NL2445 are still said once, by that walk.FreshJudgedecides whether an expression is fresh.ReferenceScanwalks one body for every occurrence of a local, sorts each use as harmless or as a second name, and resolves a captured name throughsourceLocal(feat(checker): parallelMap, parallelFrom and arrows that read the enclosing function's consts #609), which also covers a task spawned as an arrow (feat(checker): arrow tasks with captures, tasks without a destination, and a seeded Rng #644).ReferenceScan.harmlessAtcounts an assignment to the local as harmless only where its value is dropped, by a statement or aforheader. Anywhere else, the assignment's value is judged as the local is.ReferenceScan.handsOnfollows a value through every wrapper whose value it is: parentheses, a ternary arm,??,Ok/Err, and the receiver or default oforReturn/expect/unwrapOr.Resultmethod that answers the payload is judged by what its receiver carries. At asecret(...), a local carried there is refused, because the move does not reach it.OkandErr.SecretNeed: either a callee's answer, or whether a callee keeps a parameter.SecretFlow.moveandcollectMovedfollow ternary arms.src/compilation.ts:Compilation.checkSecretsanswers the needs withFreshResults.PointerParamFacts.captured, read from the facts.src/program.ts:SecretNeedandCheckedProgram.secretNeeds.src/codes.ts: NL2512.mainholds NL2492–NL2511, which typed JSON, feat(checker): parallelMap, parallelFrom and arrows that read the enclosing function's consts #609 and feat(checker): arrow tasks with captures, tasks without a destination, and a seeded Rng #644 took.tests/run.js:nish:secret: reverting the sole-reference rule lets every reject_secret_alias_* case compile (${aliasCases.length} cases)copiessrc/with the rule taken out (the site walk, the whole-program questions, and the ternary arms of the move) and builds that copy with the seed. That compiler accepts all 38 cases the rule alone refuses.reject_secret_alias_declarator_movedis exempt by name, because the move refuses it itself (NL2445). The check finds its three edit lines by exact text.nish:secret: secret(v) still takes ${what}checks that a recursive function returning a literal, a fresh payload throughOk/orReturn/expect, and the suggested copy are all accepted.nish:secret: every std/ caller of secret(v) compiles (${callers.length} modules)checkstls,x509-random,hkdf,x25519andx509.docs/LANGUAGE.md"Secrets": thesecret(v)row and bullet, with every case named.docs/AI.md"Secrets": the sole-reference condition, with a compiled NL2512 example.docs/guide/using-nish/standard-library.md: one refused example and one accepted copy, both compiled bynode tests/run.js guide.docs/security/secret.md(SECRET-1, Medium, fixed), and its row indocs/security/README.md.tests/self/goldens/checked-self.txtwas regenerated withnode tests/self/goldens.js --updateafter each merge ofmain. No positive case was added, so nothing is registered with the differential goldens.No
std/, runtime or emitter change, and no IR moved: every new case is a refusal, so no.llgolden is added, and every existing.llis unchanged.Negatives
One file per road, each with its own
.errand no absolute path:reject_secret_alias_constconst alias = rawreject_secret_alias_letlet alias = raw, andalias = rawafter the declarationreject_secret_alias_reassignedlet raw = …; raw = otherbefore the movereject_secret_alias_reassigned_afterraw = otherafter the move (a refusal of a safe program, pinned)reject_secret_alias_assign_chain,_assign_init,_assign_push,_assign_field,_assign_kept,_assign_helperalias = raw = [...],const alias = (raw = [...]),rows.push(raw = [...]),h.buf = (raw = [...]),keep(h, raw = [...]), and the same inside a calleereject_secret_alias_assign_parents??arm and anOk(...)argumentreject_secret_alias_assign_constructorreject_secret_alias_assign_sourceraw = (alias = [...]): an assignment as a local's valuereject_secret_alias_declaratorsconst n = 1, alias = raw, andconst alias = raw, key = secret(raw)reject_secret_alias_declarator_movedconst key = secret(raw), alias = raw(NL2445)reject_secret_alias_fieldh.buf = raw,{ buf: raw }reject_secret_alias_element[raw],rows[0] = raw,rows.push(raw)reject_secret_alias_paramreject_secret_alias_returnconst alias = id(raw), andsecret(id(raw))withrawread afterreject_secret_alias_mutualsecret(f(2, raw))wherefanswersg's answer andganswers its parameterreject_secret_alias_ternaryconst alias = c ? raw : other,secret(c ? raw : other)withrawread after (NL2445),secret(c ? p : [1])(NL2447)reject_secret_alias_nullishconst alias = maybe ?? rawreject_secret_alias_ok,reject_secret_alias_errconst r = Ok(raw),const r = Err(raw)reject_secret_alias_unwrap_orsecret(none().unwrapOr(d))withdread afterreject_secret_alias_unwrap_helpersecret(wrap(raw).orReturn())andsecret(wrap(raw).expect(m))reject_secret_alias_unwrap_localsecret(r.orReturn())andsecret(r.expect(m))for a localr = Ok(raw)reject_secret_alias_result_copyResultlocal copied into another before it is returned (a refusal of a safe program, pinned)reject_secret_alias_constructornew Holder(raw)keeping its argument,new Pair(r)handingthisoutreject_secret_alias_receiverthis, a method that answersthis.bufreject_secret_alias_popsecret(rows.pop()): a method of a value never answers a fresh valuereject_secret_alias_for_offor...ofbindingreject_secret_alias_capture_map,_from,_map_into,_reducerawreject_secret_alias_capture_spawnrawreject_secret_alias_spawn_arg,reject_secret_alias_spawn_withrawas a task's argument,s.spawn(first, raw, out, 0)ands.spawn(sum, raw): the scope keeps it until the task has runThe following were probed and found refused:
get(h));Okof a field throughorReturn;Map.set(k, raw);rows[0]as an initialiser;h.l = thisin a constructor;The compound assignments (
+=,??=,||=) take no array, so they are no road.Accepted unchanged:
secret_flow,secret_wipe,secret_wipe_o2andsecret_wipe_o2_plain;std/caller (tls.ts,x509-random.ts,hkdf.ts,x25519.ts,x509.ts);pushandindexOfbefore the move;crypto.getRandomValues(raw), and a helper that writes through its parameter;make().orReturn()andmake().expect(m)for a freshOkpayload;exposewith a fresh arrow result;letgiven a fresh value after the move;raw = [...]as a statement, or in aforheader, before the move.Two compilers were compared over every corpus program (
tests/cases,tests/linkand its directory programs,examples,docs/cookbook,tests/differential/corpus,bench,std): this branch, and the same tree with the rule reverted. They differ only on the new cases.Testing
Head
32cee13, withmainata22a056merged in (including #640, #641 and #644). Each command was run bare, with its exit status read, afterbash scripts/fetch-seed.sh --force:npm run check: exit 0.npm run lint: exit 0.npm test, LLVM 18 onPATH, noDEGRADED:line: 18386 passed, 0 failed, 15 skipped.npm testskips without a WASI sysroot and the WASI run below covers; fix(checker): refuse an exported function that takes a C symbol, and spell private ones apart #640'sc-symbolswasm32 check, skipped for the same reason and covered by thec-symbolsrun below; and the aarch64 static link (crt1.o), which this container cannot do.WASI_SYSROOT=/usr node tests/run.js wasi, with wasi-libc andlibclang-rt-18-dev-wasm32installed: 49 passed, 0 failed, 0 skipped. That includesweb: src/ links into one wasi module.node tests/run.js performance: 211 passed, 0 failed. The ratchet holds, with no new NL9007 or NL9011.node tests/run.js selfhost: 187 passed, 0 failed.WASI_SYSROOT=/usr node tests/run.js c-symbols: 39 passed, 0 failed, 0 skipped.node scripts/gen-diagnostic-codes.mjs --check: exit 0.node docs/check-links.mjs: ok.tests/run.js.mainhas since gained #659, which is test-only (a new case, its goldens andtests/run.js), merges cleanly, and touches nothing in this diff.