Skip to content

fix: drop stale Django session when proxy identity changes#29

Merged
awais786 merged 10 commits into
foss-mainfrom
fix/proxy-auth-stale-session-on-user-switch
May 16, 2026
Merged

fix: drop stale Django session when proxy identity changes#29
awais786 merged 10 commits into
foss-mainfrom
fix/proxy-auth-stale-session-on-user-switch

Conversation

@awais786
Copy link
Copy Markdown

@awais786 awais786 commented May 15, 2026

Summary

  • ProxyAuthMiddleware used to short-circuit on any authenticated Django session and ignore the upstream X-Auth-Request-Email header. After the portal's "Log out of all apps" + a different user logging in, the app-local Django session cookie survives (it's scoped to the app subdomain and not cleared by the shared _oauth2_proxy / Cognito logout) — so refreshing the Plane tab kept serving the previous user.
  • Now: short-circuit only when the proxy-asserted email matches request.user.email (or no header is present). On mismatch, call django.contrib.auth.logout(request) to flush the stale session immediately, then fall through and re-authenticate.
  • The explicit logout() closes a hole that relying on login()'s implicit session rotation didn't cover: if any subsequent bail-out fires (today: the incoming user is inactive; any future bail-out before login()) the request previously continued authenticated as the previous user. With the explicit logout, every non-success path is unauthenticated.
  • Header parsing extracted into _read_proxy_email (no behavior change).
  • Bypass-path check moved to the top so it skips both branches cleanly.

Three rules, in order

  1. Match (session.email == proxy.email) → short-circuit, stay logged in.
  2. No header (incoming == "") → short-circuit, stay logged in. Header absence is not a logout signal — internal calls, OPTIONS preflight, bypass paths, direct backend hits, and the Django test client all legitimately arrive without it. Trust model: the backend port is bound to 127.0.0.1, so the header is only absent on trusted internal paths.
  3. Mismatch (session.email != proxy.email) → logout(request), then attempt re-auth as the incoming user.

Repro (before this PR)

  1. Log in to FOSS as user A.
  2. Open Plane (and other service apps) in separate tabs.
  3. On the portal, click Log out of all apps.
  4. Log in as user B via password.
  5. Refresh the Plane tab → still shows user A.

SurfSense does not exhibit this because FastAPI re-derives identity from headers on every request rather than persisting a native session cookie.

Test plan

  • pytest apps/api/plane/authentication/tests/test_proxy_auth.py -v passes — TestProxyAuthMiddlewareUserSwitch covers four cases:
    • mismatched proxy email triggers logout() then re-auth with the new user
    • mismatched proxy email + inactive incoming user → logout() is called, user_login() is not, request continues unauthenticated (regression guard for the bail-out hole)
    • missing header on an authenticated request does not log the user out
    • match comparison is case- and whitespace-insensitive
  • Existing tests still pass — TestProxyAuthMiddlewareAlreadyAuthenticated.test_skips_when_user_already_authenticated exercises the match-and-short-circuit path
  • Manual: run the repro above against docker compose -f docker-compose-local.yml up -d --force-recreate api — refreshing the Plane tab as user B now serves user B, not user A
  • Verify the bypass paths (/god-mode/*, /api/instances/*) still bypass the middleware entirely

Out of scope

  • A complementary portal-driven per-app logout (clears Django session cookies across all app subdomains on portal logout) is in pipeline; the explicit logout() here is the defensive layer that closes the window when that signal hasn't reached this app yet.
  • Outline / Penpot / Twenty have the same architectural pattern (native session cookie outlives upstream auth state) and need analogous fixes in their respective middlewares.

🤖 Generated with Claude Code

ProxyAuthMiddleware short-circuited on any authenticated Django session,
ignoring the X-Auth-Request-Email header. After a portal "log out of all
apps" + login as a different user, the app-local Django session cookie
survives (it's scoped to the app subdomain and not cleared by the shared
_oauth2_proxy / Cognito logout), so refreshing the Plane tab kept serving
the previous user.

Now: short-circuit only when the upstream-asserted email matches the
session's user email (or no header is present). On mismatch, fall through
and re-authenticate — django.contrib.auth.login() flushes the stale
session automatically when the user pk changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

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 updates the Django ProxyAuthMiddleware to prevent a stale app-scoped Django session from continuing to authenticate a user when the upstream proxy identity (via X-Auth-Request-Email) has changed, and adds tests to cover user-switch scenarios.

Changes:

  • Only short-circuits on an existing authenticated Django session when the proxy-asserted email is absent or matches the session user’s email (case/whitespace-insensitive).
  • Extracts proxy email parsing into _read_proxy_email() and moves bypass-path handling to the top of the middleware.
  • Adds a new TestProxyAuthMiddlewareUserSwitch test class covering mismatch, missing-header behavior, and normalization matching.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
apps/api/plane/authentication/middleware/proxy_auth.py Adjusts authenticated-session short-circuit logic to re-auth when upstream identity differs; refactors header parsing.
apps/api/plane/authentication/tests/test_proxy_auth.py Adds regression tests for switching users across stale Django sessions and header normalization behavior.
Comments suppressed due to low confidence (1)

apps/api/plane/authentication/middleware/proxy_auth.py:93

  • When the request already has an authenticated Django session but the proxy email differs, the middleware falls through and resolves the incoming user. If that resolved user is inactive, the middleware returns without calling user_login(), leaving request.user as the previous (stale) authenticated user for this request. This defeats the goal of dropping a stale session on identity mismatch and can incorrectly serve the prior user. Consider explicitly clearing the existing session (e.g., django.contrib.auth.logout(request) or request.session.flush() + setting AnonymousUser) before returning/continuing when a mismatch is detected, especially on the inactive-user early return path.
        if request.user.is_authenticated:
            # Short-circuit only when the upstream-asserted identity matches the
            # current Django session, or when no header is present (request did
            # not pass through ForwardAuth — header absence is not a logout signal).
            #
            # If the proxy email differs (typical pattern: portal "log out of all
            # apps" clears the shared _oauth2_proxy cookie + Cognito session but
            # NOT this app's Django session cookie, then someone else logs in),
            # fall through to re-authenticate. Django's login() flushes the stale
            # session automatically when the user pk changes.
            current = _normalise_email(request.user.email or "")
            incoming = _normalise_email(proxy_email or "")
            if not incoming or current == incoming:
                return self.get_response(request)

        if not proxy_email:
            return self.get_response(request)

        email = _normalise_email(proxy_email)
        if not email:
            return self.get_response(request)

        user = self._resolve_user(email)

        # Respect deactivated accounts — mPass authentication does not
        # override an explicit Plane account suspension.
        if not user.is_active:
            return self.get_response(request)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

apps/api/plane/authentication/tests/test_proxy_auth.py:28

  • The design-contract bullet “username is always uuid4().hex” is incorrect: ProxyAuthMiddleware derives username from the email local part (see test_username_derived_from_email_local_part and proxy_auth._resolve_user). Please update/remove that bullet to avoid confusing future maintainers.
- If email is present → get_or_create User, create Profile on first creation,
  then call user_login(request, user, is_app=True) to establish session
- New users get: set_unusable_password(), is_password_autoset=True, is_email_verified=True
- username is always uuid4().hex (never the Cognito sub — avoids length/collision issues)
- Email is normalised (lowercased + stripped) before DB lookup

Comment on lines +100 to +108
"""Extract the upstream-asserted email from oauth2-proxy headers.

Handles two header shapes:
- X-Auth-Request-Email contains a real email → use as-is
- X-Auth-Request-Email contains a bare username (user_id_claim=
cognito:username) → synthesise <username>@DEFAULT_EMAIL_DOMAIN
- X-Auth-Request-Email is empty but X-Auth-Request-User has a username
→ synthesise the same way

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in 417b884 — updated the docstring from "Handles two header shapes" to "Handles three cases" to match the three bullets actually described.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Updated in commit 417b884 — the _read_proxy_email docstring wording now reflects three cases, matching the listed behavior.

Comment thread apps/api/plane/authentication/tests/test_proxy_auth.py Outdated
Comment thread apps/api/plane/authentication/middleware/proxy_auth.py
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread apps/api/plane/authentication/middleware/proxy_auth.py
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/api/plane/authentication/tests/test_proxy_auth.py:31

  • The design-contract docstring says “username is always uuid4().hex”, but the middleware actually sets username to the email local-part (see ProxyAuthMiddleware._resolve_user: username_hint = email.split("@")[0] ...). This is also what test_username_derived_from_email_local_part asserts. Please update the contract text to match the actual behavior (local-part with uuid4 fallback) so the test documentation isn’t misleading.
- If identity can be derived from headers → get_or_create User, create Profile on first creation,
  then call user_login(request, user, is_app=True) to establish session
- New users get: set_unusable_password(), is_password_autoset=True, is_email_verified=True
- username is always uuid4().hex (never the Cognito sub — avoids length/collision issues)
- Email is normalised (lowercased + stripped) before DB lookup

Three small follow-ups on PR #29 from review:

- Inline `_normalise_email(self._read_proxy_email(request))` once into
  `email` and reuse it for both the mismatch comparison and the DB lookup,
  removing the second `_normalise_email(proxy_email)` call.
- Add `TODO(security)` on `_read_proxy_email` pointing at
  `fix/proxy-auth-reject-bare-username` for the bare-username synthesis
  vulnerability this PR preserves but does not address.
- Add `test_bypass_dominates_mismatched_proxy_header` to lock in that the
  bypass check at the top of `__call__` runs before the new mismatch
  logout flow — guards god-mode local admin sessions from being kicked
  out by an unrelated mPass identity reaching the same browser.

All 22 proxy_auth tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread apps/api/plane/authentication/middleware/proxy_auth.py Outdated
Comment on lines +139 to +147
with patch(PATCH_USER_LOGIN) as mock_login, \
patch(PATCH_LOGOUT) as mock_logout:
middleware(request)

# Session should be flushed when mismatch is detected
mock_logout.assert_called_once_with(request)
# Then re-auth with the new user
mock_login.assert_called_once()
assert mock_login.call_args.kwargs["user"].pk == bob.pk
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in cc1107b — the test now uses a manager mock with attach_mock() to track call order and asserts that logout() is called before user_login(). This strengthens the regression guard by ensuring the stale session is flushed before any re-authentication work begins.

Comment on lines +196 to +205
def test_flushes_session_when_incoming_user_is_inactive(self, django_user_model):
"""
GIVEN the current Django session belongs to alice (active)
AND X-Auth-Request-Email = bob's email
AND bob exists but is_active=False
WHEN the middleware processes the request
THEN logout() is called to flush alice's session
AND user_login is NOT called (bob is inactive)
AND the request proceeds as unauthenticated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in 17e64ac — the test now uses a custom get_response stub to capture the request object and verify that the middleware passes it through after calling logout() and not calling user_login(). This confirms the request proceeds without re-authentication when the incoming user is inactive.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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