fix: capture the customer's real public site URL at signup (not the internal site name) - #900
Merged
Merged
Conversation
…oarding-capture-public-site-url] The bench handed admin the internal Frappe site name (e.g. http://staging.v15) as frappe_site_url when host_name was unset, because _public_origin used allow_header_override=False for every caller. That wrong value then drove the post-payment redirect (and the welcome email link). Capture the real public URL from the authenticated request Host on the SIGNUP flow only, and normalize a real public domain to https: - _public_origin(trust_host=False): host_name still wins; else get_url(allow_header_override=trust_host). Only signup() passes trust_host=True (a new account, self-scoped, require_jarvis_admin-gated). reconnect / replacement / lead keep the secure no-Host derivation - they target an existing account by email, so a spoofed Host must not choose their recorded URL. - _is_real_public_host(): a real domain is dotted, not localhost, not an IP literal (ipaddress), with an alphabetic non-reserved TLD - so a real domain is https-forced while jarvis.local / staging.v15 / IPs / dev hosts are left as-is (dev unaffected). - Malformed Host never raises (signup can't 500). Bench-only; the one-time correction of existing bad rows is an admin ops step.
Hard-trim to match the file's density; no logic change.
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
When a customer's site admin onboards from their own Frappe site (e.g. served at
https://staging-fleet.klerk.in, internal site namestaging.v15), the bench handed adminhttp://staging.v15asfrappe_site_urlwheneverhost_namewas unset._public_origin()usedallow_header_override=Falsefor every caller, so it fell back to the internal site name. That wrong value then drove the post-payment redirect back to the customer (and the welcome-email link), sending them to an unreachable internal URL.Fix — capture the real URL on the signup flow, keep the rest secure
The natural source of the customer's real public URL is the request
Hostheader — but the code deliberately refused it to stop a spoofed Host from poisoning the recorded URL. All onboarding paths are authenticated (require_jarvis_admin), but only signup creates a new account, so trusting that admin's own request Host is self-scoped there; reconnect/replacement/lead target an existing account by email and must stay on the secure derivation._public_origin(trust_host=False)—host_namestill wins; elseget_url(allow_header_override=trust_host). Onlysignup()passestrust_host=True; the four other callers keep the defaultFalse(unchanged, secure)._is_real_public_host()— a real domain (dotted, not localhost, not an IP literal viaipaddress, alphabetic non-reserved TLD) is normalized tohttps://<host>;jarvis.local,staging.v15, IPs, and bare/dev hosts are left as-is, so dev benches are unaffected.Hostnever raises — signup can't 500.Security
Reviewed in depth:
trust_host=Trueis unreachable by a guest (start_signupis@whitelist+require_jarvis_admin); a spoofed Host is ignored on the non-signup paths even under an authenticated session; signup always inserts a fresh Customer (no cross-tenant poisoning); admin re-validates^https?://…; the verification magic-link uses admin's own origin, not this value; no SSRF sink.Tests
110 green on
test_jarvis(+ onboarding suites, 225 total). New tests drive the real unmockedget_urlwith a request Host to prove both the capture (staging-fleet.klerk.in→ https) and the security guarantee (a spoofed Host is ignored on the default path). Each load-bearing guard mutation-verified;host_name-wins-under-trust_host=Truecovered. ruff clean.Note
Bench-only. Existing
Jarvis Customer.frappe_site_urlrows already stored with the internal name need a one-time admin-side data correction (owner-run ops step, out of this PR).