[PR #78] [MERGED] feat(errors)!: give ComfyError a sanitizing, bounded description #79

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-14667-comfyerror-sanitized-description

📝 Commits (4)

edc15a9 feat(errors)!: give ComfyError a sanitizing description so no payload reaches a log unbounded
1d17e36 fix(errors): bound JobExecutionError's server-derived fields when reflected
c9e8f79 fix(errors): redact on the failing-URL keys, not the error domain
af091e3 fix(errors): redact on URL-key presence, and spend the whole log budget

📊 Changes

8 files changed (+945 additions, -5 deletions)

View changed files

📝 README.md (+1 -1)
📝 Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift (+1 -1)
➕ Sources/ComfySwiftSDK/Internal/LogSafeText.swift (+114 -0)
📝 Sources/ComfySwiftSDK/Internal/SDKLog.swift (+4 -1)
📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+30 -1)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+19 -1)
📝 Sources/ComfySwiftSDK/Public/ComfyError.swift (+235 -0)
➕ Tests/ComfySwiftSDKTests/ComfyErrorDescriptionTests.swift (+541 -0)

📄 Description

ELI-5

If something goes wrong, the SDK hands your app a ComfyError. If you then wrote that error into a log — logger.error("\(error)") — Swift printed it by reflection, which dumps every value inside verbatim: the whole server message, the whole failing URL, newlines and all, with no length limit. A hostile or just chatty server could therefore write its own fake lines into your log, or flood it. RouterError in this same SDK already fixed that for itself; this brings the rest of the error taxonomy up to the same bar, and closes one credential leak that the length limit alone would not have caught.

What changed

ComfyError declared only Error, Sendable, so String(describing:) and every "\(error)" fell to Swift's default enum reflection. Three payload shapes reach that renderer carrying unbounded, control-character-unhandled, server- or network-derived text: the URLSession error boxed in .network(underlying:) / .unknown(underlying:) (whose bridged NSError userInfo carries the failing URL, query string included), SubmitErrorBody boxed in .unknown(underlying:) (raw server string, no description of its own), and .serverRejected(reason: .other(_)) (the raw error / message / reason field from the server's JSON).

extension ComfyError: CustomStringConvertible, CustomDebugStringConvertible — description switches over every case, deliberately exhaustively so a new case is a compile error rather than a silent reflection fallback. Payload-free cases render a short stable label reusing the vocabulary SDKLog.comfyErrorCaseName already emits (a test asserts the two agree, so a reader correlating a log line with a rendered error does not need to know two spellings). debugDescription mirrors description.
.router(_) delegates to RouterError.description rather than re-rendering its stored fields, so the Idempotency-Key and the server-echoed input values inside validationErrors that it deliberately withholds stay withheld.
.rateLimited(retryAfter:) renders the Double directly. Not Int(retryAfter) — that is a trapping conversion on a response-controlled value (Retry-After: 9223372036854775807 stores exactly 2^63, one past Int.max, and Int(_:) on it terminates the process), the same hazard already called out on RouterError.description.
New LogSafeText.bounded(_:to:) replaces every scalar in CharacterSet.controlCharacters.union(.newlines) with . — the union deliberately includes U+2028/U+2029, which are Zl/Zp rather than Cc/Cf but which a log viewer renders as line breaks — and clamps to 512 UTF-8 bytes with the ellipsis charged to the same budget. The cut walks by Character so it never lands mid-cluster; when a single cluster is wider than the room left (combining marks are neither control characters nor newlines), whatever room the Character walk leaves is filled at the scalar boundary from the cluster it stopped on — wherever in the string that cluster sits — so one wide cluster cannot collapse the rendering to its prefix.
SubmitErrorBody and JobExecutionError gain their own bounded descriptions (JobExecutionError omitting absent fields rather than rendering a nil placeholder a server could also send), so they are safe wherever they are reflected rather than only inside the one renderer that wraps them.
SDK version bumped 0.5.0 → 0.6.0 (README install pin with it), matching how the last error-taxonomy change was released.

