[PR #65] [MERGED] fix(auth): surface .authExpired when the token endpoint answers 400 invalid_grant #68

New issue

Closed

opened 2026-10-26 22:34:37 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/65
Author: @mattmillerai
Created: 9/4/2026
Status: ✅ Merged
Merged: 9/11/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-10358-auth-expired-invalid-grant

📝 Commits (2)

be03e36 fix(auth): surface .authExpired when the token endpoint answers 400 invalid_grant
99eb122 fix(auth): harden the 400 error path against echoed and forged endpoint strings

📊 Changes

7 files changed (+586 additions, -22 deletions)

View changed files

📝 README.md (+1 -1)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift (+5 -3)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift (+201 -12)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift (+5 -3)
📝 Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift (+1 -1)
📝 Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift (+20 -0)
📝 Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift (+353 -2)

📄 Description

ELI-5

When your sign-in session dies for good, the server says so with a very specific
"400 invalid_grant" answer. The SDK didn't understand that answer, so it filed it
under "the network is flaky" — which told the user to check their Wi-Fi and never
offered them the sign-in button they actually needed. This teaches the SDK to read
that answer and say "your session ended" instead.

What changed

The token endpoint rejects a refresh token that is expired, revoked, reused, or
unknown with an RFC 6749 §5.2 400 {"error":"invalid_grant","error_description":…}.
HTTP 400 has no branch in Transport.checkStatus, so it fell to the default: arm
and reached the caller as .network(URLError(.badServerResponse)) — a permanently
dead session reported as a retryable connectivity problem, with the consuming app's
re-sign-in flow (which keys on .authExpired) unreachable.

OAuthTokenEndpoint.post now classifies a 400 for the refresh grant before
Transport.checkStatus sees it:

400 body	before	after
`{"error":"invalid_grant"}`	`.network`	`.authExpired`
`{"error":"invalid_request"}` and the other RFC codes	`.network`	`.unknown(OAuthTokenEndpointError)`
unparseable	`.network`	`.unknown(OAuthTokenEndpointError)`

Non-invalid_grant codes report a malformed request — a client implementation bug —
so they deliberately do not become .network, which would invite a retry that
can never succeed. The existing 401/403 → .authExpired remap is kept as defence in
depth for a proxy or CDN answering in front of the endpoint; its test comment now
says so, since 400 is what the production endpoint actually sends.

mapAuthInvalidToExpired is renamed isRefreshGrant, because it now gates both
mappings. Both call sites are updated; grep confirms none were missed.

Scope call: the authorization-code grant is deliberately untouched

OAuthTokenEndpoint.post is shared with OAuthExchanger. invalid_grant there means
the authorization code was rejected — a failed sign-in, with no session to expire —
so mapping it to .authExpired would send the app straight back into the sign-in it
just failed. Exchange keeps its existing behaviour, and a new test pins that so the
refresh mapping can't leak into it later.

Credential hygiene

OAuthTokenEndpointError carries only the endpoint's own error / error_description
fields, never the raw body. Both are scrubbed of the request's own secret values
(refresh_token, code, code_verifier, client_secret) and then length-clamped —
redaction runs before clamping, so a clamp can't split a secret and leave the prefix
behind. A test asserts a server echoing our refresh token back inside
error_description reaches the caller as <redacted>.

Release

ComfySDKInfo.version is bumped to 0.4.1 and the README install snippet follows it.
Two judgment calls there, both flagged rather than assumed:

The constant read 0.3.0 while 0.4.0 was already tagged and released, so this
also closes that drift rather than bumping 0.3.0 → 0.3.1.
The README snippet pinned from: "0.3.1". SemVer means that range already resolves
0.4.1, so this is a docs-freshness change, not a functional one.

How this was verified

swift build and swift test --no-parallel from the package root: 252 tests in 30
suites, 0 failures.

Red→green, so the new tests exercise the fix rather than restating their own premise:
with the new 400 arm short-circuited, exactly the six new refresh tests fail; restored,
all 252 pass.

The premise was falsified against the live production endpoint, not just against my
own mocks — two non-mutating probes with a token that belongs to no family:

POST https://cloud.comfy.org/oauth/token  grant_type=refresh_token&refresh_token=<bogus>
→ HTTP 400 {"error":"invalid_grant","error_description":"refresh token unknown"}

POST https://cloud.comfy.org/oauth/token  grant_type=refresh_token   (refresh_token omitted)
→ HTTP 400 {"error":"invalid_request","error_description":"refresh_token is required"}

That confirms both the status code and the exact snake_case body shape this change
decodes, including that error_description is optional (a test covers its absence).

New tests, all in the existing mock-URLProtocol pattern:

400 invalid_grant on the refresh POST → .authExpired, not .network
400 invalid_grant with no error_description → still .authExpired
proactive refresh (the near-expiry path, a different chain of catch-arms from the
401 retry) hitting 400 invalid_grant → .authExpired, with no API call attempted
400 invalid_request → .unknown, carrying code == "invalid_request"
400 with an unparseable body → .unknown, carrying code == nil
400 error_description echoing the refresh token → redacted before it reaches the caller
exchange 400 invalid_grant → not .authExpired (scoping guard)

Residual

Not fixed here, and worth acting on separately:

The tag and GitHub release are not cut. This PR only bumps the version constant.
A tag must point at a commit on the default branch, so cutting 0.4.1 is a
post-merge step — and the consuming app cannot pin the fix until it happens. Whoever
merges this needs to tag and release, or the version bump buys nothing.
The authorization-code grant still reports a 400 as .network. The scope call
above is about invalid_grant specifically, but the .network-for-a-400
misclassification the default: arm produces is the same defect on the exchange path
— a failed sign-in still tells the user to check their connection. It needs its own
decision about which ComfyError case a rejected authorization code should carry, so
it is not folded in here.
.unknown may not fully deliver the "no useless retries" intent downstream. The
consuming app's error presentation maps .unknown to a generic "try again" with a
retry action, so a non-invalid_grant 400 still offers the user a retry — just
without the false "check your connection" diagnosis. Getting the rest of the way is
an app-side copy/affordance change, outside this repo.
Transport.checkStatus's default: arm is unchanged. Every other unmodelled
status still becomes .network(URLError(.badServerResponse)). This change routes
around it for the refresh grant rather than fixing the general case, which has a much
wider blast radius across every endpoint.
Unexercised artifacts. The originating investigation ticket, the ticket this one
is the root cause of, and the referenced consuming-app PR live in systems this run
cannot write to, so none of their state was updated. OAuthTokenEndpointError is
internal, matching this repo's existing error-payload types — a consumer receiving
it inside .unknown(underlying:) can read localizedDescription but cannot type-cast
it. That is deliberate for now; promoting it to public API is a separate call.

Provenance

Authored by: agent-work loop
Verified: swift build: clean; swift test --no-parallel: 252 tests in 30 suites, 0 failures; red→green check with the new 400 arm disabled: 6 new tests fail, rest pass; 2 live non-mutating probes of https://cloud.comfy.org/oauth/token returning 400 invalid_grant and 400 invalid_request
Deviations: the tag/release the ticket asks for is a post-merge step and is not done here (Residual 1); the 400 classification is scoped to the refresh grant, leaving the authorization-code grant's 400 handling unchanged (Residual 2)

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/65 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/4/2026 **Status:** ✅ Merged **Merged:** 9/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-10358-auth-expired-invalid-grant` --- ### 📝 Commits (2) - [`be03e36`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/be03e36b3e42df358aeec9284980510f11b59938) fix(auth): surface .authExpired when the token endpoint answers 400 invalid_grant - [`99eb122`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/99eb12217970c6e60a77d68a4eb19b6b109aa148) fix(auth): harden the 400 error path against echoed and forged endpoint strings ### 📊 Changes **7 files changed** (+586 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+1 -1) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift` (+5 -3) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift` (+201 -12) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift` (+5 -3) 📝 `Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift` (+1 -1) 📝 `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift` (+20 -0) 📝 `Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift` (+353 -2) </details> ### 📄 Description ## ELI-5 When your sign-in session dies for good, the server says so with a very specific "400 invalid_grant" answer. The SDK didn't understand that answer, so it filed it under "the network is flaky" — which told the user to check their Wi-Fi and never offered them the sign-in button they actually needed. This teaches the SDK to read that answer and say "your session ended" instead. ## What changed The token endpoint rejects a refresh token that is expired, revoked, reused, or unknown with an RFC 6749 §5.2 `400 {"error":"invalid_grant","error_description":…}`. HTTP 400 has no branch in `Transport.checkStatus`, so it fell to the `default:` arm and reached the caller as `.network(URLError(.badServerResponse))` — a permanently dead session reported as a retryable connectivity problem, with the consuming app's re-sign-in flow (which keys on `.authExpired`) unreachable. `OAuthTokenEndpoint.post` now classifies a 400 for the **refresh grant** before `Transport.checkStatus` sees it: | 400 body | before | after | | --- | --- | --- | | `{"error":"invalid_grant"}` | `.network` | `.authExpired` | | `{"error":"invalid_request"}` and the other RFC codes | `.network` | `.unknown(OAuthTokenEndpointError)` | | unparseable | `.network` | `.unknown(OAuthTokenEndpointError)` | Non-`invalid_grant` codes report a malformed request — a client implementation bug — so they deliberately do **not** become `.network`, which would invite a retry that can never succeed. The existing 401/403 → `.authExpired` remap is kept as defence in depth for a proxy or CDN answering in front of the endpoint; its test comment now says so, since 400 is what the production endpoint actually sends. `mapAuthInvalidToExpired` is renamed `isRefreshGrant`, because it now gates both mappings. Both call sites are updated; `grep` confirms none were missed. ### Scope call: the authorization-code grant is deliberately untouched `OAuthTokenEndpoint.post` is shared with `OAuthExchanger`. `invalid_grant` there means the *authorization code* was rejected — a failed sign-in, with no session to expire — so mapping it to `.authExpired` would send the app straight back into the sign-in it just failed. Exchange keeps its existing behaviour, and a new test pins that so the refresh mapping can't leak into it later. ### Credential hygiene `OAuthTokenEndpointError` carries only the endpoint's own `error` / `error_description` fields, never the raw body. Both are scrubbed of the request's own secret values (`refresh_token`, `code`, `code_verifier`, `client_secret`) and then length-clamped — redaction runs *before* clamping, so a clamp can't split a secret and leave the prefix behind. A test asserts a server echoing our refresh token back inside `error_description` reaches the caller as `<redacted>`. ## Release `ComfySDKInfo.version` is bumped to `0.4.1` and the README install snippet follows it. Two judgment calls there, both flagged rather than assumed: - The constant read `0.3.0` while `0.4.0` was already tagged and released, so this also closes that drift rather than bumping `0.3.0` → `0.3.1`. - The README snippet pinned `from: "0.3.1"`. SemVer means that range already resolves `0.4.1`, so this is a docs-freshness change, not a functional one. ## How this was verified `swift build` and `swift test --no-parallel` from the package root: **252 tests in 30 suites, 0 failures**. Red→green, so the new tests exercise the fix rather than restating their own premise: with the new 400 arm short-circuited, exactly the six new refresh tests fail; restored, all 252 pass. **The premise was falsified against the live production endpoint, not just against my own mocks** — two non-mutating probes with a token that belongs to no family: ``` POST https://cloud.comfy.org/oauth/token grant_type=refresh_token&refresh_token=<bogus> → HTTP 400 {"error":"invalid_grant","error_description":"refresh token unknown"} POST https://cloud.comfy.org/oauth/token grant_type=refresh_token (refresh_token omitted) → HTTP 400 {"error":"invalid_request","error_description":"refresh_token is required"} ``` That confirms both the status code and the exact snake_case body shape this change decodes, including that `error_description` is optional (a test covers its absence). New tests, all in the existing mock-`URLProtocol` pattern: - `400 invalid_grant` on the refresh POST → `.authExpired`, not `.network` - `400 invalid_grant` with no `error_description` → still `.authExpired` - proactive refresh (the near-expiry path, a different chain of catch-arms from the 401 retry) hitting `400 invalid_grant` → `.authExpired`, with no API call attempted - `400 invalid_request` → `.unknown`, carrying `code == "invalid_request"` - `400` with an unparseable body → `.unknown`, carrying `code == nil` - `400 error_description` echoing the refresh token → redacted before it reaches the caller - exchange `400 invalid_grant` → **not** `.authExpired` (scoping guard) ## Residual Not fixed here, and worth acting on separately: 1. **The tag and GitHub release are not cut.** This PR only bumps the version constant. A tag must point at a commit on the default branch, so cutting `0.4.1` is a post-merge step — and the consuming app cannot pin the fix until it happens. Whoever merges this needs to tag and release, or the version bump buys nothing. 2. **The authorization-code grant still reports a 400 as `.network`.** The scope call above is about `invalid_grant` specifically, but the `.network`-for-a-400 misclassification the `default:` arm produces is the same defect on the exchange path — a failed sign-in still tells the user to check their connection. It needs its own decision about which `ComfyError` case a rejected authorization code should carry, so it is not folded in here. 3. **`.unknown` may not fully deliver the "no useless retries" intent downstream.** The consuming app's error presentation maps `.unknown` to a generic "try again" with a retry action, so a non-`invalid_grant` 400 still offers the user a retry — just without the false "check your connection" diagnosis. Getting the rest of the way is an app-side copy/affordance change, outside this repo. 4. **`Transport.checkStatus`'s `default:` arm is unchanged.** Every other unmodelled status still becomes `.network(URLError(.badServerResponse))`. This change routes around it for the refresh grant rather than fixing the general case, which has a much wider blast radius across every endpoint. 5. **Unexercised artifacts.** The originating investigation ticket, the ticket this one is the root cause of, and the referenced consuming-app PR live in systems this run cannot write to, so none of their state was updated. `OAuthTokenEndpointError` is `internal`, matching this repo's existing error-payload types — a consumer receiving it inside `.unknown(underlying:)` can read `localizedDescription` but cannot type-cast it. That is deliberate for now; promoting it to public API is a separate call. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build`: clean; `swift test --no-parallel`: 252 tests in 30 suites, 0 failures; red→green check with the new 400 arm disabled: 6 new tests fail, rest pass; 2 live non-mutating probes of `https://cloud.comfy.org/oauth/token` returning `400 invalid_grant` and `400 invalid_request` - **Deviations:** the tag/release the ticket asks for is a post-merge step and is not done here (Residual 1); the 400 classification is scoped to the refresh grant, leaving the authorization-code grant's 400 handling unchanged (Residual 2) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:37 +00:00