Security review fixes: SSRF resolution and pinning, error oracle, fail2ban injection, identity casing, token expiry, rate limits, headers - #5
Merged
Conversation
… connection The host blocklist matched hostname text and never resolved DNS, so any attacker-owned name pointing at loopback, the tailnet range or a metadata address passed. The URL is now resolved first, every address is checked against the forbidden ranges (IPv4 and IPv6, mapped and NAT64 forms), and the request is dispatched through an agent whose connect-time lookup returns only the vetted address, closing the rebinding window. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sed ranges
The connect-time lookup used the three-argument callback form. Node enables
autoSelectFamily by default, so the socket layer calls a custom lookup with
{ all: true } and rejects that form with 'Invalid IP address: undefined',
which failed every pinned request. It now answers with the full array of
vetted addresses, which also restores failover for multi-homed endpoints.
A test that dispatches for real covers this; the callback type is a union,
so the wrong shape type-checks and only a live request catches it.
A trailing root dot no longer bypasses the forbidden-name list. The refused
set gains IPv4-compatible, site-local, 6to4 and local-use NAT64 addresses.
The note about a production guard on the private-host flag described
something that does not exist and now describes the actual situation.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The upstream HTTP status and the raw exception text (connection refused, DNS error, timeout) were returned straight to the anonymous browser caller. That turns a blind server-side verification request into a port scan oracle of whatever the URL pointed at: a caller could learn whether a given internal host or port is open just by reading the error text. registerWebhook now returns one constant, ENDPOINT_NOT_VERIFIED, for every failure mode of the verification POST. The real detail (status or truncated exception message) still goes to console.warn, keyed by the subscription id, for operator debugging. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…annot forge the banned address Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…owercase Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…gration, and normalise identity case everywhere it is compared The publishers-lowercase migration's delete had no tie-break: added_at defaults to the transaction start time, so multiple casings inserted in one transaction get an identical timestamp, the delete removes nothing, and the following lowercase update then collides on the primary key. Order on (added_at, ctid) instead, which keeps "earliest wins" while guaranteeing a strict order between any two rows. Proven against a temporary table shaped like publishers, since the migration itself already applied cleanly against a table with no duplicate casings. withdrawPublish and rejectPublish still compared the requester to the acting publisher case-sensitively, missing the same normalisation four-eyes already got. resolveIdentity now lowercases and trims all three identity sources (Auth0 header, Tailscale header, dev fallback) at the one place identity is resolved, instead of relying on each downstream comparison to do it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed fixed windows Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds CSP (framing/plugins/base-uri/form-action only, no script-src or style-src yet), X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Strict-Transport-Security, and Permissions-Policy via next.config.mjs headers(). Verified form-action 'self' cannot break the Auth0 login flow: the login route answers with a redirect and the callback is a GET, neither posts a form. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…l2ban check Four fixes from the final review round. 1. Lowercasing the subscriber address in the public action orphaned any subscription row written earlier in mixed case: the lookups in startEmailSubscription compare endpoint exactly, so a returning subscriber missed their own row, got a second one, and received every announcement twice. Migration 018 collapses the duplicate casings of each email address (keeping the verified row where exactly one is verified, otherwise the earliest by (created_at, ctid)), lowercases what remains, and adds a check constraint refusing future mixed-case email rows. Webhook endpoints are left alone, since a URL path is case-sensitive. startEmailSubscription now normalises the address itself rather than trusting its callers, so the subscriber-adding script cannot reintroduce mixed case; that script lowercases its argument too. 2. The offline fail2ban check used a <HOST> expansion that matched only IPv4 and hostnames, so a bare IPv6 client address went unmatched. It now mirrors fail2ban 1.0's alternation (the version the distro package installs) and covers two more lines: an IPv6 failure, and an IPv6 failure whose username carries a forged address. The jail's own failregex is unchanged, since fail2ban does that expansion itself. 3. Comments in the client-address helper record that the x-forwarded-for fallback is only safe behind a proxy that overwrites the header, and that 'unknown' is one shared, fail-closed bucket. 4. The tsconfig note listing the deep-import sites for the headers module was missing the public actions file. Co-Authored-By: Claude Fable 5.1 <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.
Closes the three High and four Medium findings of the 2026-09-07 security review. Stacks on PR #4; GitHub retargets to
mainwhen #4 merges. Each task reviewed independently, plus a whole-branch review and one fix wave.src/core/safe-url.tsresolves the name, refuses any address in loopback, link-local, RFC 1918, CGNAT/tailnet100.64/10, multicast, reserved, IPv6 unique-local/link-local/site-local, IPv4-mapped, IPv4-compatible, NAT64 and 6to4 forms, refuseslocalhost/.local/.internal/.home.arpanames (trailing dot included), then pins the connection through an undiciAgentwhose connect-time lookup returns only the vetted addresses. Used by registration and by every delivery.undicipinned as a dependency. A test dispatches through the pin to a local server.failregexanchored on%m [%p] %h;files/failregex-check.pyproves benign, forged, IPv6 and socket lines.ctidtie-break) and adds a check constraint.verify_token_issued_at; 72-hour window; token cleared on use.rate_limits, atomic fixed-window upsert. Email 3/h per address and 10/h per IP, webhook 5/h per IP. Email format and length checks; URL length cap. Both email limits consumed before deciding, so the refusal reveals nothing.frame-ancestors 'none'; object-src 'none'; base-uri 'self'; form-action 'self'), X-Frame-Options, nosniff, Referrer-Policy, HSTS, Permissions-Policy fromnext.config.mjs.script-src/style-srcleft for a nonce follow-up.Tests: 738 passing (645 + 93). Typecheck and
web:buildgreen. Thedb-tlsintegration file needs the port-5500 container and was not run here.Deploy steps
npm run migrateas theannounceowner (015–018; 015 alters a table and 017 grants).fail2banrole) so the new filter lands; thenfail2ban-client status announce-postgreson the VM.npm cion the build picks upundici.🤖 Generated with Claude Code