The judgment call worth reviewing

The plan this PR implements prescribed rendering a boxed error as type(of:) plus a clamped String(describing:). That bound does not actually close the leak it names, and the diff therefore goes further for NSURLErrorDomain errors specifically.

WebSocketSession.buildWebSocketURL puts the API key or OAuth access token in the WebSocket URL's token query item. A receive() failure on that task reaches .network(underlying:) via Transport.translate, carrying that URL in its bridged userInfo. Measured against a synthetic-but-shaped failure, the entire leaking reflection is 271 bytes against a 512-byte budget — the credential survives clamping completely intact. This repo's own rule is that credentials stay out of logs and error messages, so a renderer built to make errors log-safe should not still print a live token.

So an error carrying a failing URL is rendered from the fields that are diagnostic and cannot carry a secret — the domain, the code (for NSURLErrorDomain, the URLError.Code raw value, which is the classification), and the failing URL's scheme, host and path — with the query, fragment and any userinfo dropped and the userInfo dictionary never reflected. When the failing URL is absent or unparseable, the domain and code stand alone. Review follow-up: the trigger is the URL-bearing userInfo keys rather than the domain, because those keys are not exclusive to NSURLErrorDomain (Transport.translate boxes NSPOSIXErrorDomain and CFNetwork failures into .network / .unknown, and URLSession puts the same failing URL on those), with NSURLErrorDomain still triggering on its own and the NSUnderlyingErrorKey chain followed to a bounded depth. Round 2: the trigger is now the presence of one of those keys, not the success of extracting a URL from it. A key holding a string URL(string:) rejects — or a key sitting past the chain walk's bounds — left the extraction nil and fell through to String(describing:), which prints the whole UserInfo={…} whether or not this code could parse the value. Presence now decides the redaction; the parsed URL only decides whether a scheme/host/path url= is appended or the domain and code stand alone. The walk's own bounds (4 levels of NSUnderlyingErrorKey, 4 wide across NSMultipleUnderlyingErrorsKey, which is now followed too) count whatever they leave unwalked as carrying a URL. One measured correction to the finding: NSError.description on this toolchain stops printing nested underlying errors at about three levels, so the depth-cap fall-through was not in fact reachable that way — but that is an undocumented Foundation detail, and the code no longer depends on it. Every error carrying neither key still reflects normally, bounded and control-stripped — widening it to every bridged NSError would flatten the SDK's own boxed error types, since every Swift error bridges.

