Remove invalid single-backslash redirect test

[arikfr]

Summary

Remove the login-redirect test that treats a single leading backslash as an external redirect.

Rationale

A single \evil.com resolves to the same-origin path /evil.com for special-scheme URLs. The existing mixed slash/backslash case (/\evil.com) remains covered because it resolves to an external host and must be rejected.

Validation

• python3 -m py_compile redash/authentication/__init__.py
• git diff --check
• Required GitHub Actions checks are running.

[greptile-apps]

Greptile Summary

This PR removes the test_backslash_redirect_rejected test from tests/test_authentication.py. The net change against the base branch is a single test deletion — no changes to production authentication code ship in the final diff.

• The two-commit history shows a fix for _is_safe_next_url was added (rejecting URLs starting with \\) and then immediately reverted, with the second commit message labelling the test "invalid". The rationale is sound for WHATWG-compliant browsers: \\evil.com is resolved as /evil.com on the current host, not as an external redirect — unlike \\/evil.com which Chrome normalises to //evil.com and the existing test_slash_backslash_redirect_rejected correctly rejects.
• The PR description is stale: it says "GitHub Actions will run test_backslash_redirect_rejected" but that test no longer exists after the second commit, leaving no CI coverage of the single-leading-backslash behaviour.
• _is_safe_next_url has no comment explaining why the single-leading-backslash case intentionally passes, which risks a future contributor re-adding a rejection (or re-adding the test with the same wrong expectation).

Confidence Score: 4/5

Safe to merge for WHATWG browsers, but the leading-backslash redirect is now untested and undocumented; a follow-up comment in _is_safe_next_url explaining the intentional decision is strongly recommended.

The only production code change was introduced and immediately reverted within the PR, so nothing about the live validator actually changes. The concern is that the test deletion leaves the leading-backslash redirect as an undocumented, untested pass-through — whether it stays on-host depends entirely on the client URL parser. Modern WHATWG browsers are safe; the edge-case risk is non-WHATWG clients such as IE WebView-compat mode or some HTTP proxies.

Files Needing Attention: tests/test_authentication.py — removal of the backslash-redirect test should be paired with a comment in redash/authentication/init.py documenting why the single-leading-backslash case is intentionally allowed.

Important Files Changed

Filename	Overview
tests/test_authentication.py	Removes test_backslash_redirect_rejected (which tested that \evil.com is blocked); the PR description still references this test running in CI, creating a misleading paper trail. The validator intentionally allows \evil.com (WHATWG browsers resolve it to the same host), but the rationale is undocumented.

Sequence Diagram

sequenceDiagram
    participant Attacker
    participant Browser
    participant Server
    participant Validator as _is_safe_next_url

    Attacker->>Browser: "Phishing link /login?next=%5Cevil.com"
    Browser->>Server: "POST /login?next=\evil.com"

    Server->>Validator: _is_safe_next_url("\evil.com")
    Note over Validator: url.replace("\\","/") to /evil.com<br/>urlparse netloc empty passes
    Validator-->>Server: True (allowed)

    Server-->>Browser: 302 Location: \evil.com

    Note over Browser: WHATWG treats backslash as slash<br/>Resolves to /evil.com on same host
    Browser->>Server: GET /evil.com (same host, safe)

    Note over Server,Validator: Compare /\evil.com<br/>replace gives //evil.com<br/>urlparse netloc=evil.com REJECTED

Loading

Comments Outside Diff (1)

1. tests/test_authentication.py, line 342-351 (link)

   Removed test leaves validator behaviour undocumented

   _is_safe_next_url("\\evil.com") still returns True — get_next_path will return \evil.com unchanged, and Flask will emit Location: \evil.com. The PR description states "GitHub Actions will run test_backslash_redirect_rejected as part of backend-unit-tests", but that test was deleted in the second commit, so CI will no longer verify this behaviour at all.

   The rationale for allowing a leading-backslash URL (WHATWG browsers resolve \evil.com to the current host's /evil.com) should be captured as an inline comment in _is_safe_next_url, and the PR description updated to reflect the actual final state. Without it, a future hardening effort could re-add a rejection that breaks legitimate paths, or conversely leave the vector open if a non-WHATWG client is ever added to the stack.

_{Reviews (2): Last reviewed commit: "Remove invalid backslash redirect test" | Re-trigger Greptile}

[cubic-dev-ai]

No issues found across 1 file

_{Re-trigger cubic}