Skip to content

fix(sso): mark the post-login/language-switch redirect no_applink - #3631

Open
YishaiGlasner wants to merge 3 commits into
masterfrom
bug/sc-46673/webapp-redirects-to-nativeapp-after-login
Open

fix(sso): mark the post-login/language-switch redirect no_applink#3631
YishaiGlasner wants to merge 3 commits into
masterfrom
bug/sc-46673/webapp-redirects-to-nativeapp-after-login

Conversation

@YishaiGlasner

@YishaiGlasner YishaiGlasner commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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's apple-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 LanguageCookieMiddleware strips 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-mode login_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/reset unconditionally, 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.

⚠️ That surfaces a real bug that needs a separate fix in Mobile, not in this PR. Verified the actual code path: DeepLinkRouter.js has 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 async Sefaria.api.name() call, fails, and only then falls back to catchAllReaderApp.js's openUriInAppBrowser.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/reset straight to catchAll in Mobile/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.pyAASA_EXCLUDED_PATHS shrunk to /accounts/*, /_allauth/*, and /api/auth/google/redirect (Google One Tap's redirect-mode login_uri — allauth's LoginByTokenView exposed directly, sso/urls.py; same external-Referer problem as the other two, just not under /accounts/ or /_allauth/); added NO_APPLINK_PARAM / mark_no_applink (idempotent — won't double-mark a URL that already carries the param).
  • sefaria/utils/domains_and_languages.py — new referer_is_sefaria_domain and redirect_target_is_sefaria_domain, both built on the existing settings.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. /wikidevelopers.sefaria.org, sites/sefaria/urls.py) would get no_applink tacked onto an unrelated URL.
  • sefaria/system/middleware.py — new WebSessionRedirectMiddleware: marks any redirect's Location with no_applink when 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.py and LanguageCookieMiddleware needed no changes.
  • sefaria/settings.py — registers the new middleware immediately before LanguageCookieMiddleware (needs to wrap both LanguageCookieMiddleware and LanguageSettingsMiddleware, since either can short-circuit with its own redirect).
  • reader/views.pyapple_app_site_association adds 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 — strips no_applink from the URL via history.replaceState once a marked landing page mounts, so a bookmarked/shared copy of the URL doesn't keep it. Checks the actual query key via URLSearchParams.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".
  • Testssefaria/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_applink doesn'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-domains entitlement and AASA-verified against that exact domain. The app's entitlements only cover sefaria.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 to com.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.
  • Rebuild and re-sign with a provisioning profile that matches those entitlements, then upload to TestFlight. This is a real archive-and-upload cycle, not a config flag.
  • No Project (Django) code change needed beyond what's already in this PR — apple_app_site_association isn't host-specific, so it already serves correct AASA for whatever domain cauldron is deployed under.
  • Testing itself doesn't need the app's _baseHost pointed 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.
  • This entitlements edit is throwaway: make it on a scratch Mobile branch, build once, then discard/revert it — it should never land on Mobile's real 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-association after deploy — confirm all three static exclusions (/accounts/*, /_allauth/*, /api/auth/google/redirect) and the query-based no_applink rule 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 dedicated location /apple-app-site-association block 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
  • SSO login + register, Google + Apple, from mobile Safari with the app installed — no app-bounce at the callback hop or the landing hop
  • Language switch on mobile Safari with the app installed — both hops stay in-browser
  • A fresh /login link 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 the DeepLinkRouter.js follow-up ships — don't block this PR on it, but don't let it get lost either

🤖 Generated with Claude Code

YishaiGlasner and others added 2 commits August 17, 2026 14:12
…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>
@yitzhakc
yitzhakc requested review from yitzhakc and a lite review from Copilot August 26, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_applink query-marker mechanism (mark_no_applink) and adds an AASA components exclusion rule for that marker.
  • Adds WebSessionRedirectMiddleware to 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.

Comment on lines +201 to +206
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

@yitzhakc yitzhakc Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a worthwhile comment, since I don't think we want to be adding random query parameters to redirects to other places... @YishaiGlasner

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude says we have referer_is_sefaria_domain that we could refactor and re-use here perhaps

Comment thread static/js/client.jsx Outdated
Comment thread sefaria/utils/views_utils.py
Comment thread sefaria/utils/views_utils.py
- 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>
@YishaiGlasner
YishaiGlasner requested a review from yitzhakc August 31, 2026 12:21
return short_to_long_lang_code(matched_langs[0])


def _known_domain_hostnames():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be calculated once on startup and stored, not re-generated on the fly on each request..

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.ini configuration: collection includes sefaria/tests/*_test.py, sefaria/system/tests/*_test.py, etc., but not sefaria/utils/tests/*_test.py. As a result, the new mark_no_applink coverage may never run in CI until the file is moved into a collected suite or pytest.ini is updated.
  • Files reviewed: 10/10 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +41 to +44
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")
Comment thread static/js/client.jsx
Comment on lines +14 to +18
const url = new URL(window.location.href);
if (url.searchParams.has('no_applink')) {
url.searchParams.delete('no_applink');
history.replaceState(null, '', url);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants