[GH-ISSUE #15] Sanitize server-influenced file extension before composing the cache path #1

New issue

Closed

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

bsakaguchi commented

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

Owner

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

Surfaced by CodeRabbit on #10 (the finding is about pre-existing code, so it's tracked here rather than fixed in that comment-only PR).

`Transport` composes the on-disk destination for a downloaded output as:

```swift
.appendingPathComponent("(UUID().uuidString).(suggestedExtension)")
```

`suggestedExtension` is derived from the server's response (content type / filename). If it ever contains `/`, `\`, or `..`, the interpolation can shape the path outside the intended `UUID.ext` filename and write to an unexpected location in the caches directory.

Fix: normalize before use — lowercase, take the last path segment, and constrain to a short alphanumeric whitelist, falling back to `bin`:

```swift
let safe = suggestedExtension.lowercased().split(separator: "/").last.map(String.init) ?? "bin"
let ext = safe.range(of: #"^[a-z0-9]{1,10}$"#, options: .regularExpression) != nil ? safe : "bin"
```

Acceptance: a malicious/garbled extension cannot escape the `UUID().` filename shape; add a unit test with `../`-style and slash-bearing extensions.

Originally created by @mattmillerai on GitHub (Jun 21, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-swift-sdk/issues/15 Surfaced by CodeRabbit on #10 (the finding is about pre-existing code, so it's tracked here rather than fixed in that comment-only PR). \`Transport\` composes the on-disk destination for a downloaded output as: \`\`\`swift .appendingPathComponent("\(UUID().uuidString).\(suggestedExtension)") \`\`\` \`suggestedExtension\` is derived from the server's response (content type / filename). If it ever contains \`/\`, \`\\\`, or \`..\`, the interpolation can shape the path outside the intended \`UUID.ext\` filename and write to an unexpected location in the caches directory. **Fix:** normalize before use — lowercase, take the last path segment, and constrain to a short alphanumeric whitelist, falling back to \`bin\`: \`\`\`swift let safe = suggestedExtension.lowercased().split(separator: "/").last.map(String.init) ?? "bin" let ext = safe.range(of: #"^[a-z0-9]{1,10}$"#, options: .regularExpression) != nil ? safe : "bin" \`\`\` **Acceptance:** a malicious/garbled extension cannot escape the \`UUID().<ext>\` filename shape; add a unit test with \`../\`-style and slash-bearing extensions.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

@coderabbitai[bot] commented on GitHub (Jun 21, 2026):

🔗 Related PRs

Comfy-Org/ComfySwiftSDK#2 - fix(transport): treat POSIX socket-drop errors as transient so jobs recover via polling [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 (Jun 21, 2026):  <details> <summary>🔗 Related PRs</summary> Comfy-Org/ComfySwiftSDK#2 - fix(transport): treat POSIX socket-drop errors as transient so jobs recover via polling [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:50 +00:00

[PR #1] [MERGED] feat: X-Comfy-Client traceability header (+ fix source-level test after extraction) #6