fix(sso): mark the post-login/language-switch redirect no_applink - #3631
fix(sso): mark the post-login/language-switch redirect no_applink#3631YishaiGlasner wants to merge 3 commits into
Conversation
…link Akiva's earlier fix (379e366) excluded the allauth OAuth callback paths from AASA, but the redirect Django issues *after* that callback -- to the arbitrary "next" page -- was still unprotected, so iOS could still grab the login flow at that hop. The same unprotected-second-hop shape existed for the language-switch domain redirect. Add a general mechanism instead of enumerating more static paths: a new WebSessionRedirectMiddleware marks any redirect's Location with a no_applink query param when it's continuing an in-progress web session (sefaria Referer, or the /accounts/*, /_allauth/* OAuth callback paths where Referer is always external by protocol design). apple_app_site_association excludes that marker via AASA's query-matching form, so iOS never hands the marked URL to the app. Client-side, the marker is stripped once the landing page mounts. This also fixes an overly broad side effect of the original fix: bare /login, /register, /logout, /password/reset were unconditionally excluded from ever opening the app, even for a fresh tap with no session to protect (e.g. from a promotional email) -- those now behave like any other link. No Mobile app changes: the bug is iOS-specific (Universal Links), and the app's own native SSO (api/auth/*/mobile) never touches the affected paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/api/auth/google/redirect (sso/urls.py) is allauth's LoginByTokenView exposed directly as Google One Tap's redirect-mode login_uri -- accounts.google.com POSTs the credential straight to it, a genuine cross-domain top-level navigation into sefaria.org, then it redirects onward via the same get_login_redirect_url machinery as the /accounts/* callbacks. It was missing from AASA_EXCLUDED_PATHS: its Referer is always Google's, so referer_is_sefaria_domain() can't catch it, and its path doesn't start with /accounts/ or /_allauth/, so it fell through WebSessionRedirectMiddleware entirely -- the exact bug this fix targets could still reproduce for this specific SSO entry point. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens iOS Universal Links behavior during web-based SSO login and interface-language domain switching by dynamically marking “continuation” redirects so iOS won’t hand them off to the native app mid-flow. It centralizes redirect-marking in middleware, updates the AASA payload to exclude marked URLs via a query rule, and cleans up the marker client-side after the landing page loads.
Changes:
- Introduces a
no_applinkquery-marker mechanism (mark_no_applink) and adds an AASAcomponentsexclusion rule for that marker. - Adds
WebSessionRedirectMiddlewareto apply the marker to redirects that continue a web session (internal Referer) or are known OAuth callback endpoints. - Strips the marker from the browser URL after load, and adds focused unit tests for middleware behavior and AASA structure/ordering.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| static/js/client.jsx | Removes no_applink from the URL via history.replaceState after a marked landing page loads. |
| sefaria/utils/views_utils.py | Defines AASA_EXCLUDED_PATHS, NO_APPLINK_PARAM, and mark_no_applink() helper. |
| sefaria/utils/domains_and_languages.py | Adds referer_is_sefaria_domain() to classify “in-web-session” navigations by Referer. |
| sefaria/system/middleware.py | Adds WebSessionRedirectMiddleware to mark qualifying redirects with no_applink. |
| sefaria/settings.py | Registers WebSessionRedirectMiddleware in the middleware stack ahead of language middlewares. |
| reader/views.py | Updates AASA generation to (a) use shared excluded paths and (b) exclude no_applink via components. |
| reader/tests/apple_app_site_association_test.py | Adds tests asserting excluded rules appear before the catch-all in AASA components. |
| sefaria/system/tests/test_middleware.py | Adds tests for redirect marking behavior under various Referer and path conditions. |
| sefaria/system/tests/test_language_module_switching.py | Adds regression test ensuring the language-switch “second hop” redirect is still marked. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def process_response(self, request, response): | ||
| is_redirect = isinstance(response, (HttpResponseRedirect, HttpResponsePermanentRedirect)) | ||
| is_web_session = request.path.startswith(_OAUTH_CALLBACK_PREFIXES) or referer_is_sefaria_domain(request) | ||
| if is_redirect and is_web_session: | ||
| response['Location'] = mark_no_applink(response['Location']) | ||
| return response |
There was a problem hiding this comment.
This is a worthwhile comment, since I don't think we want to be adding random query parameters to redirects to other places... @YishaiGlasner
There was a problem hiding this comment.
Claude says we have referer_is_sefaria_domain that we could refactor and re-use here perhaps
- Only mark a redirect no_applink when its *destination* is also a sefaria domain (new redirect_target_is_sefaria_domain), not just when the request was referred by one -- a sefaria-referred request that redirects somewhere genuinely external (e.g. /wiki -> developers.sefaria.org) was getting the marker tacked onto an unrelated URL for no reason. - mark_no_applink is now idempotent -- won't double-add the param if the URL is already marked (add_query_param itself still allows duplicates by design, for its other caller). - client.jsx checks the actual no_applink query key via URLSearchParams.has(), not a substring match on the raw search string, which could misfire on an unrelated param whose value happens to contain "no_applink". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| return short_to_long_lang_code(matched_langs[0]) | ||
|
|
||
|
|
||
| def _known_domain_hostnames(): |
There was a problem hiding this comment.
This should be calculated once on startup and stored, not re-generated on the fly on each request..
There was a problem hiding this comment.
🟡 Changes recommended
A newly added test file is not collected by the repo’s pytest configuration, and there are a couple of small correctness/compatibility fixes needed in the new no_applink helpers/client stripping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sefaria/utils/tests/views_utils_test.py:11
- This test file won’t be collected by pytest with the current
pytest.iniconfiguration: collection includessefaria/tests/*_test.py,sefaria/system/tests/*_test.py, etc., but notsefaria/utils/tests/*_test.py. As a result, the newmark_no_applinkcoverage may never run in CI until the file is moved into a collected suite orpytest.iniis updated.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
| def mark_no_applink(url): | ||
| if NO_APPLINK_PARAM in dict(parse_qsl(urlparse(url).query)): | ||
| return url | ||
| return add_query_param(url, NO_APPLINK_PARAM, "1") |
| const url = new URL(window.location.href); | ||
| if (url.searchParams.has('no_applink')) { | ||
| url.searchParams.delete('no_applink'); | ||
| history.replaceState(null, '', url); | ||
| } |
Description
Mobile-web SSO login on iOS could still get interrupted by the app opening mid-flow, even after Akiva's earlier fix (
379e366ee). That fix excluded the allauth OAuth callback paths (/accounts/*,/_allauth/*, plus/login,/register,/logout,/password/reset*) from iOS'sapple-app-site-association(AASA), so the redirect back from Google/Apple stays in the browser. But after allauth finishes and sets the session cookie, Django issues a second redirect — to the "next" page (SefariaAccountAdapter.get_login_redirect_url) — and that hop is an arbitrary, un-enumerable URL that a static path list can never cover. iOS could still grab the login flow right there.The same shape of bug exists for the interface-language domain-switch (sefaria.org ↔ sefaria.org.il): a correctly-marked first hop, then an unprotected second redirect once
LanguageCookieMiddlewarestrips the marker.Rather than adding more entries to a static path list, this replaces the general case with a rule: if a redirect's triggering request was referred by a sefaria.org(.il) page, it's continuing a web session and must never be handed to the app. A small, technically-forced exception remains for
/accounts/*,/_allauth/*, and/api/auth/google/redirect(Google One Tap's redirect-modelogin_uri— added after review caught it missing, see Code Changes), since each of those is reached via a genuine cross-domain POST/redirect from the OAuth provider, whose Referer is always external by protocol design — but that's unambiguous given the current codebase (those paths are never reached any other way than a web login round-trip).Akiva's fix had its own side effect: it excluded
/login,/register,/logout,/password/resetunconditionally, with no way to tell "mid-session" from "fresh tap" apart. This change undoes that — a fresh tap on one of those now reaches the app instead of always being forced to the browser.DeepLinkRouter.jshas no dedicated route for a bare/login(or/register//logout//password/reset). It matches the generic single-segment route (^([^/]+)$→openRef), which tries to resolve "login" as a book title via an asyncSefaria.api.name()call, fails, and only then falls back tocatchAll→ReaderApp.js'sopenUri→InAppBrowser.open(). So the app launches, does a pointless failed lookup, then shows the page in an in-app browser tab — not broken, but janky, and not something to wave off as "fine either way." Before this PR, that path was unreachable — these paths were unconditionally excluded, so the bad routing never mattered. This PR makes it reachable, so the fix now needs to land too: add explicit routes for/login,/register,/logout,/password/resetstraight tocatchAllinMobile/DeepLinkRouter.js, skipping the failed lookup. That's a Mobile-repo change, intentionally out of scope for this PR (see below), but it should be tracked and shipped — ideally alongside this, not left dangling. Flagging this prominently since whoever deploys this should know about it going in, not discover it after.No Mobile app changes in this PR. This bug is iOS-specific (Universal Links) — Android's App Links can't do path/query exclusion at all, and there's no evidence Android was ever affected by this particular redirect shape. The app's own native SSO (
api/auth/*/mobile) is structurally unaffected either way: it's a plain JSON POST, never a browser navigation, and never touches/accounts/*or/_allauth/*.Code Changes
sefaria/utils/views_utils.py—AASA_EXCLUDED_PATHSshrunk to/accounts/*,/_allauth/*, and/api/auth/google/redirect(Google One Tap's redirect-modelogin_uri— allauth'sLoginByTokenViewexposed directly,sso/urls.py; same external-Referer problem as the other two, just not under/accounts/or/_allauth/); addedNO_APPLINK_PARAM/mark_no_applink(idempotent — won't double-mark a URL that already carries the param).sefaria/utils/domains_and_languages.py— newreferer_is_sefaria_domainandredirect_target_is_sefaria_domain, both built on the existingsettings.DOMAIN_MODULES(no hardcoded hostnames). The latter exists so the middleware only marks a redirect when its destination is actually a sefaria domain too — otherwise a sefaria-referred request that redirects somewhere genuinely external (e.g./wiki→developers.sefaria.org,sites/sefaria/urls.py) would getno_applinktacked onto an unrelated URL.sefaria/system/middleware.py— newWebSessionRedirectMiddleware: marks any redirect'sLocationwithno_applinkwhen the request is either to one of the paths above or referred by a sefaria domain, and the redirect's own target is a sefaria domain (or relative). One centralized mechanism —sso/adapters.pyandLanguageCookieMiddlewareneeded no changes.sefaria/settings.py— registers the new middleware immediately beforeLanguageCookieMiddleware(needs to wrap bothLanguageCookieMiddlewareandLanguageSettingsMiddleware, since either can short-circuit with its own redirect).reader/views.py—apple_app_site_associationadds a query-based AASA exclusion ({"?": {"no_applink": "*"}, "exclude": true}), the documented Apple mechanism for excluding a dynamic destination, not just a static path.static/js/client.jsx— stripsno_applinkfrom the URL viahistory.replaceStateonce a marked landing page mounts, so a bookmarked/shared copy of the URL doesn't keep it. Checks the actual query key viaURLSearchParams.has(), not a substring match on the raw search string, so it doesn't misfire on an unrelated param whose value happens to contain "no_applink".sefaria/system/tests/test_middleware.py(WebSessionRedirectMiddleware, including the external-target and Google-One-Tap cases),sefaria/system/tests/test_language_module_switching.py(language-switch second hop gets marked),reader/tests/apple_app_site_association_test.py(AASA JSON structure/ordering),sefaria/utils/tests/views_utils_test.py(mark_no_applinkdoesn't duplicate the param).Notes
Test plan
Universal Links only activate for a domain that's both declared in the app's
com.apple.developer.associated-domainsentitlement and AASA-verified against that exact domain. The app's entitlements only coversefaria.org,www.sefaria.org,sefaria.org.il,www.sefaria.org.il— not any cauldron subdomain. So there are two real ways to test the actual "does iOS refuse to open the app" behavior:Option A — Cauldron + a throwaway TestFlight build with its associated-domains entitlement pointed at the cauldron URL specifically. Gives a fully isolated pre-merge signal, but requires a Mobile-side provisioning change and coordination for a build that exists only for this test:
Mobile/ios/ReaderApp/ReaderApp.entitlements— add the cauldron host tocom.apple.developer.associated-domains, e.g.applinks:name.cauldron.sefaria.org, alongside the existing four production entries. No Apple Developer portal change needed beyond that — Associated Domains is already an enabled capability for this App ID; Apple verifies ownership by fetching AASA from the domain itself, not via a portal-side domain registry.apple_app_site_associationisn't host-specific, so it already serves correct AASA for whatever domain cauldron is deployed under._baseHostpointed at cauldron — the flow being tested is driven entirely from Safari (navigate to the cauldron URL, run through SSO/language-switch there); the installed app is just sitting there as the thing Universal Links might hand off to.master, since the cauldron host changes per branch/PR.Option B — Deploy to prod, test there immediately after. Walk through SSO (Google + Apple, login + register) and a language switch on a real device.
Either way, AASA caching applies regardless of distribution channel (App Store or TestFlight) — it's a property of the domain, not the build — so force a fresh fetch (reinstall the app) before drawing conclusions from a device that's had the app installed before. This matters less on Option A's very first install (the cauldron domain hasn't been associated on that device before, so there's nothing stale to have cached), but comes back if the same build gets reused across multiple test iterations while the fix is still being tweaked.
curl https://www.sefaria.org/apple-app-site-associationafter deploy — confirm all three static exclusions (/accounts/*,/_allauth/*,/api/auth/google/redirect) and the query-basedno_applinkrule are present and correctly ordered. This re-checks what the unit tests already assert about the JSON, but against the actual deployed path: there's a dedicatedlocation /apple-app-site-associationblock in nginx (helm-chart/sefaria/conf/nginx.template.conf.tpl), separate from Django routing, so it's worth confirming nginx/CDN is actually serving the current file/loginlink tapped from Notes/Messages (simulating an external/email tap) — confirm it now opens the app (it previously never did). Expect the janky path described above (failed book-title lookup, then an in-app browser tab) until theDeepLinkRouter.jsfollow-up ships — don't block this PR on it, but don't let it get lost either🤖 Generated with Claude Code