[PR #67] [MERGED] fix(auth): classify the authorization-code grant's 400 as .unknown, not .network #67

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/67
Author: @mattmillerai
Created: 9/11/2026
Status: ✅ Merged
Merged: 9/11/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-13493-exchange-400-unknown

📝 Commits (6)

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
c343c96 fix(auth): classify the authorization-code grant's 400 as .unknown, not .network
85a07ee merge: integrate the squash-merged 400-classification foundation from main
a5d2d15 fix(auth): stop short secrets corrupting the 400 code, and close the sanitize redaction gap
1d87360 fix(auth): match redaction past spliced separators, and floor it only in the RFC code

📊 Changes

4 files changed (+425 additions, -32 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift (+7 -4)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift (+134 -22)
📝 Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift (+249 -6)
📝 Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift (+35 -0)

📄 Description

Note on history: this branch was cut from the still-open #65, which squash-merged to main while the work was in progress. main was merged in afterwards so GitHub's three-dot diff shows only this PR's net change; the merge commit resolved three comment/code collisions in favour of this branch, whose content already contained #65's verbatim (git diff 99eb122 origin/main -- Sources/ Tests/ is empty). Base is main — this is not a stacked PR.

ELI-5

When you sign in, the app trades a short-lived code from the browser for real tokens. If that code has expired or was already used, the server says "no" with an HTTP 400. The SDK was filing that "no" under network problem — the same bucket as a dropped Wi-Fi connection — so anything that retries network errors would retry a code that can never work again. This makes the SDK call it what it is: a refusal, not a connection failure.

What changed

The HTTP 400 arm in OAuthTokenEndpoint.post was gated on isRefreshGrant, so the authorization-code grant never entered it and fell through to Transport.checkStatus, whose default: arm maps every unmodelled status to .network(URLError(.badServerResponse)). The gate is removed: both grants now enter the typed OAuthTokenEndpointError path, and the invalid_grant → .authExpired shortcut moves inside the classifier behind isRefreshGrant.

Result for the exchange grant: a 400 surfaces as .unknown(OAuthTokenEndpointError) carrying the endpoint's own scrubbed error / error_description — never .authExpired (the user had no session to expire, and that case drives a "your session ended, sign in again" sheet that would misdescribe a failed first sign-in and send them back into the sign-in they just failed), and never .network.

Refresh-grant behaviour is byte-identical: same classifier, same inputs, isRefreshGrant: true preserves every arm. Verified by the existing refresh-grant suite, which is unchanged and green.

Two comments that had gone stale were corrected rather than left contradicting the code: the post doc comment (which documented the exchange exclusion) and the call-site comment in OAuthExchanger, which said the exchange grant "leaves a 400 to Transport.checkStatus".

Chesterton's Fence

Removing the isRefreshGrant gate broadens an existing guard, so the history was read before touching it. That gate is fresh — it landed on main in #65, deliberately, to keep that PR to its stated scope, and its post doc comment said so explicitly. Two of #65's bot reviewers raised exactly this asymmetry on the same line and the reply on that thread deferred it as a follow-up rather than disputing it. So the broadening is the intended completion of that work, not a regression of it. The proof that old and new cannot disagree is structural: the only case that newly enters the arm is isRefreshGrant == false, and the only behaviour reachable there before was Transport.checkStatus's default: arm.

Tests

exchange400InvalidGrantIsNotAuthExpired caught is ComfyError {}, which confirmed the error was not .authExpired but pinned nothing about what it actually was — so the .network classification was untested in either direction. It now asserts the concrete case and the carried code and detail. Added alongside it: the never-.network regression, a non-invalid_grant code, an unparseable 400 body, and secret redaction over the exchange grant's own code / code_verifier (which the refresh grant never sends, so that path had no coverage).

Red→green was proven, not assumed: with OAuthTokenEndpoint.swift reverted to its main state and the new tests in place, all five fail; with the change, all five pass.

Provenance

Authored by: agent-work loop
Verified: swift build: clean; swift test --no-parallel: 261 tests in 30 suites, 0 failures (3 live-integration tests skipped as designed, gated on env vars). Falsification of the new tests: same suite with only OAuthTokenEndpoint.swift reverted to its main state → 5 failures, all of them the new/tightened assertions. On CI: Build & test, hygiene / public-repo-hygiene, Endpoint contract check and AGENTS.md integrity all pass.
Deviations: none against the stated acceptance criteria; see ## Residual for what was deliberately left unfixed.

Residual

Short-secret over-redaction in the shared redact helper — reachable on this grant now, not fixed here. redact is a plain substring replacement of the request's own secret values over the endpoint's error strings, so a short secret can strike characters out of the endpoint's own error code — a one-character code_verifier turns invalid_grant into in<redacted>alid_grant in the carried OAuthTokenEndpointError. This is pre-existing (it arrived with the helper in #65) and it is cosmetic rather than a leak — it errs toward redacting too much, and classification is unaffected because the invalid_grant match runs on the raw decoded value before scrubbing. It was not reachable on the exchange grant before this change and now is, which is why it is named here. Unreachable in normal use: the SDK derives its verifier from 32 random bytes (43 base64url characters) and the server's authorization code is opaque and long. It is reachable by a third-party caller of the public exchangeAuthorizationCode(_:codeVerifier:config:) entry point that supplies its own short verifier. Not fixed in this PR because the obvious fix — skipping secrets below some length — narrows a security control, which deserves its own change and its own review rather than riding along on an error-classification fix. This surfaced as a real red test here: the new fixtures use production-shaped 43-character values precisely so the suite pins real behaviour instead of an artifact of a one-letter fixture.

Artifact naming discrepancy, no work left. The source report located exchange400InvalidGrantIsNotAuthExpired in Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift; the test actually lives in Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift. Both files were exercised (the whole suite runs), and the test named in the report is the one tightened here — recording it so a reader who greps the named path and finds nothing does not conclude the work was missed.

hygiene / public-repo-hygiene — could not be run locally, confirmed on CI. That check loads its checker and allowlist from a pinned commit in a separate workflows repository by design, precisely so a PR cannot reach it from its own checkout, so nothing in a local worktree exercises it. It passed on this PR's first CI run, alongside Build & test, Endpoint contract check and AGENTS.md integrity. Recorded here because the local verification genuinely did not cover it.

Consumer-side copy is deliberately not touched. The downstream first-party consumer was checked out of band (private repo, so no detail is repeated here, as this repo is public): its sign-in path handles user-cancellation specially and folds every other error into a single message with no automatic retry, so nothing downstream branches on the case this change alters and there is no user-visible behaviour change to ship with it. Whether that copy should now distinguish a refused code is a separate product question, out of scope here.

No new public ComfyError case. A dedicated case for "the authorization code was rejected" would be a public-API addition and was deliberately not introduced; .unknown carrying OAuthTokenEndpointError is the landing spot. Worth revisiting if a consumer ever needs to branch on it, which none does today.

Negative-claim falsification: not applicable. This diff denies no capability — it adds no dead-end, no "not supported" path, and no new failure. The authorization-code exchange already failed on a 400; only the ComfyError case a consumer sees changes, and it changes from a less accurate classification to a more accurate one.

_{🔄 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/67 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/11/2026 **Status:** ✅ Merged **Merged:** 9/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-13493-exchange-400-unknown` --- ### 📝 Commits (6) - [`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 - [`c343c96`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/c343c969664e82ff2c815a9c128897e6220c8677) fix(auth): classify the authorization-code grant's 400 as .unknown, not .network - [`85a07ee`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/85a07eee64898851711dff306164705f6c929d03) merge: integrate the squash-merged 400-classification foundation from main - [`a5d2d15`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/a5d2d15feb592c6a77e558f7745958443c39fa54) fix(auth): stop short secrets corrupting the 400 code, and close the sanitize redaction gap - [`1d87360`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/1d87360a22319f2f304f29e6dc82df531b6d77ac) fix(auth): match redaction past spliced separators, and floor it only in the RFC code ### 📊 Changes **4 files changed** (+425 additions, -32 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift` (+7 -4) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift` (+134 -22) 📝 `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift` (+249 -6) 📝 `Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift` (+35 -0) </details> ### 📄 Description > **Note on history:** this branch was cut from the still-open #65, which squash-merged to `main` while the work was in progress. `main` was merged in afterwards so GitHub's three-dot diff shows only this PR's net change; the merge commit resolved three comment/code collisions in favour of this branch, whose content already contained #65's verbatim (`git diff 99eb122 origin/main -- Sources/ Tests/` is empty). Base is `main` — this is not a stacked PR. ## ELI-5 When you sign in, the app trades a short-lived code from the browser for real tokens. If that code has expired or was already used, the server says "no" with an HTTP 400. The SDK was filing that "no" under *network problem* — the same bucket as a dropped Wi-Fi connection — so anything that retries network errors would retry a code that can never work again. This makes the SDK call it what it is: a refusal, not a connection failure. ## What changed The HTTP 400 arm in `OAuthTokenEndpoint.post` was gated on `isRefreshGrant`, so the authorization-code grant never entered it and fell through to `Transport.checkStatus`, whose `default:` arm maps every unmodelled status to `.network(URLError(.badServerResponse))`. The gate is removed: both grants now enter the typed `OAuthTokenEndpointError` path, and the `invalid_grant → .authExpired` shortcut moves *inside* the classifier behind `isRefreshGrant`. Result for the exchange grant: a 400 surfaces as `.unknown(OAuthTokenEndpointError)` carrying the endpoint's own scrubbed `error` / `error_description` — never `.authExpired` (the user had no session to expire, and that case drives a "your session ended, sign in again" sheet that would misdescribe a failed first sign-in and send them back into the sign-in they just failed), and never `.network`. Refresh-grant behaviour is byte-identical: same classifier, same inputs, `isRefreshGrant: true` preserves every arm. Verified by the existing refresh-grant suite, which is unchanged and green. Two comments that had gone stale were corrected rather than left contradicting the code: the `post` doc comment (which documented the exchange exclusion) and the call-site comment in `OAuthExchanger`, which said the exchange grant "leaves a 400 to `Transport.checkStatus`". ## Chesterton's Fence Removing the `isRefreshGrant` gate broadens an existing guard, so the history was read before touching it. That gate is fresh — it landed on `main` in #65, deliberately, to keep that PR to its stated scope, and its `post` doc comment said so explicitly. Two of #65's bot reviewers raised exactly this asymmetry on the same line and the reply on that thread deferred it as a follow-up rather than disputing it. So the broadening is the intended completion of that work, not a regression of it. The proof that old and new cannot disagree is structural: the only case that newly enters the arm is `isRefreshGrant == false`, and the only behaviour reachable there before was `Transport.checkStatus`'s `default:` arm. ## Tests `exchange400InvalidGrantIsNotAuthExpired` caught `is ComfyError {}`, which confirmed the error was *not* `.authExpired` but pinned nothing about what it actually was — so the `.network` classification was untested in either direction. It now asserts the concrete case and the carried code and detail. Added alongside it: the never-`.network` regression, a non-`invalid_grant` code, an unparseable 400 body, and secret redaction over the exchange grant's own `code` / `code_verifier` (which the refresh grant never sends, so that path had no coverage). Red→green was proven, not assumed: with `OAuthTokenEndpoint.swift` reverted to its `main` state and the new tests in place, all five fail; with the change, all five pass. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build`: clean; `swift test --no-parallel`: 261 tests in 30 suites, 0 failures (3 live-integration tests skipped as designed, gated on env vars). Falsification of the new tests: same suite with only `OAuthTokenEndpoint.swift` reverted to its `main` state → 5 failures, all of them the new/tightened assertions. On CI: `Build & test`, `hygiene / public-repo-hygiene`, `Endpoint contract check` and `AGENTS.md integrity` all pass. - **Deviations:** none against the stated acceptance criteria; see `## Residual` for what was deliberately left unfixed. ## Residual **Short-secret over-redaction in the shared `redact` helper — reachable on this grant now, not fixed here.** `redact` is a plain substring replacement of the request's own secret values over the endpoint's error strings, so a *short* secret can strike characters out of the endpoint's own `error` code — a one-character `code_verifier` turns `invalid_grant` into `in<redacted>alid_grant` in the carried `OAuthTokenEndpointError`. This is pre-existing (it arrived with the helper in #65) and it is cosmetic rather than a leak — it errs toward redacting too much, and classification is unaffected because the `invalid_grant` match runs on the raw decoded value before scrubbing. It was not reachable on the exchange grant before this change and now is, which is why it is named here. Unreachable in normal use: the SDK derives its verifier from 32 random bytes (43 base64url characters) and the server's authorization code is opaque and long. It *is* reachable by a third-party caller of the public `exchangeAuthorizationCode(_:codeVerifier:config:)` entry point that supplies its own short verifier. Not fixed in this PR because the obvious fix — skipping secrets below some length — narrows a security control, which deserves its own change and its own review rather than riding along on an error-classification fix. This surfaced as a real red test here: the new fixtures use production-shaped 43-character values precisely so the suite pins real behaviour instead of an artifact of a one-letter fixture. **Artifact naming discrepancy, no work left.** The source report located `exchange400InvalidGrantIsNotAuthExpired` in `Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift`; the test actually lives in `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift`. Both files were exercised (the whole suite runs), and the test named in the report is the one tightened here — recording it so a reader who greps the named path and finds nothing does not conclude the work was missed. **`hygiene / public-repo-hygiene` — could not be run locally, confirmed on CI.** That check loads its checker and allowlist from a pinned commit in a separate workflows repository by design, precisely so a PR cannot reach it from its own checkout, so nothing in a local worktree exercises it. It passed on this PR's first CI run, alongside `Build & test`, `Endpoint contract check` and `AGENTS.md integrity`. Recorded here because the local verification genuinely did not cover it. **Consumer-side copy is deliberately not touched.** The downstream first-party consumer was checked out of band (private repo, so no detail is repeated here, as this repo is public): its sign-in path handles user-cancellation specially and folds every other error into a single message with no automatic retry, so nothing downstream branches on the case this change alters and there is no user-visible behaviour change to ship with it. Whether that copy should now distinguish a refused code is a separate product question, out of scope here. **No new public `ComfyError` case.** A dedicated case for "the authorization code was rejected" would be a public-API addition and was deliberately not introduced; `.unknown` carrying `OAuthTokenEndpointError` is the landing spot. Worth revisiting if a consumer ever needs to branch on it, which none does today. **Negative-claim falsification: not applicable.** This diff denies no capability — it adds no dead-end, no "not supported" path, and no new failure. The authorization-code exchange already failed on a 400; only the `ComfyError` case a consumer sees changes, and it changes from a less accurate classification to a more accurate one. --- <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