fix(credentials): fail closed on plaintext credential-reveal routes#271
fix(credentials): fail closed on plaintext credential-reveal routes#271chitcommit wants to merge 2 commits into
Conversation
GET /api/credentials/:vault/:item/:field and GET /api/credentials/twilio returned broker-resolved secrets as plaintext in the response body, gated only by `keyInfo.status === "active"` (src/api/middleware/auth.js:142). There were no scope checks anywhere in either file, so any active API key could reveal any credential across the infrastructure, services and integrations vaults. Adds src/api/middleware/require-scope.js and applies it to both routes: - Deny-by-default. A key with no `scopes` field, an empty array, or a non-array value is rejected. There is no allow-with-warning mode; the sensitive-intent contract requires the credential path to fail closed, and a warning nobody reads is an open door. - Per-vault scopes (`credentials:reveal:<vault>`) derived from the same vault list the handler validates against, so an unrecognised vault has no grantable scope and is denied rather than guessed at. - Namespaced separately from the MCP OAuth vocabulary. Both land in the same `keyInfo.scopes` array, and `mcp:admin` must not satisfy a credential-reveal check by vocabulary coincidence. - The 403 names the required scope so an under-provisioned caller can be reissued a correct key without reading this source. Scope-gating is harm reduction, not the cure: a scoped plaintext reveal is still a plaintext long-lived secret in a response body. No in-repo consumer of either route exists (server-side callers use createCredentialBroker in process and are unaffected), so the intended follow-up is deletion once 403 telemetry confirms no external caller. Tracked separately. Tests mount the real route module with no env bindings — the handlers need a broker and a DB, so a clean 403 is positive evidence the guard short-circuits ahead of the handler. Refs: operator decision 2026-07-25 (decision 5) Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCredential reveal authorization is added through shared scope middleware. Twilio requires the integrations reveal scope, while generic credential routes derive reveal scopes from validated vault names. Tests cover scope resolution, denial behavior, route wiring, and payload non-disclosure. ChangesCredential reveal scope enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant credentialsRoutes
participant requireScopeFrom
participant CredentialHandler
Client->>credentialsRoutes: request credential endpoint
credentialsRoutes->>requireScopeFrom: resolve vault reveal scope
requireScopeFrom-->>Client: return 403 for unresolved or insufficient scope
requireScopeFrom->>CredentialHandler: continue authorized request
CredentialHandler-->>Client: return credential response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds an explicit authorization layer (scope checks) to the plaintext credential-reveal endpoints under src/api/routes/credentials.js, so that “active key” is no longer sufficient to retrieve broker-resolved secrets. This fits into the API layer by extending the existing authenticate identity check with a deny-by-default authorization middleware and validating the wiring via real-route Vitest coverage.
Changes:
- Introduces
requireScope/requireScopeFrommiddleware to enforce per-route/per-vault scopes (fail-closed). - Wires scope enforcement onto
GET /api/credentials/twilioandGET /api/credentials/:vault/:item/:field. - Adds behavioral tests that exercise both the middleware directly and the real credentials route module mounting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/api/require-scope.test.js | Adds behavioral tests for fixed-scope and derived-scope enforcement, plus real route wiring checks. |
| src/api/routes/credentials.js | Applies scope enforcement middleware to the two plaintext credential-reveal GET routes; centralizes valid vault list. |
| src/api/middleware/require-scope.js | Adds deny-by-default scope enforcement middleware and helper utilities for credential reveal scope vocabulary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * allow-with-warning mode — per the sensitive-intent contract | ||
| * (~/.ch1tty/canon/system-wide-sensitive-intent-contract-v1.md) the credential | ||
| * path fails closed, and a warning nobody reads is an open door. |
| /** | ||
| * Vaults the credential routes expose. Kept in sync with the `validVaults` | ||
| * check in ../routes/credentials.js — a vault absent here has no reachable | ||
| * scope and is therefore denied, which is the correct direction to fail. | ||
| */ |
| async function enforce(c, next, scope) { | ||
| const keyInfo = c.get("apiKey") || {}; | ||
| const granted = scopesOf(c); | ||
| const allowed = granted.includes(scope); | ||
|
|
||
| console.log( | ||
| JSON.stringify({ | ||
| tag: "scope-audit", | ||
| scope, | ||
| allowed, | ||
| keyType: keyInfo.type || "api-key", | ||
| service: keyInfo.service || keyInfo.name || "unknown", | ||
| path: c.req.path, | ||
| }), |
Governance gap found while verifying this PR's own green checks
Both gates classify a change as security-critical by path prefix:
Neither matches Also unmatched by both lists:
Net effect: a PR that rewrites the credential authorization path requires zero security-approval labels. This PR is the demonstration — it changes exactly that surface and both gates are green. The two gates also disagree with each other ( Suggested fix, as a separate PR: consolidate to one shared critical-path list and extend it to cover I have not applied the |
What
GET /api/credentials/:vault/:item/:field(credentials.js:573) andGET /api/credentials/twilio(:467) return broker-resolved secrets as plaintext in the response body. The only authorization gate waskeyInfo.status === "active"at auth.js:142 — grep forscopeacross both files finds zero authorization checks. Any active API key could reveal any credential across theinfrastructure,services, andintegrationsvaults.This adds
src/api/middleware/require-scope.jsand applies it to both routes, deny-by-default.Design decisions
scopesall reject. The sensitive-intent contract requires the credential path to fail closed; a log-only phase means the hole stays fully open for the window and tends not to get flipped.credentials:reveal:{infrastructure,services,integrations}, derived from the same vault list the handler validates against. An unrecognised vault yields no scope and is denied rather than guessed at.mcp:read|write|admin. Both vocabularies land in the samekeyInfo.scopesarray;mcp:adminmust not satisfy a credential-reveal check by coincidence. There's an explicit test for this.required: "<scope>"so an under-provisioned caller can be reissued without reading source.Scope-gating is harm reduction, not the cure
A scoped plaintext reveal is still a plaintext long-lived secret in a response body. No in-repo consumer of either route exists — server-side callers (
github-actions.js:172,credential-provisioner-enhanced.js:27,credential-helper.js:18) usecreateCredentialBroker(env)in-process and are unaffected. Intended follow-up is deleting both routes once 403 telemetry confirms no external caller. Filed separately so gating doesn't quietly become the destination.Validation
npx vitest run→ 512 passed, 1 skipped, 0 failednpx eslinton all three changed files → cleanvi.mock()on any DB or service module; real Hono app, real Requests, real Responses.Not in this PR (audited, filed separately)
API_KEYS.put()sets noexpirationTtl— keys never expire, KV as authz source of truthsrc/middleware/mcp-auth.js:173,203env[X]truthiness on Secrets-Store bindings;resolveBinding()atthirdparty.js:890already solves it and isn't usedsrc/lib/credential-helper.js:68-81,101-121,src/intelligence/context-resolver.js:530-532failoverToEnvironmentreads arbitrary env vars on broker failure — dead under current config, butCREDENTIAL_FAILOVER_ENABLED="true"in all 3 envssrc/services/chittysecrets-connect-client.js:246-249POST /api/credentials/retrieve,/audit— no such routes)src/mcp/tool-registry.js:962,985docs/SECRET_MANAGEMENT_ARCHITECTURE.md:81wrangler.jsoncdocs/governance/Refuted during audit, recorded so it isn't re-raised: MCP OAuth tokens do not escalate via the
unwrapTokenbranch atauth.js:114.isOAuthProviderRoute()(index.js:2214) covers only/authorize,/token,/registerand two.well-knownpaths, so/api/*usesapp.fetchwith raw env whereOAUTH_PROVIDERis undefined.Deploy note
Merging does not deploy via GitHub Actions — only
binding-drift-audit.ymlruns on push to main, and it's an audit. I could not verify whether a Cloudflare Workers Build is configured to deploy this repo on push (no CF API token in the authoring session). If one is, merging changes production auth behavior immediately. Confirm before merge.Refs: operator decision 2026-07-25 (decision 5) — standalone remediation, deliberately not folded into the chittycan OAuth work.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes