Skip to content

Drop comments that restate the code - #5

Merged
kridaydave merged 1 commit into
mainfrom
k5/audit-taproot-comment-walls
Oct 5, 2026
Merged

kridaydave merged 1 commit into
mainfrom
k5/audit-taproot-comment-walls

Conversation

@kridaydave

Copy link
Copy Markdown
Contributor

What changed

Comment-only. Not one line of code moved, so no behavior can change.

file deleted why
src/engine.rs 4 inline comment lines Each restated the serde_json round-trip on the two lines under it. The doc comment on to_canonical_json stays, trimmed to the one fact a reader cannot get from the code: the hash covers this form, so key order has to be stable.
src/keys.rs 1 line add_to_index carried a note to self about deactivating keys on rotation, ending in a question mark. The code pushes, and rotate two hundred lines down is where deactivation actually happens.
src/mount.rs 4 lines A Public mount helper section banner with no helper under it. mount_readonly sits directly above.
src/cli.rs 1 line, 1 rule added // Handlers had lost its opening --- rule and sat glued to the previous function. // Validate has_breaking helper stays consistent narrates the debug_assert! on the next line.
 src/cli.rs    | 3 ++-
 src/engine.rs | 8 ++------
 src/keys.rs   | 1 -
 src/mount.rs  | 4 ----
 4 files changed, 4 insertions(+), 12 deletions(-)

Gate

cargo fmt --check && cargo clippy -- -D warnings && cargo test --locked && cargo build --locked

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 9.84s
test result: ok. 76 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.31s
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
test result: ok. 30 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.23s
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.53s

106 tests, same counts as origin/main.

Not shipped

Behavior changes from the same review run. Each needs a decision, so none are in this PR.

1. Policy fields are stored and never enforced. src/server.rs:71

Policy carries allowed_branches and blocked_env_keys. push_state reads policy.require_signed at src/server.rs:75 and nothing else. Grep for both fields across src/ returns only the struct, the two setters, and the round-trip test.

Reproduced against a live server with a policy of allowed_branches: ["main"]:

$ curl -X POST .../v1/policy/myapp -d '{"allowed_branches":["main"],"blocked_env_keys":["SECRET"]}'
{"repo":"myapp","require_signed":true,...,"allowed_branches":["main"],"blocked_env_keys":["SECRET"]}

$ curl -X POST .../v1/states --data-binary @evil-branch-state.json
{"hash":"e893ca09a5c15..."}
HTTP 200

$ ls reg/refs/myapp/
evil-branch

A state on a forbidden branch is accepted and a ref is written for it. Same path for blocked_env_keys: the sync loop adopts a state carrying SECRET=sk_live_... into a signed state file with no check against policy.

Minimal fix, both in push_state next to the existing require_signed check: reject when allowed_branches is non-empty and does not contain signed.state.base.branch, and reject when any key of signed.state.env_vars is in blocked_env_keys. Four lines each. Decide first whether a blocked key should reject the push or be stripped from the state.

2. sync --dry-run deletes the drift file. src/cli.rs:1211

The no-drift branch runs before the args.dry_run check at line 1241:

$ taproot sync --state-path s.json --from d.json --dry-run
no drift — states are identical
removed:    /tmp/drr/d.json
$ ls /tmp/drr/d.json
ls: cannot access '/tmp/drr/d.json': No such file or directory

--dry-run promises nothing is adopted, and it deletes a file. tests/cli.rs:489 covers dry-run with real drift and never reaches this branch, so the suite is green while the file is gone.

Minimal fix: hoist the if args.dry_run return above the diffs.is_empty() branch, or guard the remove_file with !args.keep && !args.dry_run. One line.

3. README shows output the binary does not produce. README.md:42

The sample taproot mount block prints materialized: 2.4 GB (lazy), a [s]ync · [f]ork · [d]etach prompt, and a container list. print_mount_header in src/cli.rs:525 prints none of those. Measured against the built binary:

$ taproot mount /tmp/mt/tree --no-fuse --state-path /tmp/mt/s.json --out /tmp/mt/tree
TAPROOT MOUNT
repo:       myapp
base:       main@9f3a2c1
state:      signed · sha256:9165613e5235
runtimes:   0
containers: 0
env-vars:   0
(no-fuse — wrote tree to /tmp/mt/tree)
env:        /tmp/mt/tree/env (writable — edit, then run `taproot sync --from-dir`)

The stats are also wrong regardless of the CLI. The state pins zero bytes of container, so "2.4 GB (lazy)" describes nothing the code has. Trimming the block to the real output needs no code change.

Comment-only. No code line changes.

- engine.rs: four inline comments restating the serde_json round-trip
  that the two lines below them already say. One doc line kept because
  it records why the order matters (the hash covers this form).
- keys.rs add_to_index: a trailing question to yourself about rotation.
  The next line is keys.push, and the code below it pushes.
- mount.rs: an empty 'Public mount helper' banner above the FUSE impl
  with no helper under it.
- cli.rs: '// Handlers' had lost its opening rule; 'Validate has_breaking
  helper stays consistent' narrates the debug_assert on the next line.

4 insertions, 12 deletions.
@kridaydave
kridaydave merged commit 3031a9d into main Oct 5, 2026
2 checks passed
@kridaydave
kridaydave deleted the k5/audit-taproot-comment-walls branch October 5, 2026 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant