[PR #46] [MERGED] refactor(auth): extract shared normalizeToken(_:) helper across auth sites #47

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-2862-normalize-token-helper

📝 Commits (2)

458f818 refactor(auth): extract shared normalizeToken(_:) helper across auth sites
3284b74 fix(auth): surface cancellation as .cancelled in normalizeToken, not .authInvalid

📊 Changes

4 files changed (+173 additions, -33 deletions)

View changed files

➕ Sources/ComfySwiftSDK/Internal/Auth/TokenNormalization.swift (+30 -0)
📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+8 -24)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+2 -9)
📝 Tests/ComfySwiftSDKTests/BearerInjectionTests.swift (+133 -0)

📄 Description

ELI-5

Three different places in the SDK all had the same little rule copy-pasted: "ask for a
token; if it comes back empty or the token-fetcher blows up, treat it as a bad login
(.authInvalid) — but if it throws a known auth error, pass that through untouched."
Two of those places set the HTTP Authorization: Bearer header; the third builds the
WebSocket ?token= URL. Because the rule was duplicated, the HTTP path and the WebSocket
path could silently drift apart over time on what "empty token" or "fetcher threw" means.

This PR moves that rule into one tiny helper, normalizeToken(_:), and has all three
sites call it. No behavior changes — same inputs still produce the exact same errors.

What changed

New: Sources/ComfySwiftSDK/Internal/Auth/TokenNormalization.swift — a pure,
stateless internal func normalizeToken(_:) that is the single source of truth for the
contract:
- empty result → ComfyError.authInvalid
- producer throws ComfyError → rethrown unchanged
- producer throws anything else → ComfyError.authInvalid
Transport.applyAuth .oauth and .oauthRefreshable cases now delegate to it.
The refreshIfNearExpiry(...) call is kept inside the closure, so a non-ComfyError
thrown by the refresh/expiry providers still maps to .authInvalid exactly as before.
WebSocketSession.buildWebSocketURL .oauth/.oauthRefreshable case delegates to
it, preserving the existing quirk that the WebSocket path does not refresh on
.oauthRefreshable (it reads the tokenProvider only).
Tests (added, none modified): three characterization asserts in
BearerInjectionTests covering the branches not directly exercised before — the .oauth
HTTP header path for a throwing provider and an empty token, and the .oauthRefreshable
HTTP path where refreshIfNearExpiry throws a non-ComfyError.

Why it's safe (behavior preservation)

The old per-site do/catch shape and normalizeToken(_:) implement byte-identical
mapping logic. The one load-bearing subtlety is keeping refreshIfNearExpiry(...) inside
the normalizeToken closure in the refreshable case; moving it out would let a
non-ComfyError refresh/expiry failure escape unmapped — a security-behavior change. I
verified the new refreshable test genuinely pins this by temporarily hoisting the call out
of the closure and confirming the test fails (Expected .authInvalid, got ProviderFailure()),
then reverted.

Verification

swift build clean.
swift test fully green — 229 tests, 28 suites, all pre-existing BearerInjection /
CredentialMode / RefreshOn401 tests unchanged and passing.
grep -rn 'isEmpty else { throw ComfyError.authInvalid' Sources/ returns only
TokenNormalization.swift — no inline guard remains at any call site.

⚠️ Security review requested

This touches the credential-injection predicate (Bearer header + WebSocket token query).
The change is a strictly structural refactor with no intended error-mapping outcome change,
but please give the auth-mapping equivalence a human security review before merge.

_{🔄 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/46 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/11/2026 **Status:** ✅ Merged **Merged:** 7/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2862-normalize-token-helper` --- ### 📝 Commits (2) - [`458f818`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/458f8184a978b5e19dd9e225f13911dcee7cec87) refactor(auth): extract shared normalizeToken(_:) helper across auth sites - [`3284b74`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/3284b7498a037e94750deef432b1e6a525d04a33) fix(auth): surface cancellation as .cancelled in normalizeToken, not .authInvalid ### 📊 Changes **4 files changed** (+173 additions, -33 deletions) <details> <summary>View changed files</summary> ➕ `Sources/ComfySwiftSDK/Internal/Auth/TokenNormalization.swift` (+30 -0) 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+8 -24) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+2 -9) 📝 `Tests/ComfySwiftSDKTests/BearerInjectionTests.swift` (+133 -0) </details> ### 📄 Description ## ELI-5 Three different places in the SDK all had the same little rule copy-pasted: "ask for a token; if it comes back empty or the token-fetcher blows up, treat it as a bad login (`.authInvalid`) — but if it throws a *known* auth error, pass that through untouched." Two of those places set the HTTP `Authorization: Bearer` header; the third builds the WebSocket `?token=` URL. Because the rule was duplicated, the HTTP path and the WebSocket path could silently drift apart over time on what "empty token" or "fetcher threw" means. This PR moves that rule into **one** tiny helper, `normalizeToken(_:)`, and has all three sites call it. No behavior changes — same inputs still produce the exact same errors. ## What changed - **New:** `Sources/ComfySwiftSDK/Internal/Auth/TokenNormalization.swift` — a pure, stateless `internal func normalizeToken(_:)` that is the single source of truth for the contract: - empty result → `ComfyError.authInvalid` - producer throws `ComfyError` → rethrown unchanged - producer throws anything else → `ComfyError.authInvalid` - **`Transport.applyAuth`** `.oauth` and `.oauthRefreshable` cases now delegate to it. The `refreshIfNearExpiry(...)` call is kept **inside** the closure, so a non-`ComfyError` thrown by the refresh/expiry providers still maps to `.authInvalid` exactly as before. - **`WebSocketSession.buildWebSocketURL`** `.oauth`/`.oauthRefreshable` case delegates to it, preserving the existing quirk that the WebSocket path does **not** refresh on `.oauthRefreshable` (it reads the `tokenProvider` only). - **Tests (added, none modified):** three characterization asserts in `BearerInjectionTests` covering the branches not directly exercised before — the `.oauth` HTTP header path for a throwing provider and an empty token, and the `.oauthRefreshable` HTTP path where `refreshIfNearExpiry` throws a non-`ComfyError`. ## Why it's safe (behavior preservation) The old per-site `do/catch` shape and `normalizeToken(_:)` implement byte-identical mapping logic. The one load-bearing subtlety is keeping `refreshIfNearExpiry(...)` inside the `normalizeToken` closure in the refreshable case; moving it out would let a non-`ComfyError` refresh/expiry failure escape unmapped — a security-behavior change. I verified the new refreshable test genuinely pins this by temporarily hoisting the call out of the closure and confirming the test fails (`Expected .authInvalid, got ProviderFailure()`), then reverted. ## Verification - `swift build` clean. - `swift test` fully green — **229 tests, 28 suites**, all pre-existing BearerInjection / CredentialMode / RefreshOn401 tests unchanged and passing. - `grep -rn 'isEmpty else { throw ComfyError.authInvalid' Sources/` returns **only** `TokenNormalization.swift` — no inline guard remains at any call site. ## ⚠️ Security review requested This touches the credential-injection predicate (Bearer header + WebSocket token query). The change is a strictly structural refactor with no intended error-mapping outcome change, but please give the auth-mapping equivalence a human security review before merge. --- <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:22 +00:00