fix(security): bucket IPv4 trust to /24 and decouple verify-IP OTP send#1420
Merged
Conversation
The new-network IP gate pinned trust to a byte-exact IPv4 address while IPv6 was already bucketed to /64. When a user's egress IP rotates within a provider block (CGNAT, dual-WAN, mobile pools), the host differs in the same /24 between the page load that mints the pending_ip_verify cookie and the OTP-send POST, so the re-check returned ip_mismatch before ever sending the email. The modal renders but the code never arrives and the prompt spins on "Sending...". - Bucket IPv4 to its /24 in normalizeIpForTrust, mirroring the IPv6 /64 rule; render the normalized key as CIDR in formatIpForDisplay. - Move the IP-binding check off the OTP-send path onto the session-minting branch only, so requesting the code is never gated on a rotating address. - Add an AbortSignal timeout to the SendGrid call so a stalled send fails closed instead of hanging the awaiting request. - Add [ip-verify] tracing with the raw pre-normalization IP at the proxy gate, trust assessment, and verify-IP early returns.
joelorzet
requested review from
a team,
OleksandrUA,
eskp and
suisuss
and removed request for
a team
🧹 PR Environment Cleaned UpThe PR environment has been successfully deleted. Deleted Resources:
All resources have been cleaned up and will no longer incur costs. |
ℹ️ No PR Environment to Clean UpNo PR environment was found for this PR. This is expected if:
|
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.
Problem
A 2FA user signing in from a new network sees the
/verify-ipmodal ("Confirm this new sign-in") but never receives the email code, and the prompt spins on "Sending...".Root cause
normalizeIpForTrustbucketed IPv6 to/64but left IPv4 byte-exact (/32). When a user's public source IP rotates within one provider block (CGNAT, dual-WAN, and mobile pools are common, especially in LATAM), the host octet differs within the same/24between two connections seconds apart: the page navigation that mints thepending_ip_verifycookie, and the OTP-send POST to/api/user/verify-ip.The verify-IP route ran the IP re-check at the top of the handler, gating the OTP send itself. A rotated host failed the byte-exact match and returned
ip_mismatch(a 401) before sending. The modal's prefetch never receivesfactors_required, so it stays in the pre-send "Sending..." state and no email is dispatched. The early-return paths logged nothing, so the failure was invisible.Changes
/24innormalizeIpForTrust, mirroring the existing IPv6/64rule.formatIpForDisplayrenders the normalized key as CIDR.AbortSignaltimeout so a stalled send fails closed (returns false) instead of hanging the awaiting request indefinitely.[ip-verify]tracing records the raw pre-normalization IP next to the normalized key at the proxy gate, trust assessment, and every verify-IP early return, so a rotating egress is greppable end to end.Operational notes
user_trusted_ipsare stored as full hosts and will not match the new/24key, so affected users re-verify once; after that the/24is trusted and rotation within the block stops prompting.(user_id, ip)constraint) could skip that one re-verification. Not included here; would land as a deliberate migration.Security tradeoff
/24trust covers up to 256 host addresses in the block, which can include other CGNAT subscribers behind the same pool. For a gate that sits on top of full MFA this is a reasonable trade and matches the IPv6/64granularity already in use. ASN+country pinning is a tighter alternative if desired later.