Share OAuth next-path control character validation

[catcherintheroad-hub]

Bounty #846

Summary

• reuse the shared contains_control_character() helper in safe_next_path() instead of maintaining two inline ordinal scans;
• preserve the existing raw and decoded next-path rejection behavior;
• add a regression case for a percent-encoded newline in the decoded OAuth next path.

Duplicate check

• MRWK bounty: 100 MRWK - code cleanup and maintainability improvements #846 is live/open as MergeWork bounty row 112 with effective award capacity before submission.
• gh search prs "safe_next_path" --repo ramimbo/mergework --state open -> no results.
• gh search prs "control character auth" --repo ramimbo/mergework --state open -> no results.
• The only open app/auth.py PR I found is Validate OAuth scalar query parameters #825, which validates repeated OAuth scalar query parameters; this PR is separate and only shares existing control-character validation inside safe_next_path().

Validation

• uv run --python 3.12 --extra dev python -m pytest tests/test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths tests/test_wallet_api.py::test_oauth_next_path_rejects_redirect_ambiguity tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q -> 17 passed, 1 existing Starlette/httpx warning.
• uv run --python 3.12 --extra dev python -m pytest tests/test_wallet_api.py tests/test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths -q -> 57 passed, 1 existing Starlette/httpx warning.
• uv run --python 3.12 --extra dev ruff check app/auth.py tests/test_security.py -> passed.
• uv run --python 3.12 --extra dev ruff format --check app/auth.py tests/test_security.py -> 2 files already formatted.
• uv run --python 3.12 --extra dev mypy app/auth.py -> success.
• uv run --python 3.12 --extra dev python scripts/docs_smoke.py -> docs smoke ok.
• git diff --check -> clean.
• git merge-tree --write-tree origin/main HEAD -> clean tree cafc60c42d14e4d36c7e563d4833b571f1576c5e.

Scope

Maintainability-only cleanup in OAuth next-path validation. No OAuth flow semantics, redirect target behavior, cookie behavior, wallet behavior, ledger behavior, treasury behavior, payout behavior, admin-token behavior, private data, exchange, bridge, cash-out, or MRWK price behavior changed.

Summary by CodeRabbit

Bug Fixes

• Strengthened security of redirect target validation by implementing enhanced control character detection and blocking mechanisms to mitigate potential header-injection and control character attacks in authentication flows
• Enhanced overall protection against malicious redirect attempts using various encoding techniques, including percent-encoded payloads and other obfuscation methods designed to circumvent existing validation checks

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 18b4704c-9613-46a5-9136-b8c0511b2cec

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 6c7dd2e.

📒 Files selected for processing (2)

• app/auth.py
• tests/test_security.py

---

📝 Walkthrough

Walkthrough

The PR refactors OAuth redirect-path validation to use a delegated contains_control_character helper instead of inline ASCII checks. The function now validates both the raw next_path and its URL-decoded form. A test case is added to verify percent-encoded newline and Location: header-like payloads are rejected.

Changes

Redirect path control character validation

Layer / File(s)	Summary
Validation refactoring and test coverage app/auth.py, tests/test_security.py	safe_next_path delegates control character detection to contains_control_character for both raw and decoded paths. Import added to app/auth.py. Test case added to verify percent-encoded header-like input is rejected.

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly names the changed surface ('Share OAuth next-path control character validation') and accurately reflects the main refactoring work in the changeset.
Description check	✅ Passed	The description covers all required sections: summary of changes, evidence of duplicate/scope checks, comprehensive validation results, and explicit scope boundaries.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene	✅ Passed	PR contains only technical code changes to OAuth validation (app/auth.py and tests/test_security.py) with no modifications to README, docs, or public artifacts that could contain prohibited claims.
Bounty Pr Focus	✅ Passed	Commit does not reference Bounty #N or Refs #N in the commit message, so the custom check's applicability condition is not met. The check is not applicable to this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[pqmfei]

Reviewed current head 6c7dd2e0d1be0251845113d27b3670210681f6ab as a non-author.

Evidence checked:

• inspected app/auth.py and tests/test_security.py;
• confirmed safe_next_path() now delegates raw and decoded control-character checks to the shared contains_control_character() helper, preserving the existing C0/C1 rejection boundary;
• confirmed raw path checks still run before decoded path checks for empty, external //, overlong, and backslash inputs;
• confirmed the new percent-encoded newline regression covers decoded header-like redirects that the raw string alone would not expose;
• checked GitHub state before review: PR open, mergeStateStatus=CLEAN, mergeable=MERGEABLE, hosted Quality/readiness/docs/image check successful, CodeRabbit successful/no actionable comments, and no prior human reviews or #838 claim for PR #906 visible.

Validation run locally on this exact head:

• .\.venv\Scripts\python.exe -m pytest tests\test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths tests\test_wallet_api.py::test_oauth_next_path_rejects_redirect_ambiguity tests\test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q -> 17 passed, 1 existing Starlette/httpx warning.
• .\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py tests\test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths -q -> 57 passed, 1 existing Starlette/httpx warning.
• .\.venv\Scripts\python.exe -m ruff check app\auth.py tests\test_security.py -> passed.
• .\.venv\Scripts\python.exe -m ruff format --check app\auth.py tests\test_security.py -> 2 files already formatted.
• .\.venv\Scripts\python.exe -m mypy app\auth.py -> success.
• .\.venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok.
• git diff --check origin/main...HEAD -> clean.
• git merge-tree --write-tree origin/main HEAD -> clean tree cafc60c42d14e4d36c7e563d4833b571f1576c5e.

No blocker found. Scope is OAuth next-path validation cleanup/test coverage only; no OAuth provider configuration, cookie signing, wallet, ledger, treasury, payout, admin-token, private data, bridge, exchange, cash-out, or MRWK price behavior changed.