[PR #70] [MERGED] feat(auth)!: add ComfyError.authCodeRejected for a refused authorization-code exchange #73

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-13548-auth-code-rejected

📝 Commits (9)

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
d28081d feat(auth)!: surface a rejected authorization code as ComfyError.authCodeRejected
6ee634b merge: integrate the squash-merged 400-classification foundation from main
6f18d8e fix(auth): close the escape-case redaction gap and the normalize asymmetry
5656dbb docs(auth): widen the non-invalid_grant recovery note beyond config mismatch

📊 Changes

11 files changed (+467 additions, -109 deletions)

View changed files

📝 README.md (+1 -1)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift (+11 -7)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift (+145 -42)
📝 Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift (+1 -1)
📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+2 -1)
📝 Sources/ComfySwiftSDK/Internal/SDKLog.swift (+1 -0)
📝 Sources/ComfySwiftSDK/Public/ComfyAuthSignIn.swift (+7 -1)
📝 Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift (+9 -1)
📝 Sources/ComfySwiftSDK/Public/ComfyError.swift (+34 -0)
📝 Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift (+222 -55)
📝 Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift (+34 -0)

📄 Description

ELI-5

When you sign in, the app trades a short-lived authorization code for real tokens. If the server says "no" to that trade, the SDK used to report it as .unknown(underlying:) wrapping an internal type — so an app could see only "something unknown went wrong" for the single auth failure it most needs to react to, with no way to read the reason. This PR gives that refusal its own public case, ComfyError.authCodeRejected(code:detail:), so an app can branch on it with the compiler's help and send the user back to sign in for a fresh code.

What changed

ComfyError gains case authCodeRejected(code: String?, detail: String?). Every HTTP 400 on the authorization_code grant now throws it — parseable body or not. The 400 on this grant is the refusal signal, and there is no consumer-meaningful difference between "refused with a body we could not parse" and "refused", so the unparseable case carries (nil, nil) rather than landing somewhere else. It is deliberately not .authExpired (a failed sign-in has no session to expire, and that case drives a "your session ended" sheet that misdescribes what happened) and deliberately not .network (which would invite retrying a code no retry can redeem).

code is normalized — trimmed and lowercased — so a consumer can compare it with == instead of re-implementing the case/whitespace tolerance the SDK already applies to the refresh grant's own invalid_grant match. The same normalization now runs on OAuthTokenEndpointError.code, through one shared private helper used by the refresh-grant match and both payloads, so what the SDK branches on and what it hands the consumer agree.

The normalization runs after the existing scrub (redact → sanitize → clamp), not before. redact matches the request's own secrets, so lowercasing first would stop it matching a mixed-case secret the server echoed back in error — and that credential would land in the public code of an error consumers log. A new test pins the ordering; see the falsification note below.

The refresh grant's classification is untouched: invalid_grant is still .authExpired, every other refresh 400 is still .unknown(OAuthTokenEndpointError), and that struct is now reached only by those. Its doc comment says so now.

Also: the two internal exhaustive switches over ComfyError gain an arm (PollingFallback.isTransient → false, listed with the other auth cases, because a refused grant is never transient; SDKLog.comfyErrorCaseName → "ComfyError.authCodeRejected"), the public - Throws: docs on ComfyCloudClient.exchangeAuthorizationCode and ComfyAuth.signIn name the new case, and ComfySDKInfo.version goes 0.4.1 → 0.5.0.

Source-breaking, and the tag is a manual post-merge step

(1) This is a source-breaking addition for consumers with exhaustive switch statements over ComfyError — a new enum case makes those switches non-exhaustive. Hence the minor bump to 0.5.0 rather than a patch: the consuming app pins upToNextMinorVersion from 0.4.0.

(2) main also carries an unreleased .router case (from the merged router error-taxonomy PR #66), which requires the same minor bump. So 0.5.0 covers both, and the tag must be cut manually after this merges — it is not created by merging. The follow-up work in the consuming iOS app cannot start until that tag exists. This mirrors the same residual left by PR #65.

A note on the history: this PR was cut while #67 was still open

This branch started stacked on #67's head. #67 squash-merged mid-flight, so its commits here had no ancestor on main and the three-dot diff double-counted the foundation. The merge commit resolves that without a force-push — and it turned out to matter for more than diff hygiene: #67 gained substantial review-driven work after the commit this branch was cut from (redaction that ignores separators spliced through an echoed credential, and a length floor narrowed to the RFC error code alone rather than applying to error_description too). Those are taken from main verbatim; this change is re-applied on top of them, so code is normalized after scrubbing the floor-filtered secret set, and the two tests #67 added for that behaviour are converted to the new case with their assertions intact rather than dropped. The net diff against main is now 10 files — this change only.

Tests

Existing exchange-path assertions were converted from #require(underlying as? OAuthTokenEndpointError) to matching .authCodeRejected — every redaction / sanitize / clamp assertion is preserved byte-for-byte, including the two that arrived with #67's later review round; only the case being matched changed. The two scoping guards ("…not .authExpired", "…never .network") are retargeted to the new case and still pin both directions. Refresh-grant tests are untouched and green.

Three new tests: a re-cased, padded code (" INVALID_GRANT \n") normalizes to invalid_grant with the description intact; an unparseable 400 body yields .authCodeRejected(nil, nil); and a secret whose casing differs from the code text is still redacted, which guards the scrub-then-normalize order.

That ordering test was falsified, not just written. I flipped the production order to scrub(normalizeCode(dto.error), …) and re-ran it — twice, before and after the merge, because the redaction internals changed underneath it. Both times it fails, and the failure message shows the authorization code leaking verbatim into the public code ("test-code-not-a-real-authorization-codeaaaa"). The original order was restored each time and the tree verified clean. So the test proves the ordering rather than asserting its own premise.

One thing worth knowing for anyone running the suite locally against this branch: inserting an enum case mid-ComfyError shifts the case tags, and an incremental build can leave the ComfyAuthKit target compiled against the old ones — which shows up as an unrelated-looking failure where catch ComfyError.authCancelled does not match an error that prints as .authCancelled. rm -rf .build clears it, and it passes on a clean build, which is what CI does. It is a build-cache artifact, not a behaviour change.

Negative-claim falsification

The capability-denial trigger does not fire here, and the reasoning is not "it looked fine": this diff adds no "not supported" / "unavailable" / "STOP" string, no new throw or deny dead-end, and no test flipped to assert a dead-end. The exchange 400 path already threw before this change — it threw .unknown. The user-facing outcome is strictly more actionable, not less: the same failure now carries a case a consumer can branch on and a documented recovery (restart sign-in for a fresh code). Nothing that previously succeeded now fails.

Judgment calls

Bumped the README install pin to from: "0.5.0" alongside ComfySDKInfo.version. That matches how the 0.4.1 bump was done — both in one PR, before the tag existed. The consequence is named under Residual.
Fixed one adjacent inaccurate comment. The existing comment above the 400 arm described this failure as "a user who let the authorization code expire in an open browser". That is wrong — the code's TTL is ~60s and it is minted at the redirect, not when the tab opens. Leaving that contradiction one screen above the new doc comment, which states the correct lifetime, would have been worse. Comment-only.
The refresh grant's .unknown payload gains the normalization too (its code is now trimmed/lowercased). This is a deliberate, requested unification so both paths agree, and it is the one refresh-side observable change in the diff: the case mapping is identical, and only a proxy that re-cased the value (e.g. INVALID_REQUEST) would see a different code string in a diagnostic. No refresh test changes behaviour; the existing ones assert already-lowercase codes.

Residual

The 0.5.0 git tag is not created by this PR and must be cut manually after merge. Nothing in the repo automates it. Until it exists: (a) the README install snippet this PR updates — from: "0.5.0" — resolves to no published version, and (b) the follow-up work in the consuming iOS app, which needs the new case, is blocked. This is the same unfinished tag step left behind by PR #65 and is believed to be tracked already; it is restated here because a reader of this PR alone would not know it is outstanding.
No live token endpoint was exercised. All 400-classification behaviour is proven against URLProtocol stubs. The repo's one live check (OAuthExchangeIntegrationTests) is gated behind RUN_OAUTH_INTEGRATION=1 and a seeded OAuth client this environment does not have, so it stayed skipped. The claim that the production endpoint answers a refused code with an RFC 6749 §5.2 400 is inherited from #65/#67, not re-measured here.
The originating investigation and the parent epic were not read. They were referenced to me by name only, with no accessible content, so their acceptance detail is unexercised — this PR implements the specification as handed to me. The consuming-app follow-up lives in a different repository and is out of scope here.
The .router case is not exercised by this change beyond compilation. Both new switch arms sit alongside it and the full suite is green, but nothing here tests router behaviour; that remains PR #66's coverage.

Provenance

Authored by: agent-work loop
Verified: swift build — clean; swift test --no-parallel after rm -rf .build — 292 tests in 31 suites, 0 failures (post-merge with the current main); pre-merge run on the stacked branch — 265 tests, 0 failures; scrub-then-normalize ordering falsified twice by flipping the production order and observing the credential leak, restored and the tree re-verified clean each time.
Deviations: none against the specification as given. Two additions beyond the literal list, both named under Judgment calls: the README version pin, and one adjacent factually-wrong comment corrected.

_{🔄 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/70 **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-13548-auth-code-rejected` --- ### 📝 Commits (9) - [`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 - [`d28081d`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/d28081d26ee3c9f0e6796d4aed2b1dab09261510) feat(auth)!: surface a rejected authorization code as ComfyError.authCodeRejected - [`6ee634b`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/6ee634b8d732d8ab0a2e2e49cff24aedcaa94ab4) merge: integrate the squash-merged 400-classification foundation from main - [`6f18d8e`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/6f18d8eac4b1c97593c7f2fd09060dc4366fd4af) fix(auth): close the escape-case redaction gap and the normalize asymmetry - [`5656dbb`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/5656dbbf8c47c4dbf7a3041a9f148ce983f4f822) docs(auth): widen the non-invalid_grant recovery note beyond config mismatch ### 📊 Changes **11 files changed** (+467 additions, -109 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+1 -1) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift` (+11 -7) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift` (+145 -42) 📝 `Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift` (+1 -1) 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+2 -1) 📝 `Sources/ComfySwiftSDK/Internal/SDKLog.swift` (+1 -0) 📝 `Sources/ComfySwiftSDK/Public/ComfyAuthSignIn.swift` (+7 -1) 📝 `Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift` (+9 -1) 📝 `Sources/ComfySwiftSDK/Public/ComfyError.swift` (+34 -0) 📝 `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift` (+222 -55) 📝 `Tests/ComfySwiftSDKTests/RefreshOn401Tests.swift` (+34 -0) </details> ### 📄 Description ## ELI-5 When you sign in, the app trades a short-lived authorization code for real tokens. If the server says "no" to that trade, the SDK used to report it as `.unknown(underlying:)` wrapping an **internal** type — so an app could see only "something unknown went wrong" for the single auth failure it most needs to react to, with no way to read the reason. This PR gives that refusal its own public case, `ComfyError.authCodeRejected(code:detail:)`, so an app can branch on it with the compiler's help and send the user back to sign in for a fresh code. ## What changed `ComfyError` gains `case authCodeRejected(code: String?, detail: String?)`. Every HTTP 400 on the `authorization_code` grant now throws it — **parseable body or not**. The 400 on this grant *is* the refusal signal, and there is no consumer-meaningful difference between "refused with a body we could not parse" and "refused", so the unparseable case carries `(nil, nil)` rather than landing somewhere else. It is deliberately not `.authExpired` (a failed sign-in has no session to expire, and that case drives a "your session ended" sheet that misdescribes what happened) and deliberately not `.network` (which would invite retrying a code no retry can redeem). `code` is normalized — trimmed and lowercased — so a consumer can compare it with `==` instead of re-implementing the case/whitespace tolerance the SDK already applies to the refresh grant's own `invalid_grant` match. The same normalization now runs on `OAuthTokenEndpointError.code`, through one shared private helper used by the refresh-grant match and both payloads, so what the SDK branches on and what it hands the consumer agree. **The normalization runs after the existing scrub (redact → sanitize → clamp), not before.** `redact` matches the request's own secrets, so lowercasing first would stop it matching a mixed-case secret the server echoed back in `error` — and that credential would land in the public `code` of an error consumers log. A new test pins the ordering; see the falsification note below. The refresh grant's classification is untouched: `invalid_grant` is still `.authExpired`, every other refresh 400 is still `.unknown(OAuthTokenEndpointError)`, and that struct is now reached only by those. Its doc comment says so now. Also: the two internal exhaustive switches over `ComfyError` gain an arm (`PollingFallback.isTransient` → `false`, listed with the other auth cases, because a refused grant is never transient; `SDKLog.comfyErrorCaseName` → `"ComfyError.authCodeRejected"`), the public `- Throws:` docs on `ComfyCloudClient.exchangeAuthorizationCode` and `ComfyAuth.signIn` name the new case, and `ComfySDKInfo.version` goes `0.4.1` → `0.5.0`. ## Source-breaking, and the tag is a manual post-merge step **(1) This is a source-breaking addition for consumers with exhaustive `switch` statements over `ComfyError`** — a new enum case makes those switches non-exhaustive. Hence the **minor** bump to `0.5.0` rather than a patch: the consuming app pins `upToNextMinorVersion` from `0.4.0`. **(2) `main` also carries an unreleased `.router` case** (from the merged router error-taxonomy PR #66), which requires the same minor bump. So `0.5.0` covers both, and the tag must be **cut manually after this merges** — it is not created by merging. The follow-up work in the consuming iOS app cannot start until that tag exists. This mirrors the same residual left by PR #65. ## A note on the history: this PR was cut while #67 was still open This branch started stacked on #67's head. #67 squash-merged mid-flight, so its commits here had no ancestor on `main` and the three-dot diff double-counted the foundation. The merge commit resolves that without a force-push — and it turned out to matter for more than diff hygiene: **#67 gained substantial review-driven work after the commit this branch was cut from** (redaction that ignores separators spliced through an echoed credential, and a length floor narrowed to the RFC `error` code alone rather than applying to `error_description` too). Those are taken from `main` verbatim; this change is re-applied on top of them, so `code` is normalized after scrubbing the floor-filtered secret set, and the two tests #67 added for that behaviour are converted to the new case with their assertions intact rather than dropped. The net diff against `main` is now 10 files — this change only. ## Tests Existing exchange-path assertions were converted from `#require(underlying as? OAuthTokenEndpointError)` to matching `.authCodeRejected` — **every redaction / sanitize / clamp assertion is preserved byte-for-byte**, including the two that arrived with #67's later review round; only the case being matched changed. The two scoping guards ("…not `.authExpired`", "…never `.network`") are retargeted to the new case and still pin both directions. Refresh-grant tests are untouched and green. Three new tests: a re-cased, padded code (`" INVALID_GRANT \n"`) normalizes to `invalid_grant` with the description intact; an unparseable 400 body yields `.authCodeRejected(nil, nil)`; and a secret whose casing differs from the code text is still redacted, which guards the scrub-then-normalize order. **That ordering test was falsified, not just written.** I flipped the production order to `scrub(normalizeCode(dto.error), …)` and re-ran it — twice, before and after the merge, because the redaction internals changed underneath it. Both times it fails, and the failure message shows the authorization code leaking verbatim into the public `code` (`"test-code-not-a-real-authorization-codeaaaa"`). The original order was restored each time and the tree verified clean. So the test proves the ordering rather than asserting its own premise. One thing worth knowing for anyone running the suite locally against this branch: inserting an enum case mid-`ComfyError` shifts the case tags, and an **incremental** build can leave the `ComfyAuthKit` target compiled against the old ones — which shows up as an unrelated-looking failure where `catch ComfyError.authCancelled` does not match an error that prints as `.authCancelled`. `rm -rf .build` clears it, and it passes on a clean build, which is what CI does. It is a build-cache artifact, not a behaviour change. ## Negative-claim falsification The capability-denial trigger does not fire here, and the reasoning is not "it looked fine": this diff adds **no** "not supported" / "unavailable" / "STOP" string, **no** new throw or deny dead-end, and **no** test flipped to assert a dead-end. The exchange 400 path already threw before this change — it threw `.unknown`. The user-facing outcome is strictly *more* actionable, not less: the same failure now carries a case a consumer can branch on and a documented recovery (restart sign-in for a fresh code). Nothing that previously succeeded now fails. ## Judgment calls - **Bumped the README install pin to `from: "0.5.0"` alongside `ComfySDKInfo.version`.** That matches how the `0.4.1` bump was done — both in one PR, before the tag existed. The consequence is named under Residual. - **Fixed one adjacent inaccurate comment.** The existing comment above the 400 arm described this failure as "a user who let the authorization code expire in an open browser". That is wrong — the code's TTL is ~60s and it is minted at the redirect, not when the tab opens. Leaving that contradiction one screen above the new doc comment, which states the correct lifetime, would have been worse. Comment-only. - **The refresh grant's `.unknown` payload gains the normalization too** (its `code` is now trimmed/lowercased). This is a deliberate, requested unification so both paths agree, and it is the one refresh-side observable change in the diff: the case mapping is identical, and only a proxy that re-cased the value (e.g. `INVALID_REQUEST`) would see a different `code` string in a diagnostic. No refresh test changes behaviour; the existing ones assert already-lowercase codes. ## Residual - **The `0.5.0` git tag is not created by this PR and must be cut manually after merge.** Nothing in the repo automates it. Until it exists: (a) the README install snippet this PR updates — `from: "0.5.0"` — resolves to no published version, and (b) the follow-up work in the consuming iOS app, which needs the new case, is blocked. This is the same unfinished tag step left behind by PR #65 and is believed to be tracked already; it is restated here because a reader of this PR alone would not know it is outstanding. - **No live token endpoint was exercised.** All 400-classification behaviour is proven against `URLProtocol` stubs. The repo's one live check (`OAuthExchangeIntegrationTests`) is gated behind `RUN_OAUTH_INTEGRATION=1` and a seeded OAuth client this environment does not have, so it stayed skipped. The claim that the production endpoint answers a refused code with an RFC 6749 §5.2 `400` is inherited from #65/#67, not re-measured here. - **The originating investigation and the parent epic were not read.** They were referenced to me by name only, with no accessible content, so their acceptance detail is unexercised — this PR implements the specification as handed to me. The consuming-app follow-up lives in a different repository and is out of scope here. - **The `.router` case is not exercised by this change beyond compilation.** Both new switch arms sit alongside it and the full suite is green, but nothing here tests router behaviour; that remains PR #66's coverage. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build` — clean; `swift test --no-parallel` after `rm -rf .build` — 292 tests in 31 suites, 0 failures (post-merge with the current `main`); pre-merge run on the stacked branch — 265 tests, 0 failures; scrub-then-normalize ordering falsified twice by flipping the production order and observing the credential leak, restored and the tree re-verified clean each time. - **Deviations:** none against the specification as given. Two additions beyond the literal list, both named under Judgment calls: the README version pin, and one adjacent factually-wrong comment corrected. --- <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:39 +00:00