Repository navigation
feat: audit command, permissions and token fingerprint - #19
Conversation
Record the catalogued command, its permissions, the provider duration and the action token fingerprint on allowed action events; the raw token never enters the record.
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Add fingerprints to action results packages/cli/src/types.ts, packages/host/src/providers/github-app.ts, test/unit/github-app.test.ts |
ActionRunResult adds an optional token fingerprint. The GitHub App provider computes the fingerprint from the minted token and returns it with the action result. The test checks the returned fingerprint. |
Record successful action details packages/host/src/broker.ts, test/unit/broker.test.ts, ROADMAP.md |
The broker adds the action command, permissions, elapsed time, and token fingerprint to successful action audit records. The tests check these fields. The roadmap marks the audit checklist item complete. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant GitHubAppProvider
participant runGhAction
participant Broker
participant ActionAudit
GitHubAppProvider->>runGhAction: Execute action
runGhAction-->>GitHubAppProvider: Return action result
GitHubAppProvider->>Broker: Return result with token fingerprint
Broker->>ActionAudit: Record command, permissions, duration, and fingerprint
Possibly related PRs
- code-vaults/git-credential-broker#14: Adds the broker action operation and success audit event that this change extends.
- code-vaults/git-credential-broker#12: Adds the action lookup and command and permission fields used in the audit record.
- code-vaults/git-credential-broker#10: Adds the
tokenFingerprintutility imported by the broker and GitHub App provider.
Suggested reviewers: i1stg
Merge Risk: 
Minimal · up to 4a82c
-
No actionable merge-blocking risk is established by this change.
Pre-merge checks: 
5
-
Full details: Title check
-
Explanation
The title "feat: audit command, permissions and token fingerprint" matches the main change. The PR adds the catalogued command, its permissions, provider duration, and a token fingerprint to allowed-action audit records. The title names the three audit fields the changes add. It does not mention the duration field or the ROADMAP update, which is expected for a short title.
-
Finishing Touches
-
📝 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
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 @coderabbitai help to get the list of available commands.
i1stG
left a comment
There was a problem hiding this comment.
Approved as @i1stG.
Verified at 0da394b:
corepack yarn check: 283/283, and CI is green on Linux, WSL, Windows and macOS.- The audit record now covers the ROADMAP item: action, repo, command, requested permissions, duration, exit status, truncation/timeout and token fingerprint. The fingerprint is the existing 12-hex SHA-256 prefix, and
redactkeepstoken_fingerprintwhile dropping secret-looking keys. - The raw token does not cross the socket: the broker builds the response field by field, and
broker.test.tsasserts the exact response with notokenFingerprint; only the audit record carries it. - Tests cover both halves: the provider test checks the fingerprint is derived from the minted action token, and the broker test checks that command, permissions, duration and fingerprint reach the audit event.
Non-blocking notes, no change requested:
command: spec.command.join(' ')recordspr view, while the ROADMAP table writesgh pr view; both are defensible, but they are not the same string.- The duration assertion only checks
typeof === 'number';>= 0would state the field's meaning. - Pre-existing, not this PR: the test name "fingerprints deterministically and reversibly" is misleading — a fingerprint is not reversible.
No blockers; ready to merge.
Record the full gh command, assert a non-negative duration, and describe the fingerprint as deterministic rather than reversible.
The extra gh prefix was a display normalization, not the command the catalog actually stores; audit the original array.
i1stG
left a comment
There was a problem hiding this comment.
Approved as @i1stG at 4a82cba.
Re-verified after the update:
- The net diff from the previously approved
0da394bis test-only: the duration assertion now requires a non-negative number, and the fingerprint test name no longer says "reversibly".packages/host/src/broker.tsis byte-identical to what I approved; the intermediateghprefix was added and reverted, so the audit keeps the catalog command (pr view). corepack yarn check: 283/283.- CI green on Linux, WSL, Windows and macOS. CodeRabbit skipped this push (manual review required) but had no actionable comments on the previous head.
No blockers; ready to merge.
Allowed action audit records now include the catalogued command, its permissions, the provider duration, and the action token fingerprint. The token itself never enters the record; the provider returns only a SHA-256 prefix.
This completes the Phase 2 audit item, so its ROADMAP checkbox is now checked.
corepack yarn check: 283 pass.Summary by CodeRabbit