fix: harden login redirect validation and test-auth gate (F-06, F-02)

[numarasSigmaSoftware]

Closes #1127 Closes #1131

Summary
Two security findings from the audit addressed in this PR:

F-06 (Medium): Open redirect via login next parameter — user-supplied redirect targets were stored and replayed without validation, allowing a post-authentication redirect to an attacker-controlled domain. F-02 (Critical): Test auth bypass — ADCP_AUTH_TEST_MODE=true alone was sufficient to enable /test/auth regardless of a tenant's auth_setup_mode setting, allowing privileged session issuance after an operator had explicitly disabled Setup Mode via the Admin UI. F-06 — Open redirect fix
Added _safe_redirect() to src/admin/blueprints/auth.py:

def _safe_redirect(url: str | None, fallback: str) -> str:
"""Return url only if it is a safe local path, otherwise fallback."""
decoded = unquote(url).strip()
if parts.scheme or parts.netloc or decoded.startswith(("//", "\\")) ...:
logger.warning("[SECURITY] Rejected unsafe redirect URL: %r", url)
return fallback
return decoded
Applied at both the ingestion point and every redirect sink:

/login and /tenant//login now call _safe_redirect() before storing login_next_url in session — absolute URLs, scheme-relative URLs, and encoded bypass variants are all rejected at entry. All four post-authentication redirect sinks (super-admin callback, single-tenant callback, multi-tenant tenant selector callback, and test_auth) now call _safe_redirect() again before the final redirect() call — defence in depth at the sink. F-02 — Test auth gate fix
Gate change in test_auth():

Before — vulnerable: env var alone was sufficient if not env_test_mode and not tenant_setup_mode:

abort(404)

After — both required

if not env_test_mode or not tenant_setup_mode:
abort(404)
A tenant operator disabling Setup Mode via the Admin UI now immediately blocks test auth for that tenant, regardless of the deployment-level ADCP_AUTH_TEST_MODE flag. The env var is now a necessary but not sufficient condition.

Hardcoded super-admin bypass removed:

Before — bypassed test_users table entirely

if is_super_admin(email) and password == "test123":
...

After — all credentials go through test_users, no special cases if email in test_users and test_users[email]["password"] == password:

...

Production hard-block added via is_admin_production() — PRODUCTION=true or ENVIRONMENT=production causes an immediate 404 before any credential check, regardless of all other flags.

Files changed
File Change
src/admin/blueprints/auth.py _safe_redirect(), validated ingestion + sinks, F-02 gate, removed bypass src/admin/utils/helpers.py Added is_admin_production() src/admin/utils/init.py Exported is_admin_production tests/unit/test_auth_setup_mode.py Updated TestTestAuthEndpointLogic for AND gate tests/unit/test_test_auth_production_guard.py Full gate coverage + TestSuperAdminCredentialPath Test results
25 new/updated tests passing. Pre-existing webhook DNS failures in test_webhook_delivery.py are unrelated (sandbox DNS restriction).

[KonstantinMirin]

The _safe_redirect() implementation is solid — defense in depth at both ingestion and sink, URL-decoding before validation, correct rejection of scheme-relative and backslash variants. The and→or gate fix and production hard-block are both correct. Removing the hardcoded super-admin bypass is a welcome cleanup.

Two concerns:

1. Several tests verify boolean arithmetic, not application behavior

Tests like test_test_auth_blocked_when_env_var_only reconstruct the boolean logic from the source and assert the result:

env_test_mode = os.environ.get("ADCP_AUTH_TEST_MODE", "").lower() == "true"
tenant_setup_mode = False
should_abort = not env_test_mode or not tenant_setup_mode
assert should_abort is True

This tests that not True or not False == True, which is a Python language property, not application behavior. If someone changes the gate logic in auth.py, these tests still pass because they don't call the actual code.

For a reference on how we test Flask endpoints in this repo, see PR #1137 — tests there use Flask's test client to exercise the actual endpoint. Could you convert the gate tests to POST to /test/auth via the test client so they'd catch a real regression?

