[GH-ISSUE #48] OAuth token decoding rejects spec-compliant responses that omit refresh_token or expires_in #5

New issue

Open

opened 2026-10-26 22:33:44 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:33:44 +00:00

Owner

Originally created by @mattmillerai on GitHub (Jul 16, 2026).
Original GitHub issue: https://github.com/Comfy-Org/comfy-swift-sdk/issues/48

Split out of the review panel on #44, which flagged both as High/Medium but deferred them: #44 is a no-behavior-change extraction of a shared token-endpoint helper, and both gaps pre-date it (OAuthExchanger and OAuthTokenRefreshExecutor on main each already decoded these as required). Fixing them properly is a breaking public API change, so it gets its own ticket.

The gap

OAuthTokenEndpoint.TokenDTO decodes both fields as required:

let refreshToken: String
let expiresIn: Int

Neither is required by RFC 6749:

refresh_token (§6) — on a refresh grant the server may omit it, meaning "keep using the one you have." Many providers do not rotate on every refresh. When absent, JSONDecoder throws keyNotFound, which OAuthTokenEndpoint.post wraps as ComfyError.unknown. The 401 retry path in Transport only recovers from .authInvalid/.authExpired, so .unknown escapes: a spec-compliant refresh fails and forces full re-authentication.
expires_in (§5.1) — only RECOMMENDED, not required. An omitting server throws during decode and surfaces as ComfyError.unknown rather than a usable token.

Why it is not a one-line fix

OAuthTokenResponse is a public type (Sources/ComfySwiftSDK/Public/OAuthTokenResponse.swift):

public let refreshToken: String
public let expiresIn: Int

So a correct fix has to decide, deliberately:

Whether the public fields become optional (source-breaking for SDK consumers) or keep their non-optional shape and get filled in internally.
For refresh_token: thread a "reuse the prior refresh token when absent" fallback through the refresh caller and whatever persists the token. The DTO alone cannot do this — only the caller knows the previous token.
For expires_in: pick a default, and check how it interacts with proactive-refresh scheduling.

Option worth considering: keep OAuthTokenResponse non-optional (no consumer break), make the DTO fields optional, and have OAuthTokenEndpoint.post take the prior refresh token + a default lifetime to fill gaps. That contains the change to internals.

Acceptance

A refresh response omitting refresh_token succeeds and retains the previous refresh token.
A token response omitting expires_in decodes to a usable token with a documented default.
Neither case surfaces as ComfyError.unknown.
Regression tests for both, plus an explicit note in the changelog if the public API shape changes.

Originally created by @mattmillerai on GitHub (Jul 16, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-swift-sdk/issues/48 Split out of the review panel on #44, which flagged both as High/Medium but deferred them: #44 is a no-behavior-change extraction of a shared token-endpoint helper, and both gaps pre-date it (`OAuthExchanger` and `OAuthTokenRefreshExecutor` on `main` each already decoded these as required). Fixing them properly is a breaking public API change, so it gets its own ticket. ## The gap `OAuthTokenEndpoint.TokenDTO` decodes both fields as required: ```swift let refreshToken: String let expiresIn: Int ``` Neither is required by RFC 6749: - **`refresh_token` (§6)** — on a *refresh* grant the server may omit it, meaning "keep using the one you have." Many providers do not rotate on every refresh. When absent, `JSONDecoder` throws `keyNotFound`, which `OAuthTokenEndpoint.post` wraps as `ComfyError.unknown`. The 401 retry path in `Transport` only recovers from `.authInvalid`/`.authExpired`, so `.unknown` escapes: a spec-compliant refresh fails and forces full re-authentication. - **`expires_in` (§5.1)** — only RECOMMENDED, not required. An omitting server throws during decode and surfaces as `ComfyError.unknown` rather than a usable token. ## Why it is not a one-line fix `OAuthTokenResponse` is a **public** type (`Sources/ComfySwiftSDK/Public/OAuthTokenResponse.swift`): ```swift public let refreshToken: String public let expiresIn: Int ``` So a correct fix has to decide, deliberately: 1. Whether the public fields become optional (source-breaking for SDK consumers) or keep their non-optional shape and get filled in internally. 2. For `refresh_token`: thread a "reuse the prior refresh token when absent" fallback through the refresh caller and whatever persists the token. The DTO alone cannot do this — only the caller knows the previous token. 3. For `expires_in`: pick a default, and check how it interacts with proactive-refresh scheduling. Option worth considering: keep `OAuthTokenResponse` non-optional (no consumer break), make the *DTO* fields optional, and have `OAuthTokenEndpoint.post` take the prior refresh token + a default lifetime to fill gaps. That contains the change to internals. ## Acceptance - A refresh response omitting `refresh_token` succeeds and retains the previous refresh token. - A token response omitting `expires_in` decodes to a usable token with a documented default. - Neither case surfaces as `ComfyError.unknown`. - Regression tests for both, plus an explicit note in the changelog if the public API shape changes.

bsakaguchi commented

2026-10-26 22:33:45 +00:00

Author

Owner

@coderabbitai[bot] commented on GitHub (Jul 16, 2026):

🔗 Related PRs

Comfy-Org/ComfySwiftSDK#33 - feat(auth): ComfyWebAuthPresenter + one-call ComfyAuth.signIn [merged]

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

@coderabbitai[bot] commented on GitHub (Jul 16, 2026):  <details> <summary>🔗 Related PRs</summary> Comfy-Org/ComfySwiftSDK#33 - feat(auth): ComfyWebAuthPresenter + one-call ComfyAuth.signIn [merged] </details> --- <details> <summary> 🧪 Issue enrichment is currently in open beta.</summary> You can configure auto-planning by selecting labels in the issue_enrichment configuration. To disable automatic issue enrichment, add the following to your `.coderabbit.yaml`: ```yaml issue_enrichment: auto_enrich: enabled: false ``` </details> 💬 Have feedback or questions? Drop into our [discord](https://discord.gg/coderabbit)!

bsakaguchi referenced this issue

2026-10-26 22:33:51 +00:00

[PR #5] [MERGED] fix(reattach): retry transient catch-up GET failures instead of failing the stream #10

bsakaguchi referenced this issue

2026-10-26 22:33:53 +00:00

[PR #7] [MERGED] feat(logging): add credential-free os.Logger error logging at SDK catch sites #12