fix: address DeepSeek provider roast findings#3
Conversation
![verdict-0528[bot]](https://avatars.githubusercontent.com/in/3840151?s=60&v=4){kind=small}
There was a problem hiding this comment.
What this PR does: Hardens the DeepSeek/OpenRouter/custom provider selection, adds prompt redaction and private-network URL blocking, tightens event/recipe schemas with strictness and size limits, and improves render streaming reliability and asset matching.
Risk areas:
- Mixing stream data handler with pipeline in render result handling may cause data loss.
- The custom provider model default falls back to an OpenRouter model when the user forgets to set SUPERCUT_MODEL, which can lead to confusing API errors.
- Default DNS resolution failure in URL policy allows private networks only when DNS lookup fails; this is an edge case unlikely in practice but worth noting.
Verdict:⚠️ Minor concerns
| @@ -95,35 +111,37 @@ export async function renderTake(opts: RenderOptions): Promise<RenderResult> { | |||
| res.end(); | |||
| } | |||
There was a problem hiding this comment.
🟠 P1 (High): The /result handler mixes a req.on('data') event listener with a pipeline(req, ...) call. The data event forces the stream into flowing mode, which will consume chunks before pipeline can process them. This will cause the encoded H.264 stream to be lost or corrupted. Remove the manual data handler and use only pipeline, or implement the size check inside a transform stream attached to the pipeline.
| } | ||
|
|
||
| const client = new OpenRouterClient({ | ||
| const client = new OpenAICompatibleClient({ |
There was a problem hiding this comment.
🟢 P3 (Low): When provider is custom and no model is supplied via environment or override, the fallback model defaults to anthropic/claude-sonnet-4.6 (the OpenRouter default). Custom endpoints rarely host this model, leading to confusing API errors. Consider requiring SUPERCUT_MODEL when SUPERCUT_PROVIDER=custom and no override is given.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2732c3c429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const h = hostname.toLowerCase(); | ||
| if (h === "localhost" || h.endsWith(".localhost")) return true; | ||
| if (h === "0.0.0.0") return true; | ||
| if (isIP(h) === 6) return h === "::1" || h.startsWith("fc") || h.startsWith("fd") || h.startsWith("fe80:"); |
There was a problem hiding this comment.
For IPv6 literal URLs such as http://[::1]/ or http://[fd00::1]/, Node's URL.hostname value is bracketed, so isIP(h) returns 0 here; the subsequent DNS lookup fails and resolvesPrivate returns false, allowing localhost/ULA/link-local IPv6 navigation even when allowPrivateNetwork is false. Strip brackets or otherwise parse IPv6 literals before the isIP checks so the default private-network block cannot be bypassed.
Useful? React with 👍 / 👎.
|
Superseded by #4, which folds in this PR's good parts (typed provider config, streaming render, security modules, schema bounds, CI, vitest v4) with the three regressions corrected (localhost allowed by default, no forced --yes, relaxed storyboard) and builds the comprehension + submit + frame-the-result work on top. |
1 participant
Summary
--modela request-scoped overrideVerification
npm run buildnpm run test:fastnpm run test:e2enpm audit --audit-level=moderate