2. Regression tests for the open redirect

The _safe_redirect() unit logic is tested implicitly through the boolean tests, but there's no regression test that reproduces the original attack: GET /admin/login?next=https://evil.example.com → authenticate → verify redirect stays internal. A test that exercises that full flow through Flask's test client would give us confidence the vulnerability can't be reintroduced.

Can approve once the tests exercise actual endpoints rather than reimplementing their logic.

[numarasSigmaSoftware]

Both points addressed — tests now exercise actual endpoints.

1. Gate tests converted to Flask test client

TestTestAuthEndpointLogic in test_auth_setup_mode.py and TestProductionHardBlock in test_test_auth_production_guard.py are replaced with Flask test client tests that POST to the actual /test/auth endpoint and assert the HTTP status code. Each test creates a real Flask app, mocks only get_db_session to return a tenant with the appropriate auth_setup_mode, and sets the relevant env vars. If someone changes the gate logic in auth.py these tests will now catch it.

2. Open redirect regression tests

Added TestOpenRedirectRejection in test_test_auth_production_guard.py with three tests:

test_external_next_url_not_stored_at_login — GET /login?next=https://evil.example.com confirms _safe_redirect() at ingestion rejects the external URL so login_next_url is never stored in session
test_external_next_url_does_not_redirect_to_attacker_domain — reproduces the original attack: prime session with evil next → POST /test/auth with valid credentials → assert Location header does not contain evil.example.com
test_session_injected_next_url_rejected_at_auth_sink — defense in depth: directly injects login_next_url = "https://evil.example.com" into the session and confirms the /test/auth sink still rejects it via its own _safe_redirect() call independently of the ingestion check

[KonstantinMirin]

Nicholas, the Flask test client conversion and open redirect regression tests are solid work. The defense-in-depth test (injecting malicious URL directly into session to verify the sink independently) is a good pattern.

We've documented our testing and architecture patterns in PR #1145. Your feedback on that doc is welcome — these are the principles we're trying to establish for codebase quality, and contributor input helps improve them. Two items to address:

1. TestSuperAdminCredentialPath must exercise actual endpoints. Two tests still don't call any application code — one reconstructs the test_users dict in test code and asserts against it, the other reads the source file as text. Neither would catch a regression if the endpoint behavior changed. See Section 4 — convert these to Flask test client tests like the gate tests.

2. Extract the shared Flask client helper. _make_client_with_tenant() in test_test_auth_production_guard.py and _make_flask_client() in test_auth_setup_mode.py are the same function with different names. Extract to a shared helper or conftest fixture — we don't want to introduce new duplication in the same PR that fixes a security issue. See Section 8.

[KonstantinMirin]

DRY concern addressed — make_auth_test_client factory fixture in conftest is clean. The test audit annotations on pre-existing SUSPECT tests (TestSetupModeLogic, TestTenantLoginLogic, TestUsersEndpointConfig) are a good way to document known debt that you didn't introduce.

Two items:

1. TestSuperAdminCredentialPath needs rework. This class is in test_test_auth_production_guard.py, which was created in this PR — these tests are new, not pre-existing. The dict-reconstruction test and source-file-grep test need to be converted to Flask test client endpoint tests, same as you did for the gate tests.

2. File a follow-up issue for the 6 pre-existing SUSPECT tests you annotated in test_auth_setup_mode.py. Those were there before your PR — annotating them is the right call, but we need it tracked so it doesn't get forgotten.

[numarasSigmaSoftware]

@KonstantinMirin thanks for the review! Test is now fixed and issue #1149 is open so we don't lose track of it

[KonstantinMirin]

Thanks Nicholas — all feedback addressed cleanly. The TestSuperAdminCredentialPath rework is solid (full session state assertions via Flask test client), the shared make_auth_test_client fixture keeps things DRY, and #1149 properly tracks the pre-existing SUSPECT debt. Good work on this one.