Skip to content

[WRONG BRANCH] web-search: validate and sanitize citation sources before relaying #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+101 −23
Draft

[WRONG BRANCH] web-search: validate and sanitize citation sources before relaying #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions src/bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { encodeReasoningEnvelope, type ReasoningEnvelope } from "./responses/rea
import { rememberReasoningForCall } from "./responses/reasoning-replay-cache";
import { resolveStallTimeoutSec } from "./stall-timeout";
import { usageDisplayTotalTokens } from "./usage/totals";
import { appendSafeWebSearchSource, safeWebSearchSources } from "./web-search/sources";
import {
isTranslatorBudgetExceededError,
releaseTranslatedEvent,
Expand Down Expand Up @@ -1073,15 +1074,13 @@ export function bridgeToResponsesSSE(
});
currentWebSearch = { itemId: wsItemId2, eventId: event.id, outputIndex };
}
closeCurrentWebSearch(event.status ?? "completed", event.queries, event.sources);
const safeSources = safeWebSearchSources(event.sources ?? []);
closeCurrentWebSearch(event.status ?? "completed", event.queries, safeSources);
// Queue this search's sources for the next assistant message (dedup by URL).
if (event.sources) {
const seen = new Set(pendingWebSources.map(s => s.url));
for (const s of event.sources) {
if (!seen.has(s.url)) {
seen.add(s.url);
if (safeSources.length > 0) {
for (const s of safeSources) {
if (appendSafeWebSearchSource(pendingWebSources, s)) {
chargeValue(s, "tool_search_sources");
pendingWebSources.push(s);
}
}
}
Expand Down Expand Up @@ -1673,18 +1672,16 @@ function buildResponseJSONWithBudget(
if (currentSummaryReasoning) flushSummaryReasoning();
if (currentRawReasoning) flushRawReasoning();
flushToolCall();
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 } : {}),
});
Comment on lines +1675 to 1680

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 (e.sources) {
const seen = new Set(pendingWebSources.map(s => s.url));
for (const s of e.sources) {
if (!seen.has(s.url)) {
seen.add(s.url);
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);
}
Comment on lines +1681 to 1685

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

}
}
Expand Down
17 changes: 8 additions & 9 deletions src/web-search/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { appendSafeWebSearchSource } from "./sources";

/** A single web source backing the sidecar's answer. */
export interface WebSearchSource {
url: string;
Expand Down Expand Up @@ -30,8 +32,9 @@ interface OutputItem {
/** Push a `url_citation` annotation as a source, de-duplicated by URL. */
function collectAnnotation(ann: AnnotationLike | undefined, sources: WebSearchSource[], seen: Set<string>): void {
if (!ann || ann.type !== "url_citation" || typeof ann.url !== "string" || seen.has(ann.url)) return;
seen.add(ann.url);
sources.push({ url: ann.url, ...(ann.title ? { title: ann.title } : {}) });
if (appendSafeWebSearchSource(sources, { url: ann.url, ...(ann.title !== undefined ? { title: ann.title } : {}) })) {
seen.add(ann.url);
}
}

/**
Expand Down Expand Up @@ -101,8 +104,7 @@ function extractTrailingSources(text: string): { text: string; sources: WebSearc
const title = cleanTitle(inlinePrefix) || (pendingTitle ? cleanTitle(pendingTitle) : "");
pendingTitle = null;
if (seen.has(url)) continue;
seen.add(url);
sources.push(title ? { url, title } : { url });
if (appendSafeWebSearchSource(sources, title ? { url, title } : { url })) seen.add(url);
}
if (sources.length === 0) return { text, sources: [] };
// Keep text before the header AND any prose after the consumed source lines.
Expand Down Expand Up @@ -200,19 +202,16 @@ export async function parseSidecarSSE(response: Response): Promise<WebSearchResu
|| acc.deltaText;
// Merge sources from the final output[] and the streaming annotation events.
const sources = [...(acc.final?.sources ?? [])];
const seenMerge = new Set(sources.map(s => s.url));
for (const s of acc.streamSources) {
if (!seenMerge.has(s.url)) { seenMerge.add(s.url); sources.push(s); }
appendSafeWebSearchSource(sources, s);
}
// Hosted web_search usually omits url_citation annotations and lists sources in a trailing
// `Sources:` markdown block instead. Pull those out (and strip the block from the answer so the
// tool_result renderer doesn't print sources twice). Annotation titles win; text-block titles
// only fill a gap. URL-deduped against annotation sources.
const { text: body, sources: textSources } = extractTrailingSources(typeof text === "string" ? text : "");
for (const s of textSources) {
if (seenMerge.has(s.url)) continue;
seenMerge.add(s.url);
sources.push(s);
appendSafeWebSearchSource(sources, s);
}
const finalText = textSources.length > 0 ? body : (typeof text === "string" ? text : "");
if (!finalText.trim() && acc.error) return { text: "", sources, error: acc.error };
Expand Down
52 changes: 52 additions & 0 deletions src/web-search/sources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
export interface SafeWebSearchSource {
url: string;
title?: string;
}

export const MAX_WEB_SEARCH_SOURCES = 20;
export const MAX_WEB_SEARCH_URL_BYTES = 2_048;
export const MAX_WEB_SEARCH_TITLE_BYTES = 256;
export const MAX_WEB_SEARCH_SOURCE_BYTES = 16_384;

const CONTROL_CHARACTERS = /[\u0000-\u001f\u007f-\u009f]/;
const encoder = new TextEncoder();

function byteLength(value: string): number {
return encoder.encode(value).byteLength;
}

/** Add a client-safe citation while enforcing the per-message count and byte budgets. */
export function appendSafeWebSearchSource(target: SafeWebSearchSource[], source: unknown): boolean {
if (target.length >= MAX_WEB_SEARCH_SOURCES || !source || typeof source !== "object") return false;
const candidate = source as { url?: unknown; title?: unknown };
if (typeof candidate.url !== "string"
|| candidate.url.length === 0
|| CONTROL_CHARACTERS.test(candidate.url)
|| byteLength(candidate.url) > MAX_WEB_SEARCH_URL_BYTES
|| target.some(existing => existing.url === candidate.url)) return false;

let parsed: URL;
try { parsed = new URL(candidate.url); } catch { return false; }
if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false;
Comment on lines +28 to +30

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

Suggested change
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


let title: string | undefined;
if (candidate.title !== undefined) {
if (typeof candidate.title !== "string"
|| candidate.title.length === 0
|| CONTROL_CHARACTERS.test(candidate.title)
|| byteLength(candidate.title) > MAX_WEB_SEARCH_TITLE_BYTES) return false;
title = candidate.title;
}

const next = title ? { url: candidate.url, title } : { url: candidate.url };
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;
Comment on lines +42 to +43

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Measure the serialized citation budget

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 👍 / 👎.

target.push(next);
return true;
}

export function safeWebSearchSources(sources: readonly unknown[]): SafeWebSearchSource[] {
const safe: SafeWebSearchSource[] = [];
for (const source of sources) appendSafeWebSearchSource(safe, source);
return safe;
}
30 changes: 30 additions & 0 deletions tests/bridge.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,36 @@ describe("Responses bridge web_search_call native item", () => {
type: "url_citation", url: "https://nodejs.org", title: "Node.js", start_index: 0, end_index: 0,
}]);
});

test("unsafe and oversized web-search citations are not relayed to clients", async () => {
const sources = [
{ url: "javascript:alert(1)", title: "unsafe" },
{ url: "data:text/html,unsafe" },
{ url: "https://control.test/path\u0000" },
{ url: "https://title.test", title: "bad\u0001title" },
{ url: "https://oversized.test", title: "x".repeat(257) },
{ url: "https://safe.test/docs", title: "Safe docs" },
...Array.from({ length: 25 }, (_, index) => ({ url: `https://safe.test/${index}` })),
];
const events: AdapterEvent[] = [
{ type: "web_search_call_end", id: "ws_safe", queries: ["docs"], sources },
{ type: "text_delta", text: "answer" },
{ type: "done" },
];
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);
Comment on lines +1001 to +1013

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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

});
});

describe("Responses bridge stopReason threading (issue #246)", () => {
Expand Down
Loading