feat(provider-security): ChefVault client, slots, degraded mode - #38
Open
OnlineChef wants to merge 12 commits into
Open
feat(provider-security): ChefVault client, slots, degraded mode#38OnlineChef wants to merge 12 commits into
OnlineChef wants to merge 12 commits into
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds ChefVault-backed provider credentials with leased in-memory storage, renewal, fencing, degraded-mode handling, asynchronous provider resolution, server forwarding, CLI diagnostics, and comprehensive tests. ChangesChefVault provider security
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ClientRequest
participant ResponseHandler
participant ProviderCredentialResolver
participant ProviderSecurityClient
participant ChefVaultAuthority
participant UpstreamProvider
ClientRequest->>ResponseHandler: submit provider request
ResponseHandler->>ProviderCredentialResolver: resolve credentialRef
ProviderCredentialResolver->>ProviderSecurityClient: resolve or renew lease
ProviderSecurityClient->>ChefVaultAuthority: authenticated lease request
ChefVaultAuthority-->>ProviderSecurityClient: lease and fencing token
ProviderSecurityClient-->>ProviderCredentialResolver: validated credential lease
ProviderCredentialResolver-->>ResponseHandler: request-local credential
ResponseHandler->>UpstreamProvider: forward authenticated request
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
3 tasks
github-actions
Bot
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+469
to
+475
| const credentialRef = prov.credentialRef?.trim(); | ||
| if (credentialRef && isChefVaultRef(credentialRef)) { | ||
| try { | ||
| const resolved = await globalProviderCredentialResolver.resolveCredentialRef(credentialRef); | ||
| return resolved.apiKey; | ||
| } catch { | ||
| return undefined; |
There was a problem hiding this comment.
This resolves a chefvault:// reference only for model discovery. A provider configured solely with credentialRef therefore authenticates /models, but ordinary response forwarding still consumes provider.apiKey and reaches the upstream without an Authorization header. Compact requests and live/image provider selection likewise use only API-key configuration. Resolve the credential reference into a per-request effective credential before routing these paths, without persisting the resolved secret.
Artifacts
CredentialRef authorization harness source
- The authored Bun harness starts mock ChefVault and upstream services, starts the proxy, and captures the discovery and response Authorization headers; the takeaway is that the exact tested flow is reproducible.
API-key authorization baseline output
- The executed API-key baseline records Bearer authorization on both model discovery and the proxied response request; the takeaway is that the capture server observes expected authentication when apiKey is configured.
CredentialRef-only observed output
- The executed credentialRef-only run records the ChefVault secret on `/v1/models` and null authorization on `/v1/chat/completions`; the takeaway is that ordinary response forwarding is unauthenticated.
Harness source checksum output
- The executed checksum command records the SHA-256 for the uploaded harness source; the takeaway is that the uploaded source is traceable to the tested file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/oauth/index.ts
Line: 469-475
Comment:
**ChefVault credentials are omitted from provider requests**
This resolves a `chefvault://` reference only for model discovery. A provider configured solely with `credentialRef` therefore authenticates `/models`, but ordinary response forwarding still consumes `provider.apiKey` and reaches the upstream without an Authorization header. Compact requests and live/image provider selection likewise use only API-key configuration. Resolve the credential reference into a per-request effective credential before routing these paths, without persisting the resolved secret.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+63
to
+69
| await this.tryRenew(ref, renewTarget.leaseId, renewTarget.fencingToken, "active").catch(() => { | ||
| // Renewal failure keeps the current snapshot until expiry; degraded handling applies on resolve. | ||
| }); | ||
| const refreshed = this.slotStore.snapshotForRequest(ref, this.now()); | ||
| if (refreshed) { | ||
| return { apiKey: refreshed.secret, snapshot: refreshed, source: "memory" }; | ||
| } |
There was a problem hiding this comment.
When ChefVault rejects renewal as revoked, this catch suppresses the error and then returns the still-unexpired active snapshot. An explicitly revoked credential can therefore continue authenticating requests until its previous expiration. Handle revocation separately by evicting or revoking the affected slot and rejecting the request; retain cached fallback only for transient authority failures.
Artifacts
Renewal revocation runtime harness source
- Authored Bun harness seeds a renewable active lease, makes the real client receive HTTP 410 revoked from renewal, and resolves the credential; it captures whether the cached secret is returned. The takeaway: it directly tests whether revocation is honored during renewal.
Runtime output before validation harness refinement
- Executed initial harness output shows a POST renewal response recorded as HTTP 410 with revoked mapping, followed by the still-valid cached secret returned from memory. The takeaway: the revoked credential remains usable.
Runtime output after direct HTTP mapping verification
- Executed refined harness directly verifies the HTTP 410 response maps to revoked and then shows resolution returns the active cached secret, confirming the defect. The takeaway: the resolver suppresses revoked renewal errors and retains the active credential.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/provider-security/resolve.ts
Line: 63-69
Comment:
**Revoked leases remain usable after renewal**
When ChefVault rejects renewal as `revoked`, this catch suppresses the error and then returns the still-unexpired active snapshot. An explicitly revoked credential can therefore continue authenticating requests until its previous expiration. Handle revocation separately by evicting or revoking the affected slot and rejecting the request; retain cached fallback only for transient authority failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
OnlineChef
force-pushed
the
feat/provider-security-plane
branch
from
August 1, 2026 07:39
02bc847 to
b89feab
Compare
…stub (PSP-008) Add provider-security plane client for chefvault:// credential refs with workload headers, active/next/retiring/revoked slot model, renewal jitter, redacted doctor/status surfaces, and bounded degraded mode when ChefVault is unreachable. Wire minimal resolve path for provider doctor/status and models auth. Co-authored-by: Cursor <cursoragent@cursor.com>
Add in-memory credential slots, workload-authenticated ChefVault client, chefvault:// resolve path, redacted doctor/status, and bounded degraded mode when the secret authority is unavailable. Co-authored-by: Cursor <cursoragent@cursor.com>
CI failed on bun install --frozen-lockfile after provider-security and fallback GUI dependency resolution drifted from the lockfile. Co-authored-by: Cursor <cursoragent@cursor.com>
Group prompt.txt appends in issue-triage and keep bun.lock aligned with frozen CI installs on provider-security branches. Co-authored-by: Cursor <cursoragent@cursor.com>
OnlineChef
force-pushed
the
feat/provider-security-plane
branch
from
August 1, 2026 07:44
fc1ab0e to
fea8ce1
Compare
… request Resolving a chefvault:// reference only for model discovery left ordinary forwarding, compaction, and the live/images/bridge tiers authenticating from provider.apiKey alone, so a credentialRef-only provider reached upstream with no Authorization header. Route every provider-key path through a shared per-request resolver that leases the secret in memory and fails closed. Also stop suppressing a revoked renewal: an authority revocation now evicts the active slot and rejects the request instead of serving the cached secret until its old expiry. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
OnlineChef
commented
Aug 1, 2026
OnlineChef
left a comment
OnlineChef
left a comment
Author
There was a problem hiding this comment.
MERGE BLOCKER — cross-repo contract mismatch against merged ChefVault lidge-jun#86.
ProviderSecurityClient sends only X-Chef-* identity assertions. The merged Vault HTTP surface authenticates /v1/credentials/resolve, /v1/credentials/renew, and /v1/refs/* with Authorization: Bearer <workload token> before accepting those assertions. Therefore this PR's client cannot resolve or renew a credential against the merged authority; it will receive 401. The current Sofie smoke/green CI does not prove compatibility with the merged authenticated contract.
Required before merge:
- Add workload-token configuration (for example
CHEF_PROVIDER_SECURITY_TOKEN) and send the bearer header on authenticated ChefVault calls without logging or serializing it. - Add a cross-repo-compatible integration test using the Vault token registry/server contract: no token => 401, valid workload token + matching headers => success, mismatched asserted identity => 403.
- Split doctor/status into unauthenticated liveness and authenticated readiness.
/healthzalone can currently report green while every lease call fails. - Preserve fail-closed behavior across responses, compact, model discovery, live, images, and video when credential resolution fails.
- Remove or split unrelated
.github/workflows/issue-triage.ymlandgui/bun.lockchurn from this security PR.
The two earlier Greptile findings (ordinary forwarding missing the leased key and revoked renewal retaining the cached lease) appear fixed in the current head with regression tests; their threads should be rerun/resolved after the authenticated integration lane passes.
…readiness Load CHEF_PROVIDER_SECURITY_TOKEN on the client, attach Authorization on protected routes, fail closed on resolve/renew without a token, and split doctor checks into liveness, authenticated readiness, and resolve readiness. Co-authored-by: Cursor <cursoragent@cursor.com>
… PSP PR Revert issue-triage workflow and gui/bun.lock to origin/dev so PSP CI stays scoped to provider-security changes only. Co-authored-by: Cursor <cursoragent@cursor.com>
…en code Co-authored-by: Cursor <cursoragent@cursor.com>
Author
Follow-up to MERGE BLOCKER reviewAddressing the Vault Bearer gap (not merging yet):
Still blocked on: green CI for these follow-ups + confirmed live deepseek resolve under TokenRegistry. |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…probes Only authority outages enter degraded mode; revoked renewals evict and fail closed; in-flight resolve coalesces concurrent cold starts; status reporting is read-only and skips probing when no refs are configured; images/compact surface credential failures with the correct status. refs #38 Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/oauth/index.ts`:
- Around line 470-480: Update resolveModelsAuthToken to attempt
globalProviderCredentialResolver.resolveCredentialRef for every non-empty
trimmed credentialRef, removing the isChefVaultRef pre-check. Preserve the
existing resolved.apiKey return and failure behavior so malformed references are
rejected by the resolver rather than falling through to
resolveEnvValue(prov.apiKey).
In `@src/provider-security/client.ts`:
- Around line 178-211: Move the authenticated token resolution via
requireToken() in the request method before creating the AbortController and
arming timer, while preserving the existing authorization header behavior.
Ensure unauthenticated failures occur before timer allocation, and keep the
current try/finally cleanup for timers used by fetch requests.
- Around line 101-138: Extract the shared lease-field extraction and validation
from parseResolveResponse and parseRenewResponse into a single helper,
preserving the existing required-field and finiteness checks. Update both
parsers to reuse that helper, with parseResolveResponse continuing to apply its
slotHint-specific handling while parseRenewResponse returns only the common
fields.
In `@src/provider-security/resolve.ts`:
- Around line 116-144: Update resolveFromAuthority to distinguish genuine
authority outages from malformed responses, local authority_error failures, and
exceptions during applyResolve or snapshot handling; only mark degraded for
confirmed transport/availability failures, preserving the original error
otherwise. Enforce degraded mode by checking DegradedModeController.canResolve
before calling client.resolveLease and deny without issuing a request when
recovery is not permitted; retain the existing successful recovery path. Add
coverage in tests/provider-security.test.ts for malformed responses not entering
degraded mode and degraded resolution making no authority request.
In `@src/provider-security/slots.ts`:
- Around line 177-182: Make revocation reference-wide rather than
phase-specific: add a revokeRef method to the slot store that invalidates the
entire state, including active and retiring leases, and use it from
resolveCredentialRef when providerSecurityErrorCode(error) is "revoked" instead
of revokePhase(ref, "active"). Add a focused lease-lifecycle regression test
covering renewal revocation with a retiring lease and assert the next resolution
fails closed.
- Around line 185-206: Update redactedStatus so each RedactedSlotSummary uses
the lease object's phase rather than the iteration key, allowing revoked leases
stored in another slot to be reported as revoked. Keep the existing slot lookup,
validity calculation, and hasUsableCredential logic unchanged.
In `@src/provider-security/status.ts`:
- Around line 62-67: Move the refs.length === 0 early return in
collectProviderSecurityDoctorChecks to the beginning of the function, before any
resolver.client.healthz or authenticatedReady probes, and remove the duplicate
later guard. Add a regression test in provider-security.test.ts for zero
references that verifies no authority probe occurs and the function returns the
expected checks.
- Around line 46-60: Update collectProviderSecurityStatus to use the scoped
store from ProviderCredentialResolver via
resolver.slotStore.redactedStatus(entry.ref) instead of the
globalCredentialSlotStore, ensuring provider-layer status matches the resolver’s
configured store.
- Around line 79-90: Centralize redaction of provider-security error details and
apply it before any status or doctor output is constructed. Update the status
authority message paths around resolver.probeAuthenticatedReady(),
liveness.message, and mapAuthorityError/healthz responses, plus the doctor
credential-check detail from resolver.resolveCredentialRef(ref), so serialized
or printed messages contain only safe redacted text while preserving success and
failure status fields.
In `@src/provider-security/types.ts`:
- Around line 129-145: Update validateChefVaultRef to accept the host-only
references already accepted by isChefVaultRef, such as
chefvault://deepseek-api-key. Validate that the parsed URL has either a
non-empty host or a resource path, instead of rejecting solely because pathname
is empty, while preserving protocol and malformed-URI checks.
In `@tests/provider-security-forwarding.test.ts`:
- Around line 69-78: Add an assertion in the mocked ChefVault resolve branch of
the relevant provider-security forwarding test to verify init?.headers contains
Authorization: Bearer ${VAULT_TOKEN}, then assert the resolve response status
remains 200. Keep the existing URL matching and upstream authorization
assertions unchanged.
In `@tests/provider-security.test.ts`:
- Around line 118-128: Update the stale-token case in the “rejects stale fencing
tokens” test to capture the error thrown by store.applyResolve for fencingToken:
3 using an assertion that always fails when no error is thrown, then assert the
captured ProviderSecurityError code is “stale_fencing_token”.
- Around line 73-81: Add negative test cases in the “chefvault ref validation”
suite for an HTTP-scheme ref, an empty chefvault path, traversal segments, and
whitespace/control characters; assert each validateChefVaultRef result has code
“ref_invalid” while preserving the existing valid and bare-path cases.
- Around line 350-398: Add focused regression tests in server-images.test.ts for
selectImagesProvider using a chefvault-backed provider and mocked
resolveProviderApiKey failures. Assert revoked errors produce HTTP 401 with
authentication_error, while authority_unavailable errors produce HTTP 503 with
api_error, covering the openai-sidecar mapping and retry classification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb645a0d-b6c8-4f3f-a203-138703ba4ac8
📒 Files selected for processing (22)
src/cli/doctor.tssrc/cli/status.tssrc/images/plan.tssrc/oauth/index.tssrc/provider-security/client.tssrc/provider-security/degraded.tssrc/provider-security/index.tssrc/provider-security/resolve.tssrc/provider-security/slots.tssrc/provider-security/status.tssrc/provider-security/types.tssrc/providers/credential.tssrc/providers/openai-sidecar.tssrc/server/auth-cors.tssrc/server/images.tssrc/server/live.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/types.tstests/provider-security-forwarding.test.tstests/provider-security.test.tstests/server-images.test.ts
…octor guards - degraded mode gates resolve before any authority call, with a bounded 30s recovery probe; malformed responses (authority_error) no longer enter degraded mode - revoked renewals revoke every slot for the ref (active/next/retiring) so rotation leftovers fail closed - doctor returns early when no chefvault refs are configured, uses the resolver's slot store, and redacts authority error detail in status/doctor messages - validateChefVaultRef validates the raw ref string (whitespace/control chars, traversal, empty segments) and accepts host-only refs; redactedStatus reports the lease's own phase - ChefVault client: dedupe lease parsing, build headers before arming the request timeout so early throws cannot leak the timer - resolveModelsAuthToken defers all non-empty credentialRefs to the resolver so invalid refs fail closed instead of silently falling through to apiKey - tests: negative ref cases, degraded no-probe/recovery-interval coverage, retiring-slot revocation, doctor zero-ref guard, and vault bearer assertion in forwarding fixture Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…ovider A revoked lease must surface as a non-retryable 401 authentication_error and an unreachable authority as a retryable 503 api_error, covering the selectImagesProvider -> providerCredentialFailure split used by Codex. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
bun test ./tests/provider-security.test.ts(12 pass)Made with Cursor
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Greptile Summary
This change adds ChefVault-backed provider credentials, lease handling, diagnostics, configurable provider fallback chains, and provider workspace updates.
Two authentication failures were reproduced. A provider configured only with a ChefVault credential reference can use that credential for model discovery but sends ordinary response traffic upstream without authentication. Also, an explicitly revoked ChefVault lease remains available from memory until its original expiration. These failures affect provider access control and should be corrected before merging.
Confidence Score: 4/5
Not safe to merge while revoked credentials can remain usable and credential-reference-only providers cannot authenticate ordinary requests.
The reproduced failures independently affect core provider authentication: one retains access after explicit revocation, and the other prevents ChefVault credentials from being applied to normal provider traffic.
Files Needing Attention: src/provider-security/resolve.ts needs explicit revoked-lease invalidation. src/oauth/index.ts and the ordinary request paths that consume provider.apiKey need request-time ChefVault credential application.
Security Review
ChefVault revocation is not enforced during lease renewal. When the authority returns a revoked response, the cached secret remains available and can continue authenticating requests until expiry. Separately, credential-reference-only provider configurations omit their resolved secret from ordinary response forwarding, which causes unauthenticated provider traffic and prevents the intended centrally managed credential flow from working.
What T-Rex did
Comments Outside Diff (1)
General comment
credentialRef: chefvault://...successfully resolves a secret for/models, yet a real proxied/v1/responsesrequest reaches the OpenAI-chat upstream with no Authorization header. Compact reads onlycompactProvider.apiKey; live and image selection also only recognizeprovider.apiKey, so credentialRef-only configuration cannot authenticate those paths.resolveModelsAuthToken, while ordinary adapters and sidecar selectors consumeprovider.apiKeydirectly and no request-time credentialRef resolution injects the resolved secret into the routed provider.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(provider-security): ChefVault clien..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
Bug Fixes