fix(cli): publish with a refused login refreshes or asks you to log in again - #4964
Conversation
…again The server now answers 401 to a rejected credential instead of publishing anonymously, so the CLI says so and drops the warning for the old fallback.
Edit accuracy: accurate 1556 (base branch 1556), smooth 1436 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…ted API key An expired access token with a refresh token is refreshed (the cloud commands already do) instead of being sent and refused. Only the requests that carry the credential map a 401; an environment API key is named, since logging in would not replace it.
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 5d6a6d1f.
What I checked
- Refresh before publish. The resolver marks a login refreshable only when it has expired and has a refresh token.
refreshTokenssaves the new tokens itself, including a rotated refresh token, so the refresh here doesn't use up the stored one. A refused refresh (REFRESH_FAILED) stops with the log-in-again message before any request goes out. Any other refresh error now ends the publish, the same way the cloud commands behave. - Which 401s get the message. Only the three requests that carry the credential (
/publish/upload,/publish/completeand the legacy/publish) are mapped. A 401 from the presigned storage upload keeps its own error. The upload endpoint's 404/405 fallback to the legacy path still runs before the 401 check. An expired login with no refresh token still sends the stale token, and the 401 then gets the login message. - Tests.
publishProject.test.tsandpublish.test.tspass 47/47 locally. I reverted two changes in turn. Removing the refresh makes 2 tests fail: "refreshes the login and publishes with the new token" and "asks for a new login, without sending anything, when the refresh is refused". Disabling theCredentialRejectedErrormapping makes 4 fail: the first-request, complete, legacy and environment-key tests.
Non-blocking. Deleting the "--update was ignored and a NEW url was created" warning is only safe once the server returns 401. Until that deploy, an expired login that can't be refreshed and uses --update gets a new anonymous URL with no warning. writeProjectLink then points the directory at that anonymous project, which is existing behaviour. The PR body already describes this window, so the fix is just to ship this right before or with the server change.
— Rames
terencecho
left a comment
There was a problem hiding this comment.
At 5d6a6d1f, the eager refresh and credentialed-401 mapping are well scoped (packages/cli/src/utils/publishProject.ts:161–189,708–732): the presigned storage PUT keeps its own error. I found two paths where the promised “expired login refreshes or asks you to log in again” behavior still fails.
Blocker — an expired login without a refresh token becomes anonymous before the server can reject it. packages/cli/src/auth/resolver.ts:97–105 discards that OAuth credential; tryResolveCredential() returns null, and packages/cli/src/utils/publishProject.ts:761–769 sends no authentication at all. A normal hyperframes publish --yes then creates an anonymous claim URL rather than asking for a new login; the planned server 401-on-rejected credential cannot fix a request with no credential. (Explicit --update/--space do fail at the command preflight, but plain publish does not.) Refresh tokens are optional in the stored/token-response shape (auth/oauth.ts:542–562). The prior review says this case sends the stale token; the resolver contradicts that specific claim. Please distinguish a known expired login from a genuinely unsigned-in user and test this path end to end.
Blocker — a credentialed HTTP 200 with claimed: false is still accepted and linked. packages/cli/src/utils/publishProject.ts:70–93,789–799 parses an anonymous response as success and writes its URL into the local project link whenever a credential was resolved. The current server repository's optional-owner resolver still returns None for a rejected bearer, its staged-upload endpoint does not authenticate, and /publish/complete can return that unclaimed result (experiment-framework/heygen/services/heygen_server/hyperframes/routes/render_routes.py:70–88,126–151, current master 6c76f68b; I have not verified the deployed server). This PR also removes the --update/--space fallback warning in packages/cli/src/commands/publish.ts:286–301; until server rejection actually ships, an attempted update can mint an anonymous project without that explicit warning. The new authenticated refresh test (publishProject.test.ts:1159–1174) even uses a successful stagedFetch() fixture whose response has a claim token and no claimed: true (:125–149). Please fail closed on an unclaimed response when credentials were supplied, and do not persist it as an owned stable link; pin the 200-anonymous case in a regression test. This guard also protects against server policy drift after rollout.
Important, not independently blocking: a locally unexpired but server-revoked token gets the re-login error despite a valid refresh token; the cloud client already force-refreshes and retries once on 401 (packages/cli/src/cloud/index.ts:18–65). Consider parity for publish, especially if uploads take long enough for a token to expire between preparation and completion.
I reviewed the exact-head source and current server-repository source; I did not run the CLI tests locally. All 11 required checks pass at this head, but the new 401 fixtures do not exercise either anonymous-success path.
Verdict: REQUEST CHANGES
Reasoning: Neither a missing refresh token nor a successful anonymous fallback is stopped by the new 401 handling, so publishing can still create an unclaimed project where the user expected an authenticated publish.
— Review by tai (pr-review)
… again A stored login past expiry with no refresh token now resolves as LOGIN_EXPIRED instead of "not signed in", so publish stops with the re-login message before sending anything. A signed-in publish that the server answers with an unclaimed project fails the same way and links nothing.
terencecho
left a comment
There was a problem hiding this comment.
At 866c9b5b, both blockers from my prior review are fixed: auth/resolver.ts:73–91 now preserves expired-login state instead of treating an OAuth login without a refresh token as anonymous, and utils/publishProject.ts:790–803 refuses an OAuth publish answered claimed: false before writing a project link. The new real-resolver and staged/direct unclaimed tests exercise those paths. The API-key unclaimed publish remains an intentional success.
Blocker — explicit --update / --space can still succeed while being ignored with a valid API key (packages/cli/src/commands/publish.ts:166–179,235–302). The new preflight only checks that some credential exists; an HEYGEN_API_KEY or saved API key passes. But this PR states API-key publishes are unowned: publishProjectArchive accepts their claimed: false response and does not link the project (utils/publishProject.ts:790–804), while the server's owner resolution requires a bearer login. With either explicit flag, the CLI now reports “Project published” and a generic Claim URL, without saying the requested existing-project update or team-space publish was ignored. The PR base had an explicit --update/--space was ignored and a NEW url was created warning in this branch (commands/publish.ts:292–301); this PR deletes it. This is reachable with a valid API key, so the planned server 401 for rejected credentials cannot fix it, and an environment API key can outrank a saved login. Please require an owner-capable login for those flags, or preserve an explicit ignored-flag warning on any unclaimed result, and pin both flags with a valid API key in a command-level test. The normal API-key claim-URL publish can stay unchanged.
All 11 active required checks passed at this head. I did not run local CLI tests. The prior concern about a locally fresh but server-revoked bearer not force-refreshing remains a nonblocking follow-up, not a new regression.
Verdict: REQUEST CHANGES
Reasoning: The two authentication blockers are resolved, but removing the warning leaves an explicit update/space request reporting success without performing that request when a valid API key is selected.
— Review by tai (pr-review)
terencecho
left a comment
There was a problem hiding this comment.
At fcfd1510, the stable valid-API-key case from my previous review is fixed: commands/publish.ts:169–190 now resolves the selected credential and rejects API-key --update / --space before building or uploading. The command tests cover environment-key versions of both flags, a saved-key --update, and successful OAuth --update. The earlier expired-login and OAuth claimed:false blockers remain fixed; ordinary API-key claim-URL publishing remains allowed.
Blocker — an OAuth-to-API-key expiry transition can still silently ignore explicit flags. The command checks OAuth at commands/publish.ts:170, then awaits proxy baking at :217–220. publishProjectArchive independently calls resolvePublishCredential() at utils/publishProject.ts:762. With stored OAuth lacking a refresh token and a saved valid API key, start when the OAuth token is just outside the resolver's 60-second expiry skew; let the bake cross that boundary. auth/resolver.ts:73–78,100–109 first selects OAuth, then falls back to the saved API key without any external credential change. The upload utility treats any credential as sufficient to send the requested project ID/space (publishProject.ts:766–770), but an API key cannot own that project; the unclaimed response is only rejected for OAuth (:790–796). commands/publish.ts:298–314 then prints “Project published” and a generic claim URL with no warning that --update or --space was ignored. This recreates the silent-flag outcome behind the new preflight and is reachable by time passing during the bake, not only a concurrent logout. Please enforce owner-capable credentials at the upload boundary, or pass/pin a suitable credential through the publish call and reject an unclaimed explicit-flag result; pin the expiry-to-key fallback with a command-level test for both flags.
Active checks at this head were still running when reviewed; I did not run local tests. I am not lifting the earlier changes-requested verdict on this head.
— Review by tai (pr-review)
… and --space are never dropped
|
Fixed at e6dd31c (c3c8b3a, e673b22, e6dd31c): New command test for both flags ( |
terencecho
left a comment
There was a problem hiding this comment.
At e6dd31c6, the prior silent explicit-flag blocker is fixed for the expiry-to-API-key transition: commands/publish.ts:172,238–244 passes the preflight-selected OAuth credential through proxy baking to the upload, and utils/publishProject.ts:181–192,772 refreshes that same credential rather than falling back to the saved API key. The earlier expired-login and OAuth claimed:false guards remain intact.
Blocker — the pinned credential can overwrite a newer login during refresh. Start publish --update or --space under OAuth login A while proxy baking awaits (commands/publish.ts:230–244). Log in as B from another CLI during the bake, then let A cross the expiry-skew boundary. resolvePublishCredential(checked) refreshes A (utils/publishProject.ts:186–192) and refreshTokens(A.refresh_token) persists the response (auth/oauth.ts:383–415) by rereading the current store and merging { ...existing.oauth, ...tokens } (auth/oauth.ts:610–631). That store is now B's. A refresh response can legitimately omit refresh_token (auth/oauth.ts:542–576,605–607): the durable result becomes A's access token paired with B's refresh token and B's user metadata. Even when A rotates its refresh token, this overwrites B's newer login. On the prior head, the publish path re-resolved credentials after baking and would have selected B instead; pinning A across the bake introduced the long-window stale refresh. Please protect refresh persistence when the stored login has changed (and fail the publish rather than mutating B's session), with a test that swaps logins while the bake is pending and exercises a refresh response without refresh_token.
All 10 active required checks passed at this head when checked, including the Windows rerun. I did not run local tests. I cannot lift the earlier changes-requested review while this durable auth-store regression remains.
— Review by tai (pr-review)
|
Fixed at 5ab36e9 (48e5e6a, plus 5ab36e9 so an unreadable credentials file during a refresh reports itself instead of a changed login), at the persistence boundary so every refresh caller is covered, not only publish. Tests:
With the guard removed, both fail. The store then ends up with A's new access token on B's refresh token, which is the state you described. |
terencecho
left a comment
There was a problem hiding this comment.
Approving 5ab36e94. My blocker from e6dd31c6 is fixed: a pinned-login refresh can no longer write A's tokens over a newer login B.
What I verified
persistOAuth(auth/oauth.ts:611–631) now takes the exchangedrefresh_tokenand throwsLOGIN_CHANGEDbefore writing unless the storedoauth.refresh_tokenstill equals it. That is a sound identity key: the only OAuth credential source is the store (auth/resolver.ts:68–74), a different login or re-login mints a different refresh token, and a logout mid-bake (no storedoauth) also mismatches rather than resurrecting the old login. A non-rotating refresh by the same login still matches and keeps its refresh token via the merge.- The case I flagged is covered end to end:
publish.test.tslogs in as B during the bake, expires A, refreshes with a response that omitsrefresh_token, and asserts B'soauthanduser.emailare untouched, only the token endpoint was called (no upload), exit code 1, and the "login changed" message.oauth.test.tspins the same at therefreshTokenslayer and keeps the refresh-preserve tests meaningful by seeding the stored login they now require. resolvePublishCredentialmapsLOGIN_CHANGEDto a publish-level error (publishProject.ts:198) so--update/--spacefail loudly instead of continuing under the wrong login; other refresh callers (cloud/auth.ts,client.tsretry) let it propagate, and an unreadable store now fails the refresh instead of being overwritten (tested). Fresh-login paths still overwrite the block cleanly.
Non-blocking: the check and the write are still separate steps (readStore then writeStore, no lock), so a login that lands inside that millisecond window could still be overwritten. It needs an interactive login to complete in that window, so I would not hold on it; an optimistic re-read before the rename would close it.
I read the code and tests but did not run them locally. At review time the 9 required checks present had passed; the lint/format check, Windows studio shard, and the edit-accuracy shards were still running, so this approval is on code merit, not on CI completion. It is not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
What
hyperframes publishwith a credential the server refuses now stops before anything is created and says what to do:HEYGEN_API_KEY/HYPERFRAMES_API_KEY: "HEYGEN_API_KEY was rejected. Fix or unset it, then publish again." (logging in would not help: the environment key outranks a login)A login that expired and has no refresh token is no longer treated as "not signed in": publish stops with the same re-login message and sends nothing, and
hyperframes auth statussays "Your HeyGen login expired" instead of "Not signed in". A publish with a login that the server still answers with an unowned (unclaimed) project fails the same way. Only an owned result is saved as this directory's link. API-key publishes are unchanged: the server never makes them owned.--updateand--spaceneed a login, because only an owned project can be updated in place or put in a team space. With an API key they now stop before uploading and say so ("--update requires a login; HEYGEN_API_KEY cannot own a project. Unset it and run 'hyperframes auth login'.", or "an API key cannot own a project" for a saved key), instead of publishing a new unowned project and ignoring the flag.publishnow looks up the credential once and uploads with that same credential. Before, the upload looked it up again after baking proxies, so a login that expired during the bake fell back to a saved API key and the flags were dropped after all. A login that expires during the bake is refreshed just before the upload, never swapped for another credential; a login that cannot be refreshed stops with the re-login message before anything is sent.A token refresh never writes over a different login. If you log in as someone else while a publish is running, the old login's refresh leaves the new login exactly as it is, and publish stops with "Your login changed during publish. Run publish again."
An expired login that has a refresh token is refreshed first, as the cloud commands already do, so the common case of a stale access token publishes normally instead of failing. The warning for the old behaviour ("Your login looks expired or invalid, so --update was ignored and a NEW url was created above") is gone.
Why
The publish API is changing so that a request carrying a credential it rejects gets HTTP 401 instead of silently publishing an anonymous project with a claim URL. Without this change the CLI would print the bare server message ("Unauthorized"), and it would send a stale access token it could have refreshed.
How
resolvePublishCredentialresolves the credential and refreshes it withrefreshIfNeededfromcloud/auth.ts(now exported, not copied). A refused refresh (REFRESH_FAILED) is the "log in again" case and stops before any request./publish/upload,/publish/complete, the legacy/publish) turn a 401 intoCredentialRejectedError;publishProjectArchivemaps it to the message for that credential. A 401 from the presigned storage upload carries no credential and keeps its own error./publish/upload) creates nothing, so a refused credential stops before the archive upload. The existing catch inpublish.tsprints the message under "Publish failed" and exits 1.auth/resolver.ts) throwsLOGIN_EXPIREDwhen the stored login is expired with no refresh token and there is no saved API key to fall back to;NOT_CONFIGUREDnow means only "never signed in".resolvePublishCredentialmapsLOGIN_EXPIREDlikeREFRESH_FAILED, and the--update/--spacepreflight uses it too.claimed: true) and drops a bearer it cannot verify, publishing anonymously (claimed: false). An API key (x-api-key) always publishes unowned. So a login publish whose result is unclaimed raisesCredentialRejectedErrorand goes through the same message mapping, andwriteProjectLinkruns only for a claimed result.publish.tsresolves the credential once, before the bake, and passes it topublishProjectArchive(new optionalcredential;nullpublishes anonymously, omitted resolves inside as before, whichfeedbackstill uses). The--update/--spacepreflight checks that same credential and accepts only a login (type === "oauth").resolvePublishCredential(checked)refreshes a passed login whose expiry has passed (the resolver'sisTokenExpired, same 60 s skew) and never resolves again.persistOAuth(auth/oauth.ts) takes the refresh token that was exchanged ({ refreshed }, replacing thepreserveMissingflag). If the stored login no longer holds that refresh token, it writes nothing and throwsLOGIN_CHANGED;resolvePublishCredentialmaps it to the message above. This applies to every refresh caller, not only publish. A refresh that cannot read the credentials file now reports that error instead of treating the file as empty.Rollout
Release this CLI before or with the API change (an older CLI shows "Unauthorized" on the 401). Before the API deploy, a token the server silently drops still creates an unowned project on the server, but the CLI now reports it as a failed publish with the re-login message instead of showing a claim URL or saving the link.
Test plan
New in
publishProject.test.ts:401 on the first request: re-login message after exactly one request, which carried the bearer.
401 on
/publish/completeand on the legacy multipart publish: same message.401 from the storage upload: the storage error, not the login message.
401 with
HEYGEN_API_KEY: names the variable.An expired, refreshable login is refreshed and the request carries the new token; a refused refresh asks for a new login and sends nothing.
A stored login that is expired with no refresh token, through the real resolver and a temp credentials file: re-login message, no request sent (
publishProject.test.ts), andLOGIN_EXPIREDfrom the resolver (resolver.test.ts).A login publish answered with an unclaimed project, staged and legacy: re-login message, no link written.
An API-key publish answered with an unclaimed project: succeeds with its claim token, no link written. Checking every credential, or linking on any credential, fails it.
publish.test.ts, through the real command and resolver:--updateand--spacewithHEYGEN_API_KEY, and--updatewith a saved API key, exit 1 with the login message and never call publish;--updatewith a login publishes. The three refusals fail with the previous "any credential" check. A command test for each flag runs the real upload code with the stored login expiring mid-publish next to a saved API key: every request carries the login and the command exits 1 without a claim URL. It fails if the upload resolves again or ifpublish.tsdoes not pass its credential. A passed login that expired after the check is refreshed and the upload carries the new token, with no second lookup (publishProject.test.ts); it fails without the expiry recompute. A passed login that expired with no refresh token stops before any request; it fails without that check. A command test swaps the stored login to another account during the bake and answers the refresh without a refresh token: the other login, its refresh token and its user stay untouched, only the token request is sent, exit 1. With the guard removed it fails, because the store ends up with the old access token on the new refresh token; theoauth.test.tsunit test fails the same way. A refresh against a corrupt credentials file reportsINVALID_STOREand leaves the file alone. Real CLI on Linux withHEYGEN_API_KEYset: both flags print the message and exit 1.src/auth,src/cloud,src/commands/auth, feedback,publishProject.test.ts,publish.test.ts: 369 pass, 3 runs in a row;tsc --noEmit, oxlint, oxfmt clean.The three new tests fail on the previous head (anonymous publish,
NOT_CONFIGURED); dropping theLOGIN_EXPIREDmapping fails the end-to-end test. Real CLI on Linux with an expired, no-refresh credentials file and the API pointed at a dead port: before,auth statussaid "Not signed in" andpublish --yestried an anonymous upload; after,auth statusprints "Your HeyGen login expired" andpublishprints the re-login message and exits 1, for plain publish and--update.src/auth,src/commands/auth,src/cloud,publishProject.test.ts,publish.test.ts: 355 pass, 3 runs in a row (with the feedback tests);tsc --noEmitclean; oxlint clean.The first test fails on main (
Unauthorized). Breaking the code three ways (no refresh, mapping the storage 401, dropping the API-key message) fails 2, 1 and 1 tests.publishProject.test.ts,publish.test.tsand the cloud tests: 152 pass, 3 runs in a row; CLItsc --noEmitclean; oxlint clean; comment ratchet at or under base.No visible change
CLI text output only; nothing under Studio or the player.
Size
Credential refresh reuse, two error types, two messages, one unclaimed-result check, one deleted branch, a login-only
--update/--spacecheck, one credential lookup per publish (refreshed, never replaced), a refresh that only writes to the login it refreshed, twenty-two tests.