[WRONG BRANCH] web-search: validate and sanitize citation sources before relaying - #183
[WRONG BRANCH] web-search: validate and sanitize citation sources before relaying#183luvs01 wants to merge 1 commit into
Conversation
|
This pull request currently targets @luvs01 Please retarget this PR to Its title has been prefixed with This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughWeb-search source handling now validates URLs and titles, rejects unsafe or oversized entries, deduplicates sources, enforces count and payload limits, and applies the same rules to parsed, streaming, and batch Responses output. ChangesWeb-search source handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebSearchParser
participant SourceSanitizer
participant Bridge
participant ResponsesOutput
participant TranslatorBudget
WebSearchParser->>SourceSanitizer: append parsed and streamed sources
Bridge->>SourceSanitizer: sanitize streaming or batch sources
SourceSanitizer-->>Bridge: return accepted, deduplicated sources
Bridge->>ResponsesOutput: emit accepted sources and citations
Bridge->>TranslatorBudget: charge accepted sources only
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bridge.ts`:
- Around line 1675-1680: Wrap the web_search_call_end switch case body in braces
so the safeSources declaration is scoped within its case and satisfies
noSwitchDeclarations; preserve the existing pushOutput behavior.
- Around line 1681-1685: Release the retained tool_search_sources budget when
pending web sources are transferred into annotations and the completed message
is retained by pushOutput. Update the relevant flushText path, matching the
release behavior already used by takeWebAnnotations, while preserving the
existing charge in appendSafeWebSearchSource.
In `@src/web-search/sources.ts`:
- Around line 28-30: Update the URL validation logic around parsed in the
citation URL function to reject candidates when parsed.username or
parsed.password is non-empty, before preserving or serializing the URL. Continue
accepting only http: and https: URLs without credentials.
In `@tests/bridge.test.ts`:
- Around line 1001-1013: Extend the test around bridgeToResponsesSSE and
buildResponseJSON to locate the streaming and batch web_search_call output items
and assert their sources equal the sanitized source list, including the expected
safe URL fields. Ensure both paths are validated so forwarding raw event.sources
cannot satisfy the existing assistant-message annotation assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f2e8a79-fcdb-4a90-aed8-f5b5ca850b15
📒 Files selected for processing (4)
src/bridge.tssrc/web-search/parse.tssrc/web-search/sources.tstests/bridge.test.ts
| const safeSources = safeWebSearchSources(e.sources ?? []); | ||
| pushOutput({ | ||
| type: "web_search_call", id: `ws_${uuid()}`, status: e.status ?? "completed", | ||
| action: webSearchAction(e.queries), | ||
| ...(e.sources && e.sources.length > 0 ? { sources: e.sources } : {}), | ||
| ...(safeSources.length > 0 ? { sources: safeSources } : {}), | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wrap this switch case in a block.
Line 1675 declares safeSources directly in a switch case. Biome reports lint/correctness/noSwitchDeclarations for this declaration.
Wrap the web_search_call_end case body in braces so the lint check passes.
🧰 Tools
🪛 Biome (2.5.6)
[error] 1675-1675: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bridge.ts` around lines 1675 - 1680, Wrap the web_search_call_end switch
case body in braces so the safeSources declaration is scoped within its case and
satisfies noSwitchDeclarations; preserve the existing pushOutput behavior.
Source: Linters/SAST tools
| if (safeSources.length > 0) { | ||
| for (const s of safeSources) { | ||
| if (appendSafeWebSearchSource(pendingWebSources, s)) { | ||
| budget?.chargeRetained(bytesOf(JSON.stringify(s)), { kind: "tool_search_sources" }); | ||
| pendingWebSources.push(s); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release batch pending-source budget after annotation transfer.
Line 1684 charges each accepted source as tool_search_sources. flushText later clears pendingWebSources after it copies the sources into annotations, but it does not release this retained charge.
Repeated web-search and message cycles can exhaust the translator budget even when the final batch response fits. Release the pending-source bytes when pushOutput retains the completed message item, as the streaming path does in takeWebAnnotations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bridge.ts` around lines 1681 - 1685, Release the retained
tool_search_sources budget when pending web sources are transferred into
annotations and the completed message is retained by pushOutput. Update the
relevant flushText path, matching the release behavior already used by
takeWebAnnotations, while preserving the existing charge in
appendSafeWebSearchSource.
| let parsed: URL; | ||
| try { parsed = new URL(candidate.url); } catch { return false; } | ||
| if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject credentials in citation URLs.
Lines 28-30 accept https://token:secret@example.test/. The function then preserves that URL and serializes the credentials to clients.
Reject URLs when parsed.username or parsed.password is non-empty.
Proposed fix
- if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false;
+ if (
+ (parsed.protocol !== "http:" && parsed.protocol !== "https:")
+ || parsed.username
+ || parsed.password
+ ) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let parsed: URL; | |
| try { parsed = new URL(candidate.url); } catch { return false; } | |
| if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false; | |
| let parsed: URL; | |
| try { parsed = new URL(candidate.url); } catch { return false; } | |
| if ( | |
| (parsed.protocol !== "http:" && parsed.protocol !== "https:") | |
| || parsed.username | |
| || parsed.password | |
| ) return false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/web-search/sources.ts` around lines 28 - 30, Update the URL validation
logic around parsed in the citation URL function to reject candidates when
parsed.username or parsed.password is non-empty, before preserving or
serializing the URL. Continue accepting only http: and https: URLs without
credentials.
Source: Path instructions
| const frames = await collectSse(bridgeToResponsesSSE(replay(events), "routed/model")); | ||
| const done = frames.find(f => f.event === "response.output_item.done" | ||
| && (f.data.item as Record<string, unknown>)?.type === "message"); | ||
| const streamingPart = ((done!.data.item as Record<string, unknown>).content as Record<string, unknown>[])[0]; | ||
| expect((streamingPart.annotations as unknown[]).slice(0, 1)).toEqual([ | ||
| { type: "url_citation", url: "https://safe.test/docs", title: "Safe docs", start_index: 0, end_index: 0 }, | ||
| ]); | ||
| expect(streamingPart.annotations).toHaveLength(20); | ||
|
|
||
| const json = buildResponseJSON(events, "routed/model"); | ||
| const message = (json.output as Record<string, unknown>[]).find(item => item.type === "message")!; | ||
| const batchPart = (message.content as Record<string, unknown>[])[0]; | ||
| expect(batchPart.annotations).toEqual(streamingPart.annotations); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert sanitized sources on web_search_call output items.
The test only checks assistant-message annotations. web_search_call.sources is also sent to clients.
If closeCurrentWebSearch or the batch output path later forwards raw event.sources, these assertions can still pass. Assert the sanitized source list on the streaming and batch web_search_call items too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bridge.test.ts` around lines 1001 - 1013, Extend the test around
bridgeToResponsesSSE and buildResponseJSON to locate the streaming and batch
web_search_call output items and assert their sources equal the sanitized source
list, including the expected safe URL fields. Ensure both paths are validated so
forwarding raw event.sources cannot satisfy the existing assistant-message
annotation assertions.
Source: Path instructions
![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: 776148183d
ℹ️ 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 usedBytes = target.reduce((sum, item) => sum + byteLength(item.url) + byteLength(item.title ?? ""), 0); | ||
| if (usedBytes + byteLength(next.url) + byteLength(next.title ?? "") > MAX_WEB_SEARCH_SOURCE_BYTES) return false; |
There was a problem hiding this comment.
When an untrusted URL or title contains JSON-escaped characters such as quotes or backslashes, this sums the raw UTF-8 strings even though both bridge paths relay their JSON serialization. For example, eight accepted HTTPS URLs containing repeated quotes total 16,128 bytes here but serialize to 32,217 bytes, bypassing the intended 16 KiB aggregate cap. Compute the budget from the serialized representation, including citation wrapper overhead, or retain and measure a normalized URL.
Useful? React with 👍 / 👎.
1 participant
Motivation
url_citationannotations and introducing unsafe URL schemes, control characters, oversized titles, or excessive/oversized source lists.Description
src/web-search/sources.tsthat enforces HTTP(S) scheme, rejects control characters, bounds URL/title byte lengths, caps per-message count and aggregate bytes, and deduplicates by URL viaappendSafeWebSearchSource/safeWebSearchSources.src/web-search/parse.tsso structuredurl_citationannotations and trailing MarkdownSources:lists are filtered before being collected as sources.src/bridge.ts) for both streaming and non-streaming paths by converting incomingweb_search_call_endsourcesthrough the safe filter and using the append helper when queuingpendingWebSourcesfor the next assistant message.tests/bridge.test.tsthat asserts unsafe schemes, control characters, oversized titles, and excess entries are not relayed to streaming or non-streaming clients.Testing
bun test tests/bridge.test.ts, which passed including the new regression for filtered citations.bun run typecheck(bun x tsc --noEmit), which succeeded with no type errors.bun run privacy:scan, which completed successfully in this environment.bun run testrun; focused tests and typecheck are green, and an unrelated environment-sensitive permission test surfaced in the full suite (an EACCES expectation that depends on non-root runner conditions) that is not caused by these changes.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests