Skip to content

feat(cli): audit subcommand for station configuration sanity checks#638

Merged
MRmarioruci merged 13 commits into
mainfrom
feat/cli-audit-subcommand
Jun 24, 2026
Merged

feat(cli): audit subcommand for station configuration sanity checks#638
MRmarioruci merged 13 commits into
mainfrom
feat/cli-audit-subcommand

Conversation

@MRmarioruci

Copy link
Copy Markdown
Contributor

Summary

  • New orbit-cli audit --station <CANISTER_ID> subcommand: read-only sanity checks against an Orbit station's configuration.
  • Three checks ship in this first release: an empty-approver-set Quorum trap, a validation/execution-method match on CallExternalCanister, and an EditAsset-weaker-than-Transfer configuration warning.
  • Uses the same dfx canister call --output json pattern as registry/ and release/. No new runtime dependencies. Read-only by construction — only query methods are invoked.

Motivation

Several station-configuration shapes that look fine in the wallet UI can silently disable approval gates or create routing-redirect vectors:

  • A Quorum / QuorumPercentage rule whose UserSpecifier resolves to zero active users auto-approves with no votes — the evaluator's min(min_approved, total_possible_approvers) clamp drives the threshold to 0.
  • A CallExternalCanister permission or policy where validation_method == execution_method runs the side effect at submission time, before the approval policy has completed.
  • An EditAsset policy gated more loosely than Transfer for the same asset lets an actor mutate asset.metadata["ledger_canister_id"] between an approved transfer's policy evaluation and its batch-job execution.

These all require manual UI walkthroughs to spot today. orbit-cli audit makes the check a one-command read-only sweep that can run on demand or in CI.

What changed

  • New cli/src/audit/:
    • index.ts — Commander command (-s/--station, -n/--network, -i/--identity, -o/--output).
    • station.core.ts — paginated dfx canister call wrappers for list_request_policies, list_users, list_user_groups, list_assets, list_named_rules, list_permissions.
    • types.ts — hand-written TS subset of the station API; small enough that the didc bind toolchain dependency isn't worth pulling in for this MVP.
    • resolver.tsresolveApprovers and minVotesForRule mirroring the relevant subset of find_matching_users and RequestApprovalSummary::evaluate from core/station/impl/src/models/request_policy_rule.rs; walks NamedRule chains with cycle detection.
    • report.tsFinding type, severity-sorted text formatter, exit-code helper.
    • checks/*.ts — one file per check; each returns Finding[].
    • checks/*.spec.ts — unit tests, 19 cases across positive and negative paths.
    • README.md — usage, exit codes, check catalogue, extension notes, roadmap.
  • cli/src/cli.ts — register the new subcommand alongside registry and release.
  • cli/package.json — add vitest 1.6.1 as a dev dependency (matching the root pin) and a test script.
  • pnpm-lock.yaml — updated for the vitest addition.

Exit codes: 0 clean / 1 warnings only / 2 any blockers. Convenient for CI gating.

--output <PATH> writes the report to a file (overwrite); the exit code is unchanged and a short confirmation is emitted to stderr so file-mode runs still give feedback without polluting the report.

Test plan

  • pnpm --filter orbit-cli test — 19 vitest cases pass (3 spec files). Covers each check's positive and negative paths, combinator descent (AnyOf), NamedRule resolution, and cycle detection.
  • pnpm --filter orbit-cli buildtsc clean.
  • Prettier + ESLint clean on all new files.
  • Manual smoke test against a mainnet station: created an empty user group, a NamedRule pointing at it, a Request Policy referencing the NamedRule, a CallExternalCanister permission with validation == execution, and a weak EditAsset policy — the audit reported 2 BLOCKERs + 1 WARNING with exit code 2. Cleanup returned the station to 0 findings, exit code 0.
  • Reviewers may want to run it against their own local or mainnet stations.

Notes for reviewers

  • The implementation deliberately mirrors the existing registry/ subcommand's structure (shell out via dfx canister call --output json, paginated list_*, hand-typed result shapes) so it slots into the codebase without introducing new patterns.
  • The asset.edit-policy-weaker-than-transfer check compares station-wide rather than per-asset/per-account — that's a deliberate MVP shortcut documented in the README's roadmap. Per-asset scoping is a planned follow-up.
  • No security-ticket / disclosure references appear in the code or commit history; the README explains each check in technical terms only.

Adds `orbit-cli audit --station <id>` for read-only sanity checks against
a station's configuration. The command pulls live state via the existing
`list_*` query methods and runs static checks against the configuration.

Ships three checks in this first release, each with a stable check id and
severity:

- `quorum.empty-approver-set` (BLOCKER) — Quorum / QuorumPercentage rules
  whose UserSpecifier currently resolves to zero active users; the
  evaluator's clamp to total_possible_approvers makes such a rule auto-
  approve with no votes cast.
- `external-call.validation-equals-execution` (BLOCKER) — sweeps request
  policies and permissions for CallExternalCanister resource targets
  where validation_method == execution_method; the validation hook runs
  before approval, so a side-effecting matching pair bypasses the gate.
- `asset.edit-policy-weaker-than-transfer` (WARNING) — flags when the
  easiest EditAsset path is strictly weaker than the strictest Transfer
  policy. Asset routing (ledger_canister_id) is resolved live at execute
  time, so a successful EditAsset between approval and execution can
  redirect funds.

The command is read-only by construction — only query methods are called.
Exit codes follow standard semantics (0 clean, 1 warning, 2 blocker) so
CI pipelines can gate on it directly. `--output <PATH>` writes the report
to a file instead of stdout.

See `cli/src/audit/README.md` for usage, the check catalogue, and notes
for adding new checks.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new orbit-cli audit subcommand that performs read-only sanity checks against an Orbit Station configuration by fetching station state via dfx canister call --output json, running a small set of static checks, and emitting a severity-sorted report with CI-friendly exit codes.

Changes:

  • Registers a new audit CLI subcommand and implements station query wrappers + report rendering.
  • Introduces three initial configuration checks (quorum empty approver set, external call validation==execution, EditAsset weaker than Transfer), with unit tests and documentation.
  • Adds vitest and a test script to the orbit-cli package.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pnpm-lock.yaml Locks vitest addition for the CLI workspace.
cli/package.json Adds vitest dev dependency and test script for orbit-cli.
cli/src/cli.ts Registers the new audit subcommand.
cli/src/audit/index.ts Implements the orbit-cli audit command wiring, fetching station state, running checks, and setting exit codes.
cli/src/audit/station.core.ts Adds paginated dfx canister call wrappers for station list_* query methods.
cli/src/audit/types.ts Hand-written TS types for the station API subset needed by the audit.
cli/src/audit/resolver.ts Implements static resolver utilities (resolveApprovers, quorum walker, and min-votes estimator).
cli/src/audit/report.ts Defines findings/report format, rendering, and exit-code logic.
cli/src/audit/checks/* Implements the three initial checks plus test fixtures and Vitest specs.
cli/src/audit/README.md Documents usage, exit codes, checks, and extension notes.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cli/src/audit/station.core.ts Outdated
Comment thread cli/src/audit/checks/quorum-empty-approver-set.ts Outdated
Comment thread cli/src/audit/checks/asset-edit-policy-weaker-than-transfer.ts Outdated
@MRmarioruci MRmarioruci requested a review from aterga June 24, 2026 14:46
@MRmarioruci MRmarioruci marked this pull request as ready for review June 24, 2026 14:46
@MRmarioruci MRmarioruci requested a review from a team as a code owner June 24, 2026 14:46
@zeropath-ai

zeropath-ai Bot commented Jun 24, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to c9a6c1d.

Security Overview
Detected Code Changes

The diff is too large to display a summary of code changes.

- station.core: shell-escape `--station`, `--network`, `--identity` so a
  value containing `'` cannot break out of the surrounding quoting and
  inject shell syntax.
- quorum.empty-approver-set: when the rule is `QuorumPercentage`, the
  `min_approved` field is a percentage rather than a vote count — render
  the finding message accordingly so operators read it correctly. Added a
  positive test for the percentage wording.
- asset.edit-policy-weaker-than-transfer: clarify in the finding message
  that the reported approval counts are lower-bound estimates (the
  `minVotesForRule` helper reduces `AllOf` via max-of-children, so the
  true minimum may be higher). Tests updated to match.
Comment thread cli/src/audit/station.core.ts Outdated
Comment thread cli/src/audit/resolver.ts
Per review feedback (#638 (comment)),
swap `dfx canister call --output json` for `@dfinity/agent` directly:

- station.core: paginated wrappers now drive an `ActorSubclass` built once at
  audit start. No shell, no string interpolation, no command-injection class.
- agent.ts: builds the `HttpAgent` + `Actor`, resolving the network → host
  mapping (mainnet hardcoded to icp-api.io; other networks fall back to
  dfx.json so local replicas still work).
- identity.ts: loads a plaintext Ed25519 dfx PEM via Node's crypto and the
  PKCS#8 envelope, hands the raw seed to `Ed25519KeyIdentity.fromSecretKey`.
  Encrypted PEMs are refused with a clear error pointing operators at the
  `--storage-mode plaintext` flag.
- index.ts: builds the actor once and passes it to each list call.
- generated/: vendored copy of `station.did.{js,d.ts,did}` so the audit can
  type the actor without reaching into apps/wallet. `npm run
  generate-station-types` script regenerates from `core/station/api/spec.did`
  via `didc bind`.
- README updated to reflect that the dfx CLI is no longer needed at runtime
  — only the PEM file in dfx's identity-store layout.

The 52 unit tests still pass (they mock at the check level, not the actor
plumbing). End-to-end verification is the same as before: point at a real
station with `--network ic --identity orbit-audit-plaintext`.
Adds a second loader alongside the existing dfx PEM path so operators can run
the audit signed as any identity registered with icp-cli, including
delegation-based identities (Internet Identity / okta-style logins) that
can't be exported as a self-contained PEM.

- New `--identity-source dfx|icp` flag (default `dfx`).
- `identity-icp.ts`: reads icp's `identity_list.json`, branches on `kind`:
  - `anonymous` → `AnonymousIdentity`.
  - `keyring` (Ed25519) → shells `icp identity export <name>` and parses the
    PEM with the same helper the dfx loader uses.
  - `internet-identity` → generates an Ed25519 session keypair locally, asks
    `icp identity delegation sign` to mint a 1-hour delegation for that
    session key, and assembles a `DelegationIdentity`. The session secret
    key never leaves the process.
  - `hsm` / `keyring`-Secp256k1 → unsupported with clear errors.
- `identity.ts` refactored: `parseEd25519Pem` and `b64urlDecode` extracted so
  both loaders share the PKCS#8 parsing path.
- `loadIdentity(source, name)` dispatcher used by `buildStationActor`.
- README documents both sources.

The 52 existing unit tests still pass — none touch the identity layer.
@MRmarioruci MRmarioruci enabled auto-merge (squash) June 24, 2026 16:51
@MRmarioruci MRmarioruci merged commit e330928 into main Jun 24, 2026
28 of 29 checks passed
@MRmarioruci MRmarioruci deleted the feat/cli-audit-subcommand branch June 24, 2026 17:11
MRmarioruci added a commit that referenced this pull request Jun 24, 2026
## Summary

- `cli/src/audit/generated/station.did.js` (vendored from `didc bind
--target js`) uses ES module syntax. The cli package is plain CommonJS.
- On Node ≤ 20.18 — the version pinned by `.nvmrc` and the one CI uses —
`require()` of that file throws `SyntaxError: Unexpected token 'export'`
at module-load time. **Every** orbit-cli invocation fails: `registry
publish`, `release publish`, `audit`, even `--help`.
- This is also the underlying cause of #638's e2e regression
(`disaster-recovery.spec.ts › can recover uninstalled station` failing
at \`execSync(\`orbit-cli registry publish --app station\`)\`).

## Motivation

Newer Node versions (≥ 20.20, backported from Node 22's
\`--experimental-detect-module\`) silently auto-detect ESM in \`.js\`
files, which masked the problem locally during development. CI's pinned
Node 20.18 has no such fallback.

## What changed

- **`cli/src/audit/generated/station.did.js`** — transformed via a
single sed substitution: \`export const X = ...\` → \`exports.X = ...\`.
Two top-level exports affected: \`idlFactory\` and \`init\`.
- **`cli/package.json` (\`generate-station-types\`)** — same sed pipe
added to the regen script so future re-generations from \`spec.did\`
don't reintroduce the breakage.

## Test plan

- [x] \`pnpm --filter orbit-cli build\` — clean.
- [x] \`pnpm --filter orbit-cli test\` — 52 vitest cases pass (no test
surface touched).
- [x] \`node cli/dist/cli.js registry --help\` — loads cleanly (was
failing on Node 20.18 with the syntax error).
- [x] \`node cli/dist/cli.js audit --help\` — loads cleanly.
- [ ] CI \`e2e-tests:required\` — should now get past \`orbit-cli
registry publish\` to whatever the actual disaster-recovery test does.

Follow-up to #638.
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.

3 participants