Repository navigation
fix(security): bind trusted execution to unambiguous snapshots (12106) - #211
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Snapshot encoding and trust keys lib/hooks.sh, lib/commands/init.sh, tests/hooks.bats, docs/configuration.md |
Configuration definitions use NUL-delimited records encoded as hexadecimal text. Trust hashing uses the snapshot, trust keys use version 3, and repository identity uses filesystem birth-time metadata. Tests cover hashing, cross-shell helpers, and birth-time precision. Documentation describes the new format and renewed approval requirement. |
CLI hook and default consumers lib/hooks.sh, lib/commands/trust.sh, lib/config.sh, tests/hooks.bats |
Hook selection, executable-default lookup, and trust output use definitions from the captured snapshot. Tests cover serialization collisions, configuration replacement during trust checks, repeated values, and boolean defaults. |
Generated post-cd hooks lib/commands/init.sh, tests/init.bats |
Generated Bash, Zsh, and Fish post-cd hooks retrieve hook definitions and verify trust using the same snapshot. Tests replace configuration during the read and check which hook runs. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: High
Sequence Diagram(s)
sequenceDiagram
participant PostCd as Bash/Zsh/Fish postCd
participant Git as Git config
participant Snapshot as Snapshot helpers
participant Trust as Trust verification
participant Hook as Hook command
PostCd->>Git: Read .gtrconfig with git config -z
Git-->>PostCd: Return NUL-delimited records
PostCd->>Snapshot: Encode records and select hooks.postCd
PostCd->>Trust: Verify captured definitions
Trust-->>PostCd: Return trust status
PostCd->>Hook: Execute selected hook when trusted
Suggested reviewers: helizaga
Merge Risk: 
a2ad2
-
The snapshot and trust-key changes have no established merge-blocking issue. They are ready for normal merge checks.
Security architecture risk: 
a2ad2
-
The snapshot design strengthens command authorization, but the upgrade does not reliably replace previously cached shell integrations. Existing cached helpers can continue honoring older approvals even though the updated CLI requires renewed approval. Freshly generated integrations use the stronger controls.
Retained concerns
- Medium · security · observed: The new v3 authorization contract does not reach existing shell caches with a matching stamp. Cache schema 7 and application version 2.11.1 are unchanged, so initialization can return pre-PR helpers instead of generating snapshot-bound helpers. Those helpers still use v2 approvals and separate configuration reads, while the updated CLI rejects older approvals. This creates inconsistent authorization during upgrade; the legacy vulnerabilities themselves are pre-existing. Fresh generation and a changed cache stamp avoid the issue.
Security review details
-
Security Blast Radius
- inferred — A repository contributor can influence committed executable definitions, but execution requires approval or a defect in that authorization boundary. Commands run through eval with the invoking user's authority rather than repository-confined authority. Consequently, potential effects extend to files, credentials, and services accessible to that process, despite approvals being scoped to a repository identity.
Security Findings and Attack Paths
- inferred — For an upgrade retaining a matching pre-PR cache and a legacy approval marker, postCd execution can continue using the old v2 authorization path while the CLI enforces v3. Legacy serialization or replacement weaknesses therefore remain reachable in that upgrade state. This is incomplete migration of the stronger control, not a newly introduced command-execution primitive.
Trust Boundaries and Controls
- observed — The updated repository-file consumers authorize the same snapshot they select from. Marker validation binds the snapshot hash, canonical repository root, Git-directory identity, and canonical configuration path. Git-config hooks remain a separate, explicitly user-controlled trust source; an empty captured snapshot supplies no repository-file command.
Resilience and Maintainability Implications
- observed — Regression assertions cover ambiguous multiline serialization, configuration replacement during selection or verification, and cross-shell snapshot parity. Generated-shell replacement assertions exercise newly generated helpers, so they do not establish migration safety for existing matching caches.
Hardening Proposals
- proposed — Couple generated-shell cache invalidation to changes in the authorization protocol, and validate upgrade behavior starting from the base revision's schema-7 cache and legacy markers. Include reloading already-installed shell functions in the upgrade procedure.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main security change: binding trusted execution to unambiguous configuration snapshots. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 19 functions across 6 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks each record twice,
NUL marks keep the lines precise.
One snapshot guides the trusted call,
Three shells share the same install.
Fresh approvals greet them all.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
A multiline executable default could serialize identically to separate default and hook records, letting a changed
.gtrconfigreuse approval. Hook/default consumers also read the mutable file separately from their trust check, so replacement could authorize commands from a different snapshot.Capture Git's NUL-delimited definition records once, preserve them as hexadecimal text in shell variables, and select executed values from that same approved snapshot. Apply the encoding and snapshot flow to CLI hooks, executable defaults, trust confirmation, and generated Bash/Zsh/Fish postCd helpers. Bump trust keys to v3 and the shell cache schema; users must review commands again with
git gtr trust. Preserve last-value and boolean semantics for executable defaults. Keep GNU birth-time nanoseconds in repository identity so a rapidly recreated Git directory cannot reuse approval through inode reuse in the same second.Validation: targeted hook/trust/init BATS tests, including actual Bash/Zsh/Fish execution, serialization collisions, trailing value newlines, and deterministic config replacement during execution. The full local BATS suite, ShellCheck, completion checks, and Linux CI pass; additional identity tests verify GNU nanosecond precision.
Refs issue 12106. Addresses 4025d805, ad4f2b79, 0c881ab3, 10d19962, bb68864e, and e068fd9f.