This removes information from a log line, so it is worth stating what is not lost: the tests assert the endpoint (wss://host/ws), the domain and the numeric code all still survive, which is what identifies the failure. What is dropped is the query string, whose only SDK-produced contents are clientId and token.

JobExecutionError is the second deviation: it is the same shape as SubmitErrorBody (three strings straight off the server's execution_error frame, boxed as .unknown(underlying:), no description) but the plan's audit named only SubmitErrorBody. It gets the identical treatment.

Behaviour change

This changes what String(describing:) returns for every consumer of ComfyError. It is a log-string format only — nothing should parse it, the full values remain readable on the associated values themselves, and localizedDescription is unaffected (ComfyError is not LocalizedError, so NSError bridging still supplies that). The package is pre-1.0. No log line the SDK itself emits changes: SDKLog renders errors through comfyErrorCaseName, never by interpolating the error.

Test plan

New ComfyErrorDescriptionTests (30 tests), mirroring the existing RouterError description tests:

a URLError carrying a failing URL renders neither the query string nor a raw newline, from both .network and .unknown — including an explicit non-vacuity assertion that the unrendered reflection does contain the credential and does fit the byte budget, so the test cannot pass for the wrong reason;
a URLSession error with no failing URL still renders its domain and code; a non-NSURLErrorDomain error still reflects normally;
.serverRejected(.other("a\nb" + long)) is control-stripped (a.b, replaced not dropped) and length-capped; the four closed reasons render by name;
.router(e) output equals "ComfyError.router(\(e.description))" exactly, and contains no idempotency key under either description or String(reflecting:);
the byte budget: truncation lands exactly at 512 bytes with …; the budget is bytes not characters (1,000 four-byte scalars); a value inside the budget is returned whole and unmarked; sanitizing that shrinks a value does not falsely mark it truncated; the ellipsis is charged to the budget rather than added to it (the bug that made a "512-byte" bound render 515, caught by these tests during development);
SubmitErrorBody and JobExecutionError are bounded and stripped when reflected standalone as well as when boxed;
a huge Retry-After does not trap the description;
every payload-free case renders its stable label, and every case's rendering leads with the label SDKLog.comfyErrorCaseName emits.

Residual

Not fixed by this PR, and each one is a separate, self-contained piece of work:

ServerRejectionReason still has no CustomStringConvertible of its own. ComfyError.description renders it safely via a private helper, but a consumer who destructures .serverRejected(reason:) and writes "\(reason)" still gets default reflection, which prints .other(_)'s raw server string verbatim and unbounded. Deliberately left alone: the plan scoped the new renderers to ComfyError and SubmitErrorBody, and adding a public conformance to a second public type is a behaviour change worth deciding on its own. Fix is ~12 lines in ComfyError.swift, mirroring the private rendered(_:) helper this PR already has.
RouterError.loggable still bounds by 512 scalars, not 512 bytes (RouterError.swift:434-441). Because .router(_) delegates, ComfyError.description for that one case can therefore exceed the 512-byte budget every other case obeys — up to ~4× per field, across several fields. Left alone deliberately: the plan's instruction was to delegate, and RouterError's rendering is shipped and covered by its own tests, so changing it is a separate behaviour change. The two units should probably be reconciled onto the shared LogSafeText helper.
~~The URL redaction is scoped to NSURLErrorDomain.~~ Closed by review follow-up: the redaction is now keyed on the URL-bearing userInfo keys, so a CFNetwork- or POSIX-domain failure carrying the same URL is redacted too. What remains is narrower: a boxed error that carries a credential somewhere other than those keys is bounded and control-stripped but not redacted. I swept every Error type the SDK can box into .network/.unknown — 40 construction sites, resolving to SubmitErrorBody, JobExecutionError, EmptyOutputError, OAuthTokenEndpointError, RouterInputSerializationError, WorkflowInputNotSupportedError, URLError, POSIX NSError and caught DecodingErrors — and none of the remainder carries a credential today: OAuthTokenEndpointError is scrubbed and clamped at construction, RouterInputSerializationError is a fixed string, WorkflowInputNotSupportedError carries a Swift case name, EmptyOutputError has no payload. A future boxed type could reintroduce the gap, and nothing enforces that it does not.
JobExecutionError RETAINS its three server strings uncapped. exceptionType / exceptionMessage / nodeType come straight off the execution_error frame and are bounded at render time only, so a hostile host's multi-megabyte exception_message stays alive for as long as the consumer holds the ComfyError.unknown(underlying:) that boxes it. SubmitErrorBody got a construction-time cap in this PR and this type did not: the log hazard (forged lines, crowded-out log) is closed either way, what is left is memory retention, and the fix is the same ~10 lines (SubmitErrorBody.messageMaxLength-style clamp in an initializer) applied to a second type. Self-contained follow-up.
ServerRejectionReason.other's payload is likewise bounded only at render time. The stored string is uncapped, and it is extracted from the same fully buffered, size-unlimited submit body before Transport.checkBody throws .serverRejected(reason: .other(message)) (Sources/ComfySwiftSDK/Internal/Transport.swift:649). Capping inside the enum would change a public associated value, so the placement would be that Transport throw site instead — a behaviour change to what a public error payload carries, which deserves its own PR rather than a rider on this one. (This is the retention half; the bullet above about ServerRejectionReason having no CustomStringConvertible is the separate rendering half.)
Unexercised artifacts. The plan this PR implements came from a prior read-only audit whose findings I could not read directly, so its count of "30+ .unknown sites" was re-derived rather than re-verified site by site: I measured 40 .network/.unknown construction sites across Sources/ComfySwiftSDK/. Nothing was run against a live Comfy server — the WebSocket credential leak is demonstrated against a synthetic URLError shaped like the one URLSession produces (correct keys, correct domain), not against a real receive() failure, so the claim that a real failure populates NSURLErrorFailingURLErrorKey is inferred from URLSession's documented behaviour rather than observed. The consuming iOS app is out of scope and untouched.

Provenance

Authored by: agent-work loop
Verified: swift build: clean, no warnings from changed files; swift test --no-parallel: 402 tests in 33 suites passed, 0 failed (397 after the round-1 follow-up, 394 at open) (two pre-existing Swift 6 SendableClosureCaptures warnings in PollingFallbackTests / JobStatusEndpointMigrationTests, both untouched by this PR); python3 Scripts/contract/check_router_contract.py: OK, 15 error types in sync with the vendored spec.
Deviations: two, both argued under "The judgment call worth reviewing" above — NSURLErrorDomain errors are rendered from chosen fields rather than a clamped String(describing:), because the prescribed clamp measurably does not remove the credential it was meant to bound; and JobExecutionError received the same belt-and-braces description as SubmitErrorBody, which the plan's audit did not name.

_{🔄 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/78 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-14667-comfyerror-sanitized-description` --- ### 📝 Commits (4) - [`edc15a9`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/edc15a946661d518f9928a7b5a61dc43f3eeb7d9) feat(errors)!: give ComfyError a sanitizing description so no payload reaches a log unbounded - [`1d17e36`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/1d17e365698400dacb1436d56268983329c2a2e0) fix(errors): bound JobExecutionError's server-derived fields when reflected - [`c9e8f79`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/c9e8f7900ed7a7b430e6ed1f5ac449a67deb3295) fix(errors): redact on the failing-URL keys, not the error domain - [`af091e3`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/af091e3651c5c52b7149841dd0b4b27dc6bd16bd) fix(errors): redact on URL-key presence, and spend the whole log budget ### 📊 Changes **8 files changed** (+945 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+1 -1) 📝 `Sources/ComfySwiftSDK/Internal/ComfySDKInfo.swift` (+1 -1) ➕ `Sources/ComfySwiftSDK/Internal/LogSafeText.swift` (+114 -0) 📝 `Sources/ComfySwiftSDK/Internal/SDKLog.swift` (+4 -1) 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+30 -1) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+19 -1) 📝 `Sources/ComfySwiftSDK/Public/ComfyError.swift` (+235 -0) ➕ `Tests/ComfySwiftSDKTests/ComfyErrorDescriptionTests.swift` (+541 -0) </details> ### 📄 Description ## ELI-5 If something goes wrong, the SDK hands your app a `ComfyError`. If you then wrote that error into a log — `logger.error("\(error)")` — Swift printed it by reflection, which dumps every value inside verbatim: the whole server message, the whole failing URL, newlines and all, with no length limit. A hostile or just chatty server could therefore write its own fake lines into your log, or flood it. `RouterError` in this same SDK already fixed that for itself; this brings the rest of the error taxonomy up to the same bar, and closes one credential leak that the length limit alone would not have caught. ## What changed `ComfyError` declared only `Error, Sendable`, so `String(describing:)` and every `"\(error)"` fell to Swift's default enum reflection. Three payload shapes reach that renderer carrying unbounded, control-character-unhandled, server- or network-derived text: the `URLSession` error boxed in `.network(underlying:)` / `.unknown(underlying:)` (whose bridged `NSError` `userInfo` carries the failing URL, query string included), `SubmitErrorBody` boxed in `.unknown(underlying:)` (raw server string, no `description` of its own), and `.serverRejected(reason: .other(_))` (the raw `error` / `message` / `reason` field from the server's JSON). - **`extension ComfyError: CustomStringConvertible, CustomDebugStringConvertible`** — `description` switches over every case, deliberately exhaustively so a new case is a compile error rather than a silent reflection fallback. Payload-free cases render a short stable label reusing the vocabulary `SDKLog.comfyErrorCaseName` already emits (a test asserts the two agree, so a reader correlating a log line with a rendered error does not need to know two spellings). `debugDescription` mirrors `description`. - **`.router(_)` delegates** to `RouterError.description` rather than re-rendering its stored fields, so the `Idempotency-Key` and the server-echoed `input` values inside `validationErrors` that it deliberately withholds stay withheld. - **`.rateLimited(retryAfter:)` renders the `Double` directly.** Not `Int(retryAfter)` — that is a trapping conversion on a response-controlled value (`Retry-After: 9223372036854775807` stores exactly 2^63, one past `Int.max`, and `Int(_:)` on it terminates the process), the same hazard already called out on `RouterError.description`. - **New `LogSafeText.bounded(_:to:)`** replaces every scalar in `CharacterSet.controlCharacters.union(.newlines)` with `.` — the union deliberately includes U+2028/U+2029, which are Zl/Zp rather than Cc/Cf but which a log viewer renders as line breaks — and clamps to **512 UTF-8 bytes** with the ellipsis charged to the same budget. The cut walks by `Character` so it never lands mid-cluster; when a single cluster is wider than the room left (combining marks are neither control characters nor newlines), whatever room the `Character` walk leaves is filled at the scalar boundary from the cluster it stopped on — wherever in the string that cluster sits — so one wide cluster cannot collapse the rendering to its prefix. - **`SubmitErrorBody` and `JobExecutionError` gain their own bounded `description`s** (`JobExecutionError` omitting absent fields rather than rendering a `nil` placeholder a server could also send), so they are safe wherever they are reflected rather than only inside the one renderer that wraps them. - SDK version bumped `0.5.0` → `0.6.0` (README install pin with it), matching how the last error-taxonomy change was released. ## The judgment call worth reviewing The plan this PR implements prescribed rendering a boxed error as `type(of:)` plus a clamped `String(describing:)`. **That bound does not actually close the leak it names**, and the diff therefore goes further for `NSURLErrorDomain` errors specifically. `WebSocketSession.buildWebSocketURL` puts the API key or OAuth access token in the WebSocket URL's `token` query item. A `receive()` failure on that task reaches `.network(underlying:)` via `Transport.translate`, carrying that URL in its bridged `userInfo`. Measured against a synthetic-but-shaped failure, the entire leaking reflection is **271 bytes against a 512-byte budget** — the credential survives clamping completely intact. This repo's own rule is that credentials stay out of logs and error messages, so a renderer built to make errors log-safe should not still print a live token. So an error carrying a failing URL is rendered from the fields that are diagnostic and cannot carry a secret — the domain, the code (for `NSURLErrorDomain`, the `URLError.Code` raw value, which *is* the classification), and the failing URL's scheme, host and path — with the query, fragment and any userinfo dropped and the `userInfo` dictionary never reflected. When the failing URL is absent or unparseable, the domain and code stand alone. Review follow-up: the trigger is the URL-bearing `userInfo` keys rather than the domain, because those keys are not exclusive to `NSURLErrorDomain` (`Transport.translate` boxes `NSPOSIXErrorDomain` and CFNetwork failures into `.network` / `.unknown`, and `URLSession` puts the same failing URL on those), with `NSURLErrorDomain` still triggering on its own and the `NSUnderlyingErrorKey` chain followed to a bounded depth. **Round 2:** the trigger is now the *presence* of one of those keys, not the success of extracting a URL from it. A key holding a string `URL(string:)` rejects — or a key sitting past the chain walk's bounds — left the extraction nil and fell through to `String(describing:)`, which prints the whole `UserInfo={…}` whether or not this code could parse the value. Presence now decides the redaction; the parsed URL only decides whether a scheme/host/path `url=` is appended or the domain and code stand alone. The walk's own bounds (4 levels of `NSUnderlyingErrorKey`, 4 wide across `NSMultipleUnderlyingErrorsKey`, which is now followed too) count whatever they leave unwalked as carrying a URL. One measured correction to the finding: `NSError.description` on this toolchain stops printing nested underlying errors at about three levels, so the depth-cap fall-through was not in fact reachable that way — but that is an undocumented Foundation detail, and the code no longer depends on it. Every error carrying neither key still reflects normally, bounded and control-stripped — widening it to every bridged `NSError` would flatten the SDK's own boxed error types, since every Swift error bridges. This removes information from a log line, so it is worth stating what is *not* lost: the tests assert the endpoint (`wss://host/ws`), the domain and the numeric code all still survive, which is what identifies the failure. What is dropped is the query string, whose only SDK-produced contents are `clientId` and `token`. `JobExecutionError` is the second deviation: it is the same shape as `SubmitErrorBody` (three strings straight off the server's `execution_error` frame, boxed as `.unknown(underlying:)`, no `description`) but the plan's audit named only `SubmitErrorBody`. It gets the identical treatment. ## Behaviour change This changes what `String(describing:)` returns for every consumer of `ComfyError`. It is a log-string format only — nothing should parse it, the full values remain readable on the associated values themselves, and `localizedDescription` is unaffected (`ComfyError` is not `LocalizedError`, so NSError bridging still supplies that). The package is pre-1.0. No log line the SDK itself emits changes: `SDKLog` renders errors through `comfyErrorCaseName`, never by interpolating the error. ## Test plan New `ComfyErrorDescriptionTests` (30 tests), mirroring the existing `RouterError` description tests: - a `URLError` carrying a failing URL renders neither the query string nor a raw newline, from both `.network` and `.unknown` — including an explicit non-vacuity assertion that the unrendered reflection *does* contain the credential and *does* fit the byte budget, so the test cannot pass for the wrong reason; - a `URLSession` error with no failing URL still renders its domain and code; a non-`NSURLErrorDomain` error still reflects normally; - `.serverRejected(.other("a\nb" + long))` is control-stripped (`a.b`, replaced not dropped) and length-capped; the four closed reasons render by name; - `.router(e)` output equals `"ComfyError.router(\(e.description))"` exactly, and contains no idempotency key under either `description` or `String(reflecting:)`; - the byte budget: truncation lands exactly at 512 bytes with `…`; the budget is bytes not characters (1,000 four-byte scalars); a value inside the budget is returned whole and unmarked; sanitizing that *shrinks* a value does not falsely mark it truncated; the ellipsis is charged to the budget rather than added to it (the bug that made a "512-byte" bound render 515, caught by these tests during development); - `SubmitErrorBody` and `JobExecutionError` are bounded and stripped when reflected standalone as well as when boxed; - a huge `Retry-After` does not trap the description; - every payload-free case renders its stable label, and every case's rendering leads with the label `SDKLog.comfyErrorCaseName` emits. ## Residual Not fixed by this PR, and each one is a separate, self-contained piece of work: - **`ServerRejectionReason` still has no `CustomStringConvertible` of its own.** `ComfyError.description` renders it safely via a private helper, but a consumer who destructures `.serverRejected(reason:)` and writes `"\(reason)"` still gets default reflection, which prints `.other(_)`'s raw server string verbatim and unbounded. Deliberately left alone: the plan scoped the new renderers to `ComfyError` and `SubmitErrorBody`, and adding a public conformance to a second public type is a behaviour change worth deciding on its own. Fix is ~12 lines in `ComfyError.swift`, mirroring the private `rendered(_:)` helper this PR already has. - **`RouterError.loggable` still bounds by 512 *scalars*, not 512 bytes** (`RouterError.swift:434-441`). Because `.router(_)` delegates, `ComfyError.description` for that one case can therefore exceed the 512-byte budget every other case obeys — up to ~4× per field, across several fields. Left alone deliberately: the plan's instruction was to delegate, and `RouterError`'s rendering is shipped and covered by its own tests, so changing it is a separate behaviour change. The two units should probably be reconciled onto the shared `LogSafeText` helper. - ~~**The URL redaction is scoped to `NSURLErrorDomain`.**~~ **Closed by review follow-up:** the redaction is now keyed on the URL-bearing `userInfo` keys, so a CFNetwork- or POSIX-domain failure carrying the same URL is redacted too. What remains is narrower: a boxed error that carries a credential somewhere *other* than those keys is bounded and control-stripped but not redacted. I swept every `Error` type the SDK can box into `.network`/`.unknown` — 40 construction sites, resolving to `SubmitErrorBody`, `JobExecutionError`, `EmptyOutputError`, `OAuthTokenEndpointError`, `RouterInputSerializationError`, `WorkflowInputNotSupportedError`, `URLError`, POSIX `NSError` and caught `DecodingError`s — and none of the remainder carries a credential today: `OAuthTokenEndpointError` is scrubbed and clamped at construction, `RouterInputSerializationError` is a fixed string, `WorkflowInputNotSupportedError` carries a Swift case name, `EmptyOutputError` has no payload. A future boxed type could reintroduce the gap, and nothing enforces that it does not. - **`JobExecutionError` RETAINS its three server strings uncapped.** `exceptionType` / `exceptionMessage` / `nodeType` come straight off the `execution_error` frame and are bounded at *render* time only, so a hostile host's multi-megabyte `exception_message` stays alive for as long as the consumer holds the `ComfyError.unknown(underlying:)` that boxes it. `SubmitErrorBody` got a construction-time cap in this PR and this type did not: the log hazard (forged lines, crowded-out log) is closed either way, what is left is memory retention, and the fix is the same ~10 lines (`SubmitErrorBody.messageMaxLength`-style clamp in an initializer) applied to a second type. Self-contained follow-up. - **`ServerRejectionReason.other`'s payload is likewise bounded only at render time.** The stored string is uncapped, and it is extracted from the same fully buffered, size-unlimited submit body before `Transport.checkBody` throws `.serverRejected(reason: .other(message))` (`Sources/ComfySwiftSDK/Internal/Transport.swift:649`). Capping inside the enum would change a *public* associated value, so the placement would be that `Transport` throw site instead — a behaviour change to what a public error payload carries, which deserves its own PR rather than a rider on this one. (This is the retention half; the bullet above about `ServerRejectionReason` having no `CustomStringConvertible` is the separate rendering half.) - **Unexercised artifacts.** The plan this PR implements came from a prior read-only audit whose findings I could not read directly, so its count of "30+ `.unknown` sites" was re-derived rather than re-verified site by site: I measured 40 `.network`/`.unknown` construction sites across `Sources/ComfySwiftSDK/`. Nothing was run against a live Comfy server — the WebSocket credential leak is demonstrated against a synthetic `URLError` shaped like the one `URLSession` produces (correct keys, correct domain), not against a real `receive()` failure, so the claim that a real failure populates `NSURLErrorFailingURLErrorKey` is inferred from `URLSession`'s documented behaviour rather than observed. The consuming iOS app is out of scope and untouched. ## Provenance - **Authored by:** agent-work loop - **Verified:** `swift build`: clean, no warnings from changed files; `swift test --no-parallel`: 402 tests in 33 suites passed, 0 failed (397 after the round-1 follow-up, 394 at open) (two pre-existing Swift 6 `SendableClosureCaptures` warnings in `PollingFallbackTests` / `JobStatusEndpointMigrationTests`, both untouched by this PR); `python3 Scripts/contract/check_router_contract.py`: OK, 15 error types in sync with the vendored spec. - **Deviations:** two, both argued under "The judgment call worth reviewing" above — `NSURLErrorDomain` errors are rendered from chosen fields rather than a clamped `String(describing:)`, because the prescribed clamp measurably does not remove the credential it was meant to bound; and `JobExecutionError` received the same belt-and-braces `description` as `SubmitErrorBody`, which the plan's audit did not name. --- <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:42 +00:00