fix(portal): validate webhook URLs to block SSRF write path (VULN-6369) - #2121
Conversation
Partner action webhook_uri is fetched server-side by app-backend, so an attacker-controlled value is a blind SSRF sink. The api_key Hasura write path was already closed (PR #2077); this closes the service-role UI write path, where the old check skipped validation on staging and let any https host through (incl. 169.254.169.254, 127.0.0.1, and private/IPv6 ranges). Add validateWebhookUrl() (HTTPS-only plus a loopback/private/link-local/metadata/IPv6/IPv4-mapped/alternate-encoding denylist), wire it into the 4 action form-schemas, and add unit + schema + api_key-update regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 690b81b02a
ℹ️ 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".
| if (!hostname) return false; | ||
|
|
||
| // Block localhost by name (and any *.localhost subdomain). | ||
| if (hostname === "localhost" || hostname.endsWith(".localhost")) { |
There was a problem hiding this comment.
For webhook URLs such as https://localhost./hook or https://foo.localhost./hook, new URL(...).hostname preserves the trailing root dot (localhost.), so this condition does not match and the value falls through as an allowed DNS name. In environments where the backend resolver treats the root-qualified .localhost. special-use name as loopback, this leaves a direct SSRF bypass despite the new localhost denylist; normalize or strip a single trailing dot before this suffix check.
Useful? React with 👍 / 👎.
| const dotted = addr.match(/^::(ffff:)?(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); | ||
| if (dotted) return parseIPv4Octets(dotted[2]); | ||
|
|
||
| const mapped = addr.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); |
There was a problem hiding this comment.
When the input is an IPv4-compatible literal like https://[::127.0.0.1]/hook, the URL parser canonicalizes the hostname to ::7f00:1; the dotted branch never sees it, and this regex only handles the ::ffff: mapped form. That means isBlockedIPv6() accepts deprecated IPv4-compatible loopback/link-local targets even though the comment says they are covered, so add handling for the ::hhhh:hhhh compatible form or block ::/96 before allowing the IPv6 literal.
Useful? React with 👍 / 👎.
…SSRF bypasses (VULN-6369) Codex review of PR #2121 found two write-layer bypasses in validateWebhookUrl: (1) a trailing root dot (localhost., foo.localhost.) is preserved by URL.hostname and slipped past the localhost denylist; (2) IPv4-compatible IPv6 (::127.0.0.1) canonicalizes to ::7f00:1, which the ::ffff:-only mapped check missed. Strip a trailing dot during host normalization and block the whole ::/96 range; add regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f1946fad
ℹ️ 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".
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 private | ||
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 private | ||
|
|
||
| return false; |
There was a problem hiding this comment.
At this fallthrough, any literal IPv4 address outside the handful of ranges above is treated as a public webhook target, so values like https://198.18.0.1/, https://224.0.0.1/, or https://255.255.255.255/ still pass the new SSRF guard even though they are not public unicast addresses and may be routed or handled inside the local network. For this write-layer guard to match its “non-public/internal” contract, use a complete special-purpose IPv4 range check instead of only RFC1918/link-local/CGNAT.
Useful? React with 👍 / 👎.
| * app-backend, which is the component that performs the request. | ||
| */ | ||
| export const validateWebhookUrl = (candidate: string): boolean => { | ||
| const value = candidate?.trim(); |
There was a problem hiding this comment.
Because the value is trimmed before containsControlCharacter() runs, a webhook URL with leading or trailing controls such as https://collector.example.com/ passes validation, while the form schema still stores the original untrimmed string. That leaves the request/header-smuggling case this helper is trying to reject dependent on app-backend normalizing the value in exactly the same way; validate the raw candidate for controls before trimming or persist the normalized URL.
Useful? React with 👍 / 👎.
…m (VULN-6369) Address Codex re-review of PR #2121: (1) isBlockedIPv4 now rejects all non-public-unicast ranges (multicast 224/4, reserved+broadcast 240/4, benchmarking 198.18/15, TEST-NETs, 192.0.0/24, 6to4) instead of only RFC1918/loopback/link-local/CGNAT; (2) control-character check runs on the raw candidate before trim() so a leading/trailing newline can no longer be stripped past the smuggling guard while the schema persists the untrimmed value. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
PR Type
Description
Partner action
webhook_uriis fetched server-side by app-backend, so an attacker-controlled value is a blind SSRF sink (VULN-6369 / CE25-C014). Theapi_keyHasura write path was already closed (PR #2077); this closes the remaining service-role UI write path, where the old check skipped validation entirely on the staging deployment and let anyhttps://host through — including169.254.169.254,127.0.0.1, and private/IPv6 ranges. This addsvalidateWebhookUrl()(HTTPS-only, plus a loopback / private / link-local / cloud-metadata / IPv6 / IPv4-mapped / alternate-encoding denylist) and wires it into the four action form-schemas (Portal + PortalV3, create + update), backed by unit, schema, andapi_key-update regression tests. DNS-rebinding and resolution-time egress defense must still be added inapp-backend-main(cross-repo), which is the service that actually resolves and fetches the URL.Checklist
🤖 Generated with Claude Code