feat: harden study auth redirects

[ekassos]

Internal

Updates & Improvements

• Magic-link generation sanitizes and URL-encodes redirect query parameters.

[coderabbitai]

Warning

Rate limit exceeded

@ekassos has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 49 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 49 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1dbcfcc2-683c-48e5-941c-2a0b5363879a

📥 Commits

Reviewing files that changed from the base of the PR and between 2584aab and bb48cc5.

📒 Files selected for processing (2)

• pingpong/auth.py
• pingpong/study/server.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ekassos/feat/harden-study-auth-redirects

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[greptile-apps]

Greptile Summary

This PR hardens the study auth redirect flow by introducing normalize_study_redirect — a multi-layer validator that rejects non-path-relative URLs, ////\ prefix bypasses, and control characters — and applies it consistently across magic-link generation, session redirects, and auth callback endpoints. urlencode is also used to properly encode query parameters when building magic-link URLs.

Confidence Score: 4/5

Safe to merge; the open-redirect hardening is correct and well-applied, with only a minor URL-encoding gap in fallback redirect paths that doesn't introduce a security regression.

The core security fix (normalize_study_redirect + urlencode in magic links) is correct and comprehensively applied. The one P2 finding — forward not URL-encoded in fallback RedirectResponse URLs — is pre-existing behavior and doesn't re-introduce an open-redirect risk since the value is already constrained to in-app paths by the validator. Double normalization is cosmetic.

Minor attention on pingpong/study/server.py fallback RedirectResponse calls (lines 287, 290, 354, 357) for URL-encoding of forward.

Vulnerabilities

The open-redirect hardening is solid: normalize_study_redirect applies defense-in-depth (prefix, double-slash, control-character, and urlsplit scheme/netloc checks) and urlencode is used correctly when building magic-link URLs. The only residual gap is that the fallback RedirectResponse paths (e.g. /login?expired=true&forward={forward}) embed the sanitized forward value without URL-encoding it — a path containing ? or # is allowed by the validator and would break query-string parsing on the frontend, but this does not re-open an external-redirect risk since the value is already constrained to in-app paths.

Important Files Changed

Filename	Overview
pingpong/auth.py	Adds normalize_study_redirect with layered open-redirect defenses (prefix check, ////\ check, control-char check, urlsplit scheme/netloc check) and wires it into generate_auth_link (with proper urlencode) and redirect_with_session_study.
pingpong/study/server.py	Applies normalize_study_redirect at all magic-link and auth-callback call sites; fallback RedirectResponse URLs still embed forward without urlencode, which can break query-string parsing for paths containing ? or &.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming redirect param] --> B{normalize_study_redirect}
    B --> C{Empty or None?}
    C -- Yes --> Z[Return '/']
    C -- No --> D{strip and check}
    D --> E{Starts with '/'?}
    E -- No --> Z
    E -- Yes --> F{Starts with '//' or '/backslash'?}
    F -- Yes --> Z
    F -- No --> G{Contains control chars ord lt 32?}
    G -- Yes --> Z
    G -- No --> H{urlsplit: has scheme or netloc?}
    H -- Yes --> Z
    H -- No --> I[Return sanitized path]
    I --> J{Usage context}
    J --> K[generate_auth_link: urlencode into magic link URL]
    J --> L[redirect_with_session_study: config.study_url]
    J --> M[Fallback RedirectResponse: embedded raw in query string]

Loading

Comments Outside Diff (2)

1. pingpong/study/server.py, line 287-290 (link)

   forward not URL-encoded in fallback redirect

   forward is embedded raw into the RedirectResponse URL. If the destination path contains ?, &, =, or # (all valid, and not blocked by normalize_study_redirect), the query string will be malformed and the frontend will parse forward incorrectly (e.g. /courses?tab=1 becomes just /courses with a dangling tab=1 param).

   Use urlencode to encode forward when injecting it as a query parameter value:

   from urllib.parse import urlencode
   
   # example fix for the fallback redirects
   return RedirectResponse(
       f"/login?expired=true&{urlencode({'forward': forward})}", status_code=303
   )

   The same pattern applies at lines 290, 354, and 357.

2. pingpong/study/server.py, line 165-172 (link)

   Redundant double normalization

   safe_forward is already the result of normalize_study_redirect(body.forward). It is then passed as redirect=safe_forward to generate_auth_link, which internally calls normalize_study_redirect(redirect) again (see auth.py line 165). Normalizing twice is harmless but unnecessary. The same applies at login_as (line 209) and the auth/auth_admin endpoints where dest and forward are each independently derived from the same already-validated query param.

   You can remove the pre-normalization in login_magic and login_as (since generate_auth_link already handles it), or remove the normalization inside generate_auth_link and rely entirely on the call-site to sanitize before calling.

_{Reviews (1): Last reviewed commit: "Lint" | Re-trigger Greptile}

[ekassos]

@claude review