New SQS For Stripe Links#706
Conversation
WalkthroughThe pull request introduces a complete subscriber callback delivery system for Stripe payment links. A new Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import type { ValidLoggers } from "api/types.js"; | ||
|
|
||
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | ||
| { cidr: "10.0.0.0", mask: 8 }, |
|
|
||
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | ||
| { cidr: "10.0.0.0", mask: 8 }, | ||
| { cidr: "172.16.0.0", mask: 12 }, |
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | ||
| { cidr: "10.0.0.0", mask: 8 }, | ||
| { cidr: "172.16.0.0", mask: 12 }, | ||
| { cidr: "192.168.0.0", mask: 16 }, |
| { cidr: "172.16.0.0", mask: 12 }, | ||
| { cidr: "192.168.0.0", mask: 16 }, | ||
| { cidr: "127.0.0.0", mask: 8 }, | ||
| { cidr: "169.254.0.0", mask: 16 }, |
| { cidr: "127.0.0.0", mask: 8 }, | ||
| { cidr: "169.254.0.0", mask: 16 }, | ||
| { cidr: "0.0.0.0", mask: 8 }, | ||
| { cidr: "100.64.0.0", mask: 10 }, |
| }); | ||
|
|
||
| test("assertCallbackUrlIsExternal rejects hostnames that resolve privately", async () => { | ||
| lookupMock.mockResolvedValueOnce([{ address: "192.168.1.5", family: 4 }]); |
|
|
||
| test("assertCallbackUrlIsExternal accepts hostnames that resolve publicly", async () => { | ||
| lookupMock.mockResolvedValueOnce([ | ||
| { address: "93.184.216.34", family: 4 }, |
| test("assertCallbackUrlIsExternal accepts hostnames that resolve publicly", async () => { | ||
| lookupMock.mockResolvedValueOnce([ | ||
| { address: "93.184.216.34", family: 4 }, | ||
| { address: "2606:2800:220:1:248:1893:25c8:1946", family: 6 }, |
| }); | ||
|
|
||
| test("deliverSubscriberCallback posts signed JSON and logs success", async () => { | ||
| lookupMock.mockResolvedValueOnce([{ address: "93.184.216.34", family: 4 }]); |
| }); | ||
|
|
||
| test("deliverSubscriberCallback throws on non-2xx so SQS can retry", async () => { | ||
| lookupMock.mockResolvedValueOnce([{ address: "93.184.216.34", family: 4 }]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/api/functions/subscriberCallback.ts (1)
1-3: ⚡ Quick winUse
node:prefix for Node built-in imports.Static analysis flagged each of these imports. Prefer the explicit
node:protocol for built-ins to avoid accidental shadowing by a same-named npm package and improve ESM clarity.♻️ Proposed diff
-import { createHmac } from "crypto"; -import { lookup } from "dns/promises"; -import { isIP } from "net"; +import { createHmac } from "node:crypto"; +import { lookup } from "node:dns/promises"; +import { isIP } from "node:net";The same change should be applied to
src/api/routes/stripe.tsline 56 (import { randomBytes } from "crypto").🤖 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/api/functions/subscriberCallback.ts` around lines 1 - 3, Update the three built-in imports in subscriberCallback.ts to use the node: prefix — replace createHmac from "crypto", lookup from "dns/promises", and isIP from "net" with node:crypto, node:dns/promises, and node:net respectively; also apply the same change for randomBytes in src/api/routes/stripe.ts (replace "crypto" with "node:crypto") so built-in modules are explicitly imported and cannot be shadowed by npm packages.src/api/routes/stripe.ts (1)
99-116: ⚡ Quick winConsider sourcing
occurredAtfromevent.createdrather than enqueue time.
occurredAt: new Date().toISOString()records when the webhook handler enqueued the message, not when the Stripe event actually occurred. For subscribers reconciling state or detecting out-of-order delivery, the Stripe event'screatedtimestamp is more authoritative and stable across retries.- eventType: StripeLinkCallbackEventType; + eventType: StripeLinkCallbackEventType; + occurredAt: string; linkId: string;And at call sites, pass
occurredAt: new Date(event.created * 1000).toISOString().🤖 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/api/routes/stripe.ts` around lines 99 - 116, The payload's occurredAt is currently set to enqueue time; change it to use the Stripe event's created timestamp instead by reading event.created and converting to an ISO string (e.g. new Date(event.created * 1000).toISOString()) before assigning into callbackPayload.payload.occurredAt; update any call sites constructing this SQSPayload (referencing callbackPayload and AvailableSQSFunctions.StripeLinkSubscriberCallback) to pass the converted event.created value.
🤖 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/api/functions/subscriberCallback.ts`:
- Around line 6-14: Update the PRIVATE_IPV4_RANGES blocklist to include
additional special-use and non-routable IPv4 ranges so callbacks cannot target
them: add entries for 224.0.0.0/4, 240.0.0.0/4, 198.18.0.0/15, 192.0.0.0/24, and
the documentation ranges 192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24; ensure
the same { cidr: "...", mask: N } shape is used and update any unit tests or
validation logic that iterates over PRIVATE_IPV4_RANGES (e.g., the callback URL
validation path) to treat these as blocked destinations.
- Around line 28-45: The isPrivateIPv6 function misses IPv4-mapped and other
special IPv6 ranges; update isPrivateIPv6 to detect and handle the following:
treat "::" as private/unspecified, detect IPv4-mapped addresses in the
::ffff:0:0/96 space, extract the embedded IPv4 and call the existing
isPrivateIPv4(...) to reject mapped private IPv4s, and also add explicit checks
for NAT64 (64:ff9b::/96) and the documentation range (2001:db8::/32) alongside
the existing fc00::/7 and fe80::/10 checks; reference the isPrivateIPv6 function
and the isPrivateIPv4 helper when making these changes.
- Around line 81-98: assertCallbackUrlIsExternal currently validates DNS via
lookup() but allows undici's fetch to perform a separate DNS resolution
(TOCTOU); after validating the resolved addresses (resolved and
isPrivateIPv4/isPrivateIPv6 checks), pick a single validated IP and family
(e.g., resolved[0]) and force fetch to use that IP by creating an undici
Agent/dispatcher that overrides connect.lookup to return the pinned IP/family,
then pass that dispatcher to fetch (or use undici.request with the IP and set
the Host header to the original host) so the Host/SNI remain the hostname while
the TCP connection goes to the validated IP; update the fetch call in the code
that follows assertCallbackUrlIsExternal to use this dispatcher and ensure any
error paths still throw SubscriberCallbackBlockedError as before.
In `@src/api/sqs/handlers/stripeLinkSubscriberCallbackHandler.ts`:
- Line 14: The handler currently creates a new DynamoDBClient instance inside
the message processing path (the const dynamoClient = new DynamoDBClient({
region: genericConfig.AwsRegion }) in stripeLinkSubscriberCallbackHandler.ts);
change this so a single DynamoDBClient is created once and reused across
messages — either move the instantiation to module scope above the handler
function or accept a shared client via dependency injection into the handler —
and update any references to use that shared dynamoClient instead of
constructing a new one per callback.
- Around line 25-31: The handler in stripeLinkSubscriberCallbackHandler.ts
currently logs and returns when response.Items is not exactly one, which drops
messages; change the logic so that when response.Items is empty (response.Items
=== undefined || response.Items.length === 0) you throw an Error (including
context like payload.linkId) to trigger SQS retry, while still logging and
returning (or handling) the multi-item case (response.Items.length > 1) as
before; use the existing logger.warn and reference response.Items and
payload.linkId in the thrown error message so retries include useful context.
---
Nitpick comments:
In `@src/api/functions/subscriberCallback.ts`:
- Around line 1-3: Update the three built-in imports in subscriberCallback.ts to
use the node: prefix — replace createHmac from "crypto", lookup from
"dns/promises", and isIP from "net" with node:crypto, node:dns/promises, and
node:net respectively; also apply the same change for randomBytes in
src/api/routes/stripe.ts (replace "crypto" with "node:crypto") so built-in
modules are explicitly imported and cannot be shadowed by npm packages.
In `@src/api/routes/stripe.ts`:
- Around line 99-116: The payload's occurredAt is currently set to enqueue time;
change it to use the Stripe event's created timestamp instead by reading
event.created and converting to an ISO string (e.g. new Date(event.created *
1000).toISOString()) before assigning into callbackPayload.payload.occurredAt;
update any call sites constructing this SQSPayload (referencing callbackPayload
and AvailableSQSFunctions.StripeLinkSubscriberCallback) to pass the converted
event.created value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe6e7c7d-68dc-49dc-aa48-442dbafcb84d
📒 Files selected for processing (11)
src/api/functions/subscriberCallback.tssrc/api/routes/stripe.tssrc/api/sqs/handlers/index.tssrc/api/sqs/handlers/stripeLinkSubscriberCallbackHandler.tssrc/api/sqs/index.tssrc/common/types/sqsMessage.tssrc/common/types/stripe.tstests/live/stripe.test.tstests/unit/common/sqsMessage.test.tstests/unit/sqs/handlers/stripeLinkSubscriberCallbackHandler.test.tstests/unit/sqs/subscriberCallback.test.ts
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | ||
| { cidr: "10.0.0.0", mask: 8 }, | ||
| { cidr: "172.16.0.0", mask: 12 }, | ||
| { cidr: "192.168.0.0", mask: 16 }, | ||
| { cidr: "127.0.0.0", mask: 8 }, | ||
| { cidr: "169.254.0.0", mask: 16 }, | ||
| { cidr: "0.0.0.0", mask: 8 }, | ||
| { cidr: "100.64.0.0", mask: 10 }, | ||
| ]; |
There was a problem hiding this comment.
SSRF coverage gap: missing special-use IPv4 ranges.
The blocklist omits several non-routable / special-use ranges that should not be valid callback destinations:
224.0.0.0/4— multicast240.0.0.0/4— reserved/future198.18.0.0/15— benchmarking (RFC 2544)192.0.0.0/24— IETF protocol assignments192.0.2.0/24,198.51.100.0/24,203.0.113.0/24— documentation (RFC 5737)
Any of these reaching fetch could either fail unexpectedly or, on misconfigured networks, reach internal services.
🛡️ Proposed additions
const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [
{ cidr: "10.0.0.0", mask: 8 },
{ cidr: "172.16.0.0", mask: 12 },
{ cidr: "192.168.0.0", mask: 16 },
{ cidr: "127.0.0.0", mask: 8 },
{ cidr: "169.254.0.0", mask: 16 },
{ cidr: "0.0.0.0", mask: 8 },
{ cidr: "100.64.0.0", mask: 10 },
+ { cidr: "224.0.0.0", mask: 4 }, // multicast
+ { cidr: "240.0.0.0", mask: 4 }, // reserved
+ { cidr: "198.18.0.0", mask: 15 }, // benchmarking
+ { cidr: "192.0.0.0", mask: 24 }, // IETF protocol assignments
+ { cidr: "192.0.2.0", mask: 24 }, // TEST-NET-1
+ { cidr: "198.51.100.0", mask: 24 }, // TEST-NET-2
+ { cidr: "203.0.113.0", mask: 24 }, // TEST-NET-3
];📝 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.
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | |
| { cidr: "10.0.0.0", mask: 8 }, | |
| { cidr: "172.16.0.0", mask: 12 }, | |
| { cidr: "192.168.0.0", mask: 16 }, | |
| { cidr: "127.0.0.0", mask: 8 }, | |
| { cidr: "169.254.0.0", mask: 16 }, | |
| { cidr: "0.0.0.0", mask: 8 }, | |
| { cidr: "100.64.0.0", mask: 10 }, | |
| ]; | |
| const PRIVATE_IPV4_RANGES: { cidr: string; mask: number }[] = [ | |
| { cidr: "10.0.0.0", mask: 8 }, | |
| { cidr: "172.16.0.0", mask: 12 }, | |
| { cidr: "192.168.0.0", mask: 16 }, | |
| { cidr: "127.0.0.0", mask: 8 }, | |
| { cidr: "169.254.0.0", mask: 16 }, | |
| { cidr: "0.0.0.0", mask: 8 }, | |
| { cidr: "100.64.0.0", mask: 10 }, | |
| { cidr: "224.0.0.0", mask: 4 }, // multicast | |
| { cidr: "240.0.0.0", mask: 4 }, // reserved | |
| { cidr: "198.18.0.0", mask: 15 }, // benchmarking | |
| { cidr: "192.0.0.0", mask: 24 }, // IETF protocol assignments | |
| { cidr: "192.0.2.0", mask: 24 }, // TEST-NET-1 | |
| { cidr: "198.51.100.0", mask: 24 }, // TEST-NET-2 | |
| { cidr: "203.0.113.0", mask: 24 }, // TEST-NET-3 | |
| ]; |
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 7-7: IP addresses should not be hardcoded
Make sure using a hardcoded IP address 10.0.0.0 is safe here.See more on SonarQube Cloud
[notice] 8-8: IP addresses should not be hardcoded
Make sure using a hardcoded IP address 172.16.0.0 is safe here.See more on SonarQube Cloud
[notice] 9-9: IP addresses should not be hardcoded
Make sure using a hardcoded IP address 192.168.0.0 is safe here.See more on SonarQube Cloud
[notice] 11-11: IP addresses should not be hardcoded
Make sure using a hardcoded IP address 169.254.0.0 is safe here.See more on SonarQube Cloud
[notice] 13-13: IP addresses should not be hardcoded
Make sure using a hardcoded IP address 100.64.0.0 is safe here.See more on SonarQube Cloud
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 8-8: Make sure using a hardcoded IP address 172.16.0.0 is safe here.
[warning] 13-13: Make sure using a hardcoded IP address 100.64.0.0 is safe here.
[warning] 9-9: Make sure using a hardcoded IP address 192.168.0.0 is safe here.
[warning] 7-7: Make sure using a hardcoded IP address 10.0.0.0 is safe here.
[warning] 11-11: Make sure using a hardcoded IP address 169.254.0.0 is safe here.
🤖 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/api/functions/subscriberCallback.ts` around lines 6 - 14, Update the
PRIVATE_IPV4_RANGES blocklist to include additional special-use and non-routable
IPv4 ranges so callbacks cannot target them: add entries for 224.0.0.0/4,
240.0.0.0/4, 198.18.0.0/15, 192.0.0.0/24, and the documentation ranges
192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24; ensure the same { cidr: "...",
mask: N } shape is used and update any unit tests or validation logic that
iterates over PRIVATE_IPV4_RANGES (e.g., the callback URL validation path) to
treat these as blocked destinations.
| const isPrivateIPv6 = (ip: string): boolean => { | ||
| const normalized = ip.toLowerCase(); | ||
| if (normalized === "::1") { | ||
| return true; | ||
| } | ||
| if (normalized.startsWith("fc") || normalized.startsWith("fd")) { | ||
| return true; // fc00::/7 | ||
| } | ||
| if ( | ||
| normalized.startsWith("fe8") || | ||
| normalized.startsWith("fe9") || | ||
| normalized.startsWith("fea") || | ||
| normalized.startsWith("feb") | ||
| ) { | ||
| return true; // fe80::/10 | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
IPv6 SSRF bypass: IPv4-mapped addresses not detected.
isPrivateIPv6 does not handle IPv4-mapped IPv6 addresses (::ffff:0:0/96). A hostname with an AAAA record like ::ffff:127.0.0.1 or ::ffff:10.0.0.1 will return from isIP() as version 6 and pass this check, then fetch will route the request to the embedded IPv4 address — defeating the IPv4 private-range guard entirely.
Also worth covering for completeness:
::(unspecified)::ffff:0:0/96(IPv4-mapped)64:ff9b::/96(NAT64)2001:db8::/32(documentation)
🛡️ Suggested approach
When isIP(host) === 6, extract the embedded IPv4 from IPv4-mapped forms and re-run isPrivateIPv4 on it before doing the prefix-based checks. Example sketch:
const isPrivateIPv6 = (ip: string): boolean => {
const normalized = ip.toLowerCase();
+ if (normalized === "::" || normalized === "::1") {
+ return true;
+ }
+ // IPv4-mapped/compat: ::ffff:a.b.c.d or ::ffff:0:a.b.c.d
+ const mappedMatch = normalized.match(
+ /^::(?:ffff(?::0)?:)?(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/,
+ );
+ if (mappedMatch) {
+ return isPrivateIPv4(mappedMatch[1]);
+ }
- if (normalized === "::1") {
- return true;
- }
if (normalized.startsWith("fc") || normalized.startsWith("fd")) {
return true;
}
if (
normalized.startsWith("fe8") ||
normalized.startsWith("fe9") ||
normalized.startsWith("fea") ||
normalized.startsWith("feb")
) {
return true;
}
+ if (normalized.startsWith("2001:db8:") || normalized.startsWith("64:ff9b:")) {
+ return true;
+ }
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/api/functions/subscriberCallback.ts` around lines 28 - 45, The
isPrivateIPv6 function misses IPv4-mapped and other special IPv6 ranges; update
isPrivateIPv6 to detect and handle the following: treat "::" as
private/unspecified, detect IPv4-mapped addresses in the ::ffff:0:0/96 space,
extract the embedded IPv4 and call the existing isPrivateIPv4(...) to reject
mapped private IPv4s, and also add explicit checks for NAT64 (64:ff9b::/96) and
the documentation range (2001:db8::/32) alongside the existing fc00::/7 and
fe80::/10 checks; reference the isPrivateIPv6 function and the isPrivateIPv4
helper when making these changes.
| const resolved = await lookup(host, { all: true }); | ||
| if (resolved.length === 0) { | ||
| throw new SubscriberCallbackBlockedError( | ||
| `callbackUrl host ${host} did not resolve.`, | ||
| ); | ||
| } | ||
| for (const entry of resolved) { | ||
| if (entry.family === 4 && isPrivateIPv4(entry.address)) { | ||
| throw new SubscriberCallbackBlockedError( | ||
| `callbackUrl host ${host} resolves to private IPv4 ${entry.address}.`, | ||
| ); | ||
| } | ||
| if (entry.family === 6 && isPrivateIPv6(entry.address)) { | ||
| throw new SubscriberCallbackBlockedError( | ||
| `callbackUrl host ${host} resolves to private IPv6 ${entry.address}.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DNS rebinding SSRF mitigation Node.js undici fetch pinned IP
💡 Result:
To mitigate DNS rebinding attacks in SSRF scenarios when using Node.js undici fetch, create a custom Agent with a safe DNS lookup function in the connect options. This intercepts DNS resolution after the initial lookup, allowing you to validate the resolved IP address against private ranges, loopback, etc., before connecting. This effectively pins and checks the IP, preventing rebinding where a domain resolves to a safe IP first then rebinds to internal IPs.[1][2] Use a library like ipaddr.js for robust IP range classification.[1] Example code: import { Agent } from 'undici'; import { lookup } from 'dns'; import ipaddr from 'ipaddr.js'; const safeLookup = (hostname, options, callback) => { lookup(hostname, options, (err, address, family) => { if (err) { return callback(err, address, family); } if (!ipaddr.isValid(address)) { return callback(new Error('Invalid address'), address, family); } const ipObj = ipaddr.parse(address); if (ipObj.range!== 'unicast') { // Blocks private, loopback, link-local, etc. return callback(new Error('Not Allowed'), address, family); } return callback(null, address, family); }); }; const safeAgent = new Agent({ connect: { lookup: safeLookup, }, }); // Usage with fetch const response = await fetch('http://example.com', { dispatcher: safeAgent }); This approach is recommended in undici discussions for SSRF protection and directly counters DNS rebinding by validating every resolution.[1][3] Undici also has a DNS interceptor (added in recent versions via PR #3490, merged 2024) for caching/round-robin, but custom lookup in connect is the standard for security validation.[4][5] OWASP emphasizes resolving hostname, classifying IP, and rejecting unsafe ranges, which this implements.[2] For known/trusted hosts only, resolve once upfront, extract IP, and use a Client with that fixed IP as origin (bypassing DNS entirely).[6]
Citations:
- 1: SSRF protection in undici / native-node-fetch nodejs/undici#2019
- 2: https://owasp.org/www-community/pages/controls/SSRF_Prevention_in_Nodejs
- 3: Feature request: allow a custom dns lookup function to be supplied nodejs/undici#421
- 4: feat: add DNS interceptor nodejs/undici#3490
- 5: feat: use resolved ports in dns interceptor nodejs/undici#3786
- 6: https://github.com/nodejs/undici/blob/main/docs/docs/api/Client.md
🏁 Script executed:
# First, find the file and understand its full context
cat -n src/api/functions/subscriberCallback.ts | head -150Repository: acm-uiuc/core
Length of output: 5343
🏁 Script executed:
# See the complete fetch call and its options
sed -n '145,160p' src/api/functions/subscriberCallback.tsRepository: acm-uiuc/core
Length of output: 455
DNS rebinding TOCTOU vulnerability: validated host may resolve to a different IP at fetch time.
assertCallbackUrlIsExternal resolves the hostname via lookup() and validates the resolved addresses, but fetch (Node's undici) performs its own independent DNS resolution. An attacker controlling DNS for the callback host can return a public IP for the validation lookup and a private IP (with TTL=0) for the subsequent fetch — bypassing the entire IP allow-list.
Repro pattern: attacker registers a payment link with callbackUrl=https://evil.example/. When the webhook fires, evil.example returns 203.0.113.10 for the first resolution and 127.0.0.1 (or 169.254.169.254 for cloud metadata) for the second.
Recommended mitigation: pin the validated IP and direct fetch to it explicitly, preserving the Host header via SNI. With undici this looks like:
import { Agent } from "undici";
// after lookup() + validation, pick one address (e.g., resolved[0])
const dispatcher = new Agent({
connect: { lookup: (_host, opts, cb) => cb(null, validatedIp, validatedFamily) },
});
await fetch(callbackUrl, { /* ... */, dispatcher });Alternatively, use undici.request and supply the resolved IP directly while setting the Host header to the original hostname.
🤖 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/api/functions/subscriberCallback.ts` around lines 81 - 98,
assertCallbackUrlIsExternal currently validates DNS via lookup() but allows
undici's fetch to perform a separate DNS resolution (TOCTOU); after validating
the resolved addresses (resolved and isPrivateIPv4/isPrivateIPv6 checks), pick a
single validated IP and family (e.g., resolved[0]) and force fetch to use that
IP by creating an undici Agent/dispatcher that overrides connect.lookup to
return the pinned IP/family, then pass that dispatcher to fetch (or use
undici.request with the IP and set the Host header to the original host) so the
Host/SNI remain the hostname while the TCP connection goes to the validated IP;
update the fetch call in the code that follows assertCallbackUrlIsExternal to
use this dispatcher and ensure any error paths still throw
SubscriberCallbackBlockedError as before.
| export const stripeLinkSubscriberCallbackHandler: SQSHandlerFunction< | ||
| AvailableSQSFunctions.StripeLinkSubscriberCallback | ||
| > = async (payload, _metadata, logger) => { | ||
| const dynamoClient = new DynamoDBClient({ region: genericConfig.AwsRegion }); |
There was a problem hiding this comment.
Reuse a single DynamoDBClient instead of creating one per message.
Line 14 creates a new SDK client for each callback job. On higher SQS throughput, this adds avoidable connection/setup overhead.
Suggested change
+const dynamoClient = new DynamoDBClient({ region: genericConfig.AwsRegion });
+
export const stripeLinkSubscriberCallbackHandler: SQSHandlerFunction<
AvailableSQSFunctions.StripeLinkSubscriberCallback
> = async (payload, _metadata, logger) => {
- const dynamoClient = new DynamoDBClient({ region: genericConfig.AwsRegion });
const response = await dynamoClient.send(📝 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.
| const dynamoClient = new DynamoDBClient({ region: genericConfig.AwsRegion }); | |
| const dynamoClient = new DynamoDBClient({ region: genericConfig.AwsRegion }); | |
| export const stripeLinkSubscriberCallbackHandler: SQSHandlerFunction< | |
| AvailableSQSFunctions.StripeLinkSubscriberCallback | |
| > = async (payload, _metadata, logger) => { | |
| const response = await dynamoClient.send( |
🤖 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/api/sqs/handlers/stripeLinkSubscriberCallbackHandler.ts` at line 14, The
handler currently creates a new DynamoDBClient instance inside the message
processing path (the const dynamoClient = new DynamoDBClient({ region:
genericConfig.AwsRegion }) in stripeLinkSubscriberCallbackHandler.ts); change
this so a single DynamoDBClient is created once and reused across messages —
either move the instantiation to module scope above the handler function or
accept a shared client via dependency injection into the handler — and update
any references to use that shared dynamoClient instead of constructing a new one
per callback.
| if (!response.Items || response.Items.length !== 1) { | ||
| logger.warn( | ||
| { linkId: payload.linkId }, | ||
| "Stripe link not found for subscriber callback; dropping message.", | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do not permanently drop on “link not found” without retry.
Line 25 currently drops the message when the lookup result is not exactly one item. This can lose callbacks if the read is transiently stale (e.g., index propagation timing). Prefer retrying this condition by throwing, at least for the zero-item case.
Suggested change
- if (!response.Items || response.Items.length !== 1) {
+ if (!response.Items || response.Items.length === 0) {
+ logger.warn(
+ { linkId: payload.linkId },
+ "Stripe link not found yet for subscriber callback; retrying.",
+ );
+ throw new Error("Stripe link not found for callback delivery");
+ }
+ if (response.Items.length !== 1) {
logger.warn(
{ linkId: payload.linkId },
- "Stripe link not found for subscriber callback; dropping message.",
+ "Multiple Stripe links found for subscriber callback; dropping message.",
);
return;
}📝 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.
| if (!response.Items || response.Items.length !== 1) { | |
| logger.warn( | |
| { linkId: payload.linkId }, | |
| "Stripe link not found for subscriber callback; dropping message.", | |
| ); | |
| return; | |
| } | |
| if (!response.Items || response.Items.length === 0) { | |
| logger.warn( | |
| { linkId: payload.linkId }, | |
| "Stripe link not found yet for subscriber callback; retrying.", | |
| ); | |
| throw new Error("Stripe link not found for callback delivery"); | |
| } | |
| if (response.Items.length !== 1) { | |
| logger.warn( | |
| { linkId: payload.linkId }, | |
| "Multiple Stripe links found for subscriber callback; dropping message.", | |
| ); | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 25-25: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🤖 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/api/sqs/handlers/stripeLinkSubscriberCallbackHandler.ts` around lines 25
- 31, The handler in stripeLinkSubscriberCallbackHandler.ts currently logs and
returns when response.Items is not exactly one, which drops messages; change the
logic so that when response.Items is empty (response.Items === undefined ||
response.Items.length === 0) you throw an Error (including context like
payload.linkId) to trigger SQS retry, while still logging and returning (or
handling) the multi-item case (response.Items.length > 1) as before; use the
existing logger.warn and reference response.Items and payload.linkId in the
thrown error message so retries include useful context.
|
2 participants
Added a callback url SQS job for Stripe links, which allows a url to be pinged after a Stripe link has been processed.
Summary by CodeRabbit
Release Notes