Skip to content

fix: pin the SSRF transport to every validated address and retry connection failures (v2.71.0) - #1010

Merged
czlonkowski merged 2 commits into
mainfrom
fix/ssrf-pinned-agent-resilience
Aug 18, 2026
Merged

fix: pin the SSRF transport to every validated address and retry connection failures (v2.71.0)#1010
czlonkowski merged 2 commits into
mainfrom
fix/ssrf-pinned-agent-resilience

Conversation

@czlonkowski

Copy link
Copy Markdown
Owner

Fixes #978. Fixes #989. Fixes #990.

Thanks to @ConnorCloze (#978) and @Boulevard-Dreams (#989/#990) for unusually precise root-cause analyses — both traced the installed package to the exact lines, and both were confirmed verbatim against main.

The problem

The DNS-pinning SSRF protection (GHSA-cmrh-wvq6-wm9r) resolved the API hostname with a single-answer dns.lookup(), pinned every connection to that one address, cached it for the life of the process, and had no retry path (N8N_API_MAX_RETRIES was stored and never read). Consequences:

The fix

ssrf-protection.tsvalidateWebhookUrl resolves {all: true} and validates every record via a shared per-address policy helper, failing closed if any record is disallowed. This also closes the mixed-record variant of DNS rebinding, and deliberately applies to cloud-metadata addresses in any record position in permissive mode too (previously only the first answer was ever checked). createPinnedAgents pins the full validated set: the custom lookup hands back all candidates, and autoSelectFamily (guarded for old Node) lets net.connect try each in turn — every candidate has already passed validation, so the pinning guarantee is unchanged.

n8n-api-client.ts — the cached pinned agents get a 60s TTL (stamped at dispatch to prevent a re-resolution stampede, refreshed on fulfillment only while still current) and are invalidated after any terminal NO_RESPONSE. maxRetries now drives a real retry path in the response interceptor: pre-connection failures (ECONNREFUSED/EHOSTUNREACH/ENETUNREACH/ENOTFOUND/EAI_AGAIN, including AggregateError members) retry any method; ECONNRESET/ETIMEDOUT/ECONNABORTED retry only GET/HEAD, so a non-idempotent request that may have reached the server is never re-executed. Each retry clears the pinned cache first, so the retried attempt resolves fresh DNS — this is what heals the rotated-CDN-edge case mid-session.

n8n-errors.tsNO_RESPONSE now names what failed: No response from n8n server (ECONNREFUSED 127.0.0.1:5678, ECONNREFUSED [::1]:5678), reading detail from the error, its cause, or each AggregateError member; the user-facing message carries it through. The trigger handlers also summarize AggregateError members instead of surfacing an empty message.

Review notes

Gauntlet: code-reviewer + Codex (adversarial), both confirmed the core invariant — no connection can reach an address that did not pass validation — and both verified the axios re-issue mechanics against the installed axios 1.18.1 (__retryCount survives mergeConfig; serialized bodies aren't double-transformed; recursion bounded). Applied from review: the TTL stampede fix, the cause fallback in error details, non-idempotent default for a missing method, AggregateError messages in trigger handlers, and coverage for autoSelectFamily wiring, permissive-mode fail-close, concurrent-expiry, and fresh-DNS-on-retry. Declined: retrying DNS-resolution failures inside URL validation (fail-fast is intentional; the cache resets so the next call re-resolves).

Behavior changes to be aware of: a hostname whose record set mixes allowed and disallowed addresses now fails closed even when the allowed one comes first (stricter; previously only the first answer was checked), and permissive mode now rejects tunneled/metadata records in any position.

Verification

  • npm run typecheck clean.
  • Full unit suite in the worktree: 170 files, 5762 passed, 35 skipped (baseline), 0 failures.
  • New coverage: multi-address validation, fail-closed mixed records (strict + permissive), pinned-set lookup shapes, autoSelectFamily on createConnection, TTL expiry + concurrent-expiry single-flight, cache invalidation on NO_RESPONSE, retry matrix (GET/POST × refused/reset/timeout), fresh DNS on retried attempts, enriched error details incl. cause and code-less aggregates.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings August 18, 2026 16:39
Comment thread src/utils/n8n-errors.ts Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Improves resilience of the GHSA-cmrh-wvq6-wm9r SSRF DNS-pinning transport used by the n8n management client and trigger handlers by validating/pinning all DNS answers (fail-closed on mixed disallowed records), enabling multi-address connection fallback, adding TTL-based re-resolution + retry-on-connection-failure behavior, and enriching NO_RESPONSE errors with attempted address details.

Changes:

  • Update SSRF validation/agent pinning to resolve { all: true }, validate every answer, and pin the full validated set with autoSelectFamily fallback.
  • Add pinned-agent TTL + invalidate-on-failure behavior and implement connection-level retries driven by N8N_API_MAX_RETRIES.
  • Improve diagnosability of NO_RESPONSE (including AggregateError member summaries) and add unit coverage for the new behaviors.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/ssrf-protection.ts Validate every DNS answer and pin a multi-address set with Happy-Eyeballs fallback.
src/services/n8n-api-client.ts Add pinned-agent TTL, invalidate on failures, and implement safe connection-level retries.
src/utils/n8n-errors.ts Enrich NO_RESPONSE messaging with connection failure details (incl. AggregateError members).
src/triggers/handlers/chat-handler.ts Use multi-address pinned agents; improve empty-message AggregateError reporting.
src/triggers/handlers/form-handler.ts Use multi-address pinned agents; improve empty-message AggregateError reporting.
tests/unit/utils/ssrf-protection.test.ts Add/adjust coverage for multi-address validation/pinning and autoSelectFamily wiring.
tests/unit/services/n8n-api-client.test.ts Add coverage for TTL, invalidation, fresh-DNS-on-retry, and retry matrix.
tests/unit/utils/n8n-errors.test.ts Add coverage for NO_RESPONSE detail formatting and user-facing message propagation.
CHANGELOG.md Document the behavioral fixes in 2.71.0.
package.json Bump version to 2.71.0.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +462 to +466
static createPinnedAgents(addresses: Array<{ address: string; family: 4 | 6 }>): PinnedAgents {
if (!addresses || addresses.length === 0) {
throw new Error('createPinnedAgents requires at least one validated address');
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Declining the compat overload: SSRFProtection is an internal utility of this server, not part of the embedding API surface (mcp-engine.ts is), and all four in-repo call sites are updated in this PR. An overload keeping the single-address form would preserve exactly the first-answer-only pinning these issues exist to remove, so leaving the old signature callable works against the fix.

Copilot AI review requested due to automatic review settings August 18, 2026 16:44
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Test Results Summary

📊 Artifacts


Generated at Tue, 18 Aug 2026 20:51:22 GMT
Commit: 34acdf1
Run: #1486

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/utils/ssrf-protection.ts:471

  • pinnedLookup assumes the 3-argument lookup(hostname, options, callback) signature. Node’s lookup contract also allows lookup(hostname, callback); if any caller (now or in future Node versions) invokes the pinned lookup with 2 args, callback will be undefined and this will throw when calling it.

Make the pinned lookup robust to the 2-arg overload by normalizing options/callback when options is a function.

    const pinnedLookup = (
      _hostname: string,
      options: any,
      callback: any
    ): void => {

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.51965% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/triggers/handlers/chat-handler.ts 10.00% 9 Missing ⚠️
src/triggers/handlers/form-handler.ts 10.00% 9 Missing ⚠️
src/services/n8n-api-client.ts 95.71% 3 Missing ⚠️
src/utils/ssrf-protection.ts 97.95% 2 Missing ⚠️
src/utils/n8n-errors.ts 97.56% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

czlonkowski and others added 2 commits August 18, 2026 22:41
…ection failures (v2.71.0)

The DNS-pinning protection resolved one address and pinned it for the
process lifetime: localhost resolving ::1-first broke IPv4-only n8n
instances on macOS, and CDN-fronted instances stayed nailed to a
single possibly-dead edge. Resolve the full record set, validate every
answer (fail closed on any disallowed record, all modes), pin the
whole validated set with autoSelectFamily fallback, re-resolve after a
60s TTL and on connection failure, wire N8N_API_MAX_RETRIES into a
real retry path (pre-connection failures any method, reset/timeout
reads only), and name the failing address in NO_RESPONSE errors.

Fixes #978
Fixes #989
Fixes #990
Reported with detailed root-cause analyses by @ConnorCloze (#978)
and @Boulevard-Dreams (#989, #990).

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeQL flagged /\(([^)]+)\)\s*$/ as polynomially backtracking on
'('-heavy input. Scan for the parenthesized suffix with lastIndexOf
instead; same accepted shape.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 18, 2026 20:41
@czlonkowski
czlonkowski force-pushed the fix/ssrf-pinned-agent-resilience branch from 2e81b11 to d20753c Compare August 18, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/utils/ssrf-protection.ts:34

  • autoSelectFamily fallback is only enabled when net.getDefaultAutoSelectFamily exists (Node >=18.13), but this repo’s runtime engines allow Node >=18.0.0. On Node 18.0–18.12, the pinned lookup will still effectively pick only the first DNS answer, so the #978 localhost (::1-first) failure can still reproduce on a supported runtime. Consider either bumping the supported Node version to >=18.13.0 (so the fix is guaranteed) or implementing an explicit cross-address fallback for older Node versions.
// SECURITY (#978/#989/#990 resilience follow-up to GHSA-cmrh-wvq6-wm9r):
// `autoSelectFamily`/`autoSelectFamilyAttemptTimeout` were added in Node 18.13
// and are stable by 20.x. Guard for older runtimes; computed once at module
// scope rather than probed per-socket.
const supportsAutoSelectFamily = typeof (net as any).getDefaultAutoSelectFamily === 'function';

src/services/n8n-api-client.ts:316

  • The JSDoc for isRetryableConnectionError says in-flight failures are retried for “idempotent methods”, but the implementation only treats GET/HEAD as idempotent. Either broaden the predicate (if intended) or adjust the comment to match the current behavior so readers don’t assume PUT/DELETE are retried too.
   * HTTP method. Errors that occurred before any bytes reached the wire
   * (connection refused/unreachable/DNS failure) are safe to retry
   * regardless of method - the server never saw the request. Errors that may
   * have interrupted an in-flight request (reset, timeout) are only retried
   * for idempotent methods.

@czlonkowski
czlonkowski merged commit 25156a1 into main Aug 18, 2026
15 checks passed
@czlonkowski
czlonkowski deleted the fix/ssrf-pinned-agent-resilience branch August 18, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants