[PR #31] [MERGED] feat(auth): parameterize OAuth client config + add callback state-verification helper #32

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-2449-oauth-client-config

📝 Commits (2)

c2ae295 feat(auth): parameterize OAuth client config + add callback state-verification helper
c1ca10c fix(auth): thread OAuthClientConfig through refresh + tighten callback checks

📊 Changes

13 files changed (+422 additions, -69 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthConfiguration.swift (+4 -12)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift (+7 -3)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift (+9 -2)
📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+2 -2)
📝 Sources/ComfySwiftSDK/Internal/SDKLog.swift (+2 -0)
📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+9 -2)
📝 Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift (+42 -17)
📝 Sources/ComfySwiftSDK/Public/ComfyError.swift (+11 -0)
📝 Sources/ComfySwiftSDK/Public/OAuthAuthorizationRequest.swift (+43 -1)
➕ Sources/ComfySwiftSDK/Public/OAuthClientConfig.swift (+78 -0)
➕ Tests/ComfySwiftSDKTests/OAuthClientConfigTests.swift (+176 -0)
📝 Tests/ComfySwiftSDKTests/OAuthConfigurationTests.swift (+2 -29)
📝 Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift (+37 -1)

📄 Description

ELI-5

The SDK's "Sign in with Comfy" code had the iOS app's identity (client id, redirect URL, scopes) baked in as constants, so no other app could reuse it. This makes those values a small OAuthClientConfig value you pass in — defaulting to the exact iOS values, so nothing existing changes. It also adds one SDK helper, extractCode(fromCallback:), for the security check every app was copy-pasting: confirm the callback's state matches the one we sent (CSRF defense) and that there's a real code, before redeeming it.

What changed

New public struct OAuthClientConfig: Sendable (clientId, redirectScheme, redirectURI, scopes: [String]) with static let comfyIOS carrying today's exact hardcoded values. Shared protocol endpoints (issuer / authorize / token / metadata) stay in the internal OAuthConfiguration.
buildAuthorizationRequest(config:) and exchangeAuthorizationCode(_:codeVerifier:config:) now take a config defaulting to .comfyIOS. Existing no-arg / two-arg call sites keep compiling and behave identically.
OAuthAuthorizationRequest.extractCode(fromCallback:) — ports the app's parseCallback state-verification into the SDK: parses code+state, rejects an empty/missing code, and requires state == self.state.
ComfyError.authStateMismatch and .authCancelled added and documented (DocC); wired into the two exhaustive ComfyError switches (PollingFallback.isTransient, SDKLog case-name mapper — both treat them as non-transient, and the mapper only ever emits the case name, never a secret).
oauthCallbackScheme static now sources the comfyIOS default from the config; unchanged value (org.comfy.ios).

Tests

New OAuthClientConfigTests + updates:

authorize URL carries the injected client_id / redirect_uri / scopes for a custom config;
extractCode matrix: valid, empty code, missing code, missing state, and state-mismatch → .authStateMismatch;
back-compat: comfyIOS equals the prior hardcoded values (client id, scheme, redirect URI, scope list/string), default helpers still use comfyIOS, and oauthCallbackScheme is unchanged.

swift build + swift test green (180 tests, 0 failures, incl. ImportBoundaryTests). No new imports — OAuthClientConfig is Foundation-only, and the allowlist boundary test passes.

Judgment calls (please sanity-check)

Error mapping for malformed callbacks. extractCode mirrors parseCallback's control flow exactly (same guards, fail-closed), but the SDK only gained two error cases, so the app's single missingCallbackParameters bucket (missing/empty code, or missing state) maps to .authCancelled here, while state mismatch maps to .authStateMismatch (as the ticket specifies). Missing-state therefore surfaces as .authCancelled, not .authStateMismatch — deliberate: a code we can't state-verify is never redeemed.
Parameterized via a defaulted argument, not a second overload. buildAuthorizationRequest(config: = .comfyIOS) is one method; the old no-arg form is just the default. Source-compatible for an SPM (source) package.
Refresh grant left comfyIOS-pinned. OAuthTokenRefreshExecutor still uses the comfy-ios client id (now sourced from OAuthClientConfig.comfyIOS for a single home) — parameterizing the refresh flow is out of scope; behavior is byte-identical.

_{🔄 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/31 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/5/2026 **Status:** ✅ Merged **Merged:** 7/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2449-oauth-client-config` --- ### 📝 Commits (2) - [`c2ae295`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/c2ae295562719280e164f08d5acb25e94c8e564c) feat(auth): parameterize OAuth client config + add callback state-verification helper - [`c1ca10c`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/c1ca10cff689e3acdc9073a894ec4180f91a3829) fix(auth): thread OAuthClientConfig through refresh + tighten callback checks ### 📊 Changes **13 files changed** (+422 additions, -69 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthConfiguration.swift` (+4 -12) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift` (+7 -3) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift` (+9 -2) 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+2 -2) 📝 `Sources/ComfySwiftSDK/Internal/SDKLog.swift` (+2 -0) 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+9 -2) 📝 `Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift` (+42 -17) 📝 `Sources/ComfySwiftSDK/Public/ComfyError.swift` (+11 -0) 📝 `Sources/ComfySwiftSDK/Public/OAuthAuthorizationRequest.swift` (+43 -1) ➕ `Sources/ComfySwiftSDK/Public/OAuthClientConfig.swift` (+78 -0) ➕ `Tests/ComfySwiftSDKTests/OAuthClientConfigTests.swift` (+176 -0) 📝 `Tests/ComfySwiftSDKTests/OAuthConfigurationTests.swift` (+2 -29) 📝 `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift` (+37 -1) </details> ### 📄 Description ## ELI-5 The SDK's "Sign in with Comfy" code had the iOS app's identity (client id, redirect URL, scopes) baked in as constants, so no other app could reuse it. This makes those values a small `OAuthClientConfig` value you pass in — defaulting to the exact iOS values, so nothing existing changes. It also adds one SDK helper, `extractCode(fromCallback:)`, for the security check every app was copy-pasting: confirm the callback's `state` matches the one we sent (CSRF defense) and that there's a real code, before redeeming it. ## What changed - **New `public struct OAuthClientConfig: Sendable`** (`clientId`, `redirectScheme`, `redirectURI`, `scopes: [String]`) with `static let comfyIOS` carrying today's exact hardcoded values. Shared protocol endpoints (issuer / authorize / token / metadata) stay in the internal `OAuthConfiguration`. - **`buildAuthorizationRequest(config:)` and `exchangeAuthorizationCode(_:codeVerifier:config:)`** now take a config defaulting to `.comfyIOS`. Existing no-arg / two-arg call sites keep compiling and behave identically. - **`OAuthAuthorizationRequest.extractCode(fromCallback:)`** — ports the app's `parseCallback` state-verification into the SDK: parses `code`+`state`, rejects an empty/missing `code`, and requires `state == self.state`. - **`ComfyError.authStateMismatch` and `.authCancelled`** added and documented (DocC); wired into the two exhaustive `ComfyError` switches (`PollingFallback.isTransient`, `SDKLog` case-name mapper — both treat them as non-transient, and the mapper only ever emits the case name, never a secret). - **`oauthCallbackScheme`** static now sources the comfyIOS default from the config; unchanged value (`org.comfy.ios`). ## Tests New `OAuthClientConfigTests` + updates: - authorize URL carries the **injected** `client_id` / `redirect_uri` / `scopes` for a custom config; - `extractCode` matrix: valid, empty code, missing code, missing state, and state-mismatch → `.authStateMismatch`; - back-compat: `comfyIOS` equals the prior hardcoded values (client id, scheme, redirect URI, scope list/string), default helpers still use comfyIOS, and `oauthCallbackScheme` is unchanged. `swift build` + `swift test` green (**180 tests, 0 failures**, incl. `ImportBoundaryTests`). No new imports — `OAuthClientConfig` is Foundation-only, and the allowlist boundary test passes. ## Judgment calls (please sanity-check) 1. **Error mapping for malformed callbacks.** `extractCode` mirrors `parseCallback`'s *control flow* exactly (same guards, fail-closed), but the SDK only gained two error cases, so the app's single `missingCallbackParameters` bucket (missing/empty `code`, or missing `state`) maps to **`.authCancelled`** here, while `state` **mismatch** maps to `.authStateMismatch` (as the ticket specifies). Missing-`state` therefore surfaces as `.authCancelled`, not `.authStateMismatch` — deliberate: a code we can't state-verify is never redeemed. 2. **Parameterized via a defaulted argument, not a second overload.** `buildAuthorizationRequest(config: = .comfyIOS)` is one method; the old no-arg form is just the default. Source-compatible for an SPM (source) package. 3. **Refresh grant left comfyIOS-pinned.** `OAuthTokenRefreshExecutor` still uses the comfy-ios client id (now sourced from `OAuthClientConfig.comfyIOS` for a single home) — parameterizing the refresh flow is out of scope; behavior is byte-identical. --- <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:10 +00:00