Fix: (v14 v15) harden authentication entry points and redirects - #990
Fix: (v14 v15) harden authentication entry points and redirects#990KeZhao783 wants to merge 3 commits into
Conversation
Files changed: - app/urls.py: remove the legacy minilogin route from the shared router - app/views.py: remove the predictable-token miniLogin view and hasher use - app/test/test_auth_security.py: prove both legacy mounted paths cannot authenticate
Files changed: - utils/http/utils.py: Centralize same-host redirect validation. - generic/views.py: Protect login and WebView redirect targets. - generic/tests.py: Cover helper, HTTPS downgrade, and WebView redirect safety. - app/views.py: Protect account-switch redirect targets. - app/test/test_auth_security.py: Cover login and account-switch redirect safety.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6039d4b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| origin = safe_local_redirect_target( | ||
| request, request.GET.get("origin"), "/welcome/" | ||
| ) | ||
| return redirect(origin) |
There was a problem hiding this comment.
When an authenticated user visits a cross-site link or embedded resource targeting /shiftAccount/?oname=..., this GET request still calls update_related_account_in_session(..., shift=True), which logs out the current principal and logs in the selected person or organization account. The new redirect handling and test preserve this state-changing GET without CSRF protection, allowing account-switch CSRF; convert callers to a CSRF-protected POST, or document a verified compatibility exception next to the route.
AGENTS.md reference: AGENTS.md:L502-L504
Useful? React with 👍 / 👎.
Reject state-changing GET requests, accept account and redirect targets only from POST data, and submit every sidebar switch through a CSRF-protected form. Files changed: - app/test/test_auth_security.py: cover GET non-mutation, CSRF rejection, protected switching, safe redirects, and rendered sidebar forms. - app/views.py: require CSRF-protected POST requests and parse switch inputs from POST data. - static/assets/css/structure.css: preserve sidebar link styling for account-switch submit buttons. - templates/org_left_navbar.html: replace personal and organization switch links with CSRF POST forms. - templates/user_left_navbar.html: replace organization switch links with CSRF POST forms.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85aa0f23f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <form method="post" action="{% url 'shiftAccount' %}" | ||
| class="account-switch-form"> | ||
| {% csrf_token %} |
There was a problem hiding this comment.
Because CsrfViewMiddleware is disabled in boot/settings.py, rendering {% csrf_token %} in an ordinary sidebar response creates the hidden form value but does not run the middleware response hook that writes the corresponding CSRF cookie. A user with a clean browser session therefore submits this form to the new @csrf_protect endpoint without a cookie and receives a 403; the added success test masks this by manually assigning client.cookies[settings.CSRF_COOKIE_NAME]. Ensure responses rendering these forms set the cookie, such as with ensure_csrf_cookie, or enable the middleware.
AGENTS.md reference: AGENTS.md:L467-L469
Useful? React with 👍 / 👎.
| if not url_has_allowed_host_and_scheme( | ||
| target, | ||
| allowed_hosts={request.get_host()}, | ||
| require_https=request.is_secure(), |
There was a problem hiding this comment.
When the deployment accepts an unrecognized Host header—as the current ALLOWED_HOSTS = ["*"] setting permits—an attacker can send Host: evil.example with a matching https://evil.example/... target, and this dynamically constructed allowlist treats that external destination as local. That restores an open redirect after login or ticket consumption in any proxy configuration that forwards such hosts; validate against a configured canonical host or restrict caller-supplied targets to relative local paths.
AGENTS.md reference: AGENTS.md:L517-L520
Useful? React with 👍 / 👎.
2 participants
No description provided.