fix(deploy): stop failing deploys on a probe an API key cannot pass - #3527
Conversation
Deploying to a protected environment from an API-key-authenticated CLI always failed on the last step, reported as `unknown-error`, after the deployment had already been committed and verified. The readiness probe sent the stored credential as an `authToken` cookie. The gate reading that cookie resolves it by decoding a JWT and reading `userId` from the verified payload (src/proxy/proxy-access-control.ts); it has no API-key branch. An opaque `vf_` key was therefore rejected exactly like no credential at all, so the probe could never succeed and sending the key leaked it for no benefit. The probe now checks whether the credential is JWT-shaped. When it is not, it probes unauthenticated and accepts the sign-in redirect as ready. That redirect still proves routing resolves and the proxy is serving the environment, which is all this step can establish without a session -- and `create-deployment` and `verify-deployment` have both already completed by the time it runs. The throws in the readiness loop were bare `Error`s, so the boundary wrapped every diagnosed condition in the catch-all `unknown-error` slug with "Check logs for more details". They now carry DEPLOYMENT_ERROR. Verified against the reported failure: the same URL and API key that produced `unknown-error` now pass the probe, and fail identically on the unfixed code. Closes veryfront/veryfront-issue-inbox#444
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnvironment readiness probes now distinguish JWT-shaped session credentials from API keys. API keys are withheld from protected gates, authentication challenges can indicate readiness without a session, and failures use structured ChangesDeployment readiness probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cli/shared/deployment/deploy-project.ts`:
- Around line 962-985: Remove probe.url from all user-facing
DEPLOYMENT_ERROR.detail messages in the environment readiness and
response-status branches, replacing it with generic error text. Preserve only
approved redacted diagnostics in the non-user-facing context path, and add
coverage verifying a custom hostname never appears in detail.
- Around line 842-845: Update isSessionCredential to decode the first two
base64url segments, parse the payload as JSON, and return true only when it
contains a userId; otherwise reject the credential, including opaque dotted
values. Add or update a focused test verifying authenticate withholds a dotted
opaque credential from the authToken cookie.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f59d86ec-20ef-4eab-8bcb-eb87bf14340b
📒 Files selected for processing (2)
cli/shared/deployment/deploy-project.test.tscli/shared/deployment/deploy-project.ts
…ntial Counting three non-empty segments let an opaque credential containing dots through, so `opaque.segment.value` would still be sent in the authToken cookie. The check now decodes the header and payload as base64url JSON and requires a string `userId` in the payload -- which is exactly what the gate reads before testing project membership. Anything that cannot resolve to a member is withheld rather than presented and rejected. Verified against real credentials: the Google session JWT is still sent, and the API key is still withheld.
Every authentication challenge was reported as "redirected to sign-in", but a challenge is also a 401 or a 403. A public environment answering 403 sent the operator looking for a redirect that never happened. The message now names the status when there was no redirect, and the error context carries it. Also records why the probe accepts the whole challenge rather than only the redirect: it is the allowance the protected custom-domain probe already makes, and narrowing it would buy nothing, because the gate answers before the app does.
|
Two follow-up commits since the review, both from issues raised on this PR. 321f27b — tightens the credential check. Counting three non-empty segments let cafd912 — the error message named the wrong thing. Every authentication challenge was reported as "redirected to sign-in", but a challenge is also a 401 or a 403, and a public environment answering 403 sent the operator looking for a redirect that never happened. The message now names the status when there was no redirect, and the context carries it. One test. That commit also records why the probe accepts the whole challenge and not only the redirect, which the description had understated. It is the allowance the protected custom-domain probe already makes, and narrowing it would buy nothing: the gate answers before the app does, so with no session this probe is blind to application health whichever challenge it accepts. Left alone on purpose: a readiness failure still fails a deploy that has already been committed and verified. Fixed here only for the case that could never have succeeded. Turning readiness into a warning touches every credential type and every failure mode, so it belongs in its own change. Description updated to match. |
1 participant
Problem
Deploying to a protected environment from an API-key-authenticated CLI always failed on the last step — reported as
unknown-error, after the deployment had already been committed and verified.The suggested remedy is a dead end:
veryfront loginis what produced the API key.Root cause
The readiness probe sent the stored credential as an
authTokencookie (deploy-project.ts:908). The gate reading that cookie resolves it by decoding a JWT and readinguserIdfrom the verified payload (src/proxy/proxy-access-control.ts:104-166) — RS256 or HS256 only, no API-key branch. An opaquevf_…key was rejected exactly like no credential at all.Verified against production — all three return the same sign-in redirect:
So the probe could never succeed, and sending the key leaked it for no benefit.
The probe is also the last step.
create-deployment(line 1193) andverify-deployment(line 1203) both complete before it runs — the deploy had already landed when the CLI reported failure.Separately, every throw in the readiness loop was a bare
Error, so the boundary wrapped these fully-diagnosed conditions in the catch-allunknown-errorslug with "Check logs for more details".Fix
1. Don't present a credential the gate cannot accept.
isSessionCredentialdecodes the header and payload as base64url JSON and requires a stringuserIdin the payload — the same value the gate reads before testing project membership. Anything else probes unauthenticated.The check is deliberately fail-closed: a credential the CLI cannot positively recognise is withheld, never sent. Counting dot-separated segments would not have been enough, because an opaque
a.b.ccredential would still have been presented — and presenting it is the leak this check exists to prevent.2. Accept the gate's challenge as ready when there is no session. A challenge — sign-in redirect, 401, or 403 — still proves routing resolves and the proxy is serving the environment, which is all this step can establish without a session. This reuses the existing
acceptAuthenticationChallengepath already used for protected custom domains; no new semantics.Accepting the whole challenge rather than only the redirect is the same allowance that path already makes. Narrowing it would buy nothing here either: the gate answers before the app does, so for a protected environment with no session this probe is blind to application health whichever challenge it accepts.
3. Classify the errors. The throws in the readiness loop now carry
DEPLOYMENT_ERRORinstead of a bareError, so the slug, suggestion, and docs URL match the actual condition.4. Name the challenge that actually arrived. Every challenge was reported as "redirected to sign-in", including a 401 or 403. A public environment answering 403 sent the operator looking for a redirect that never happened. The message now names the status when there was no redirect, and the error context carries it.
A JWT session that is genuinely rejected still fails the deploy, with the same message — that path is unchanged, and there "Run veryfront login" is correct advice.
Verification
End-to-end, against the reported failure — same URL, same API key, calling the real
waitForEnvironmentReady:Checked against real credentials: the session JWT is still sent, the API key is still withheld.
Tests — 6 added. Each was confirmed to fail on the unfixed code and pass with the fix:
does not send an API key to the protected environment gatedoes not send an opaque credential that merely contains dotsdoes not send a JWT-shaped credential whose payload carries no userIdtreats a sign-in redirect as ready when the credential is an API keyclassifies a rejected session credential as a deployment errornames the status when a challenge was not a sign-in redirectThe first of these also holds the invariant against
isApiKeyTokenincli/auth/login.ts: it goes red the moment an API key becomes presentable to the gate again.The existing fixture token
test-tokenwas not JWT-shaped, so it would have silently taken the new unauthenticated path and stopped covering the authenticated one. It is now a JWT-shapedsessionToken, keeping those assertions meaningful.cli/shared/deployment/— 12 files, 87 steps, all passing.deno check,deno lint,deno fmt --checkclean.Scope
This does not make API keys work against protected environments — whether an API key should grant browser access to a protected environment is a product decision with real security implications, and belongs in the proxy, not the CLI. This PR stops a probe from failing a deploy that succeeded.
It also does not change the broader shape of the last step: a readiness failure still fails a deploy that has already been committed and verified. That is fixed here only for the case that could never have succeeded. Making readiness a warning rather than a failure affects every credential type and every failure mode, so it belongs in its own change.
Closes veryfront/veryfront-issue-inbox#444
Summary by CodeRabbit