fix(authenticator): redirect failed /auth/callback into the SPA with auth_error

[cyberantonz]

Summary

Fixes #2032. A failed OIDC callback answered problem+json straight to the browser — no page is loaded at /auth/callback (it's an IdP redirect target), so an expired login state (the 300 s Redis TTL), an IdP-reported error, a replayed callback, or a denied person dead-ended the login on raw JSON.

Every browser-facing callback failure now 302s to default_return_to with a fixed auth_error=<reason> query parameter so the SPA can restart the login from scratch:

reason	condition	SPA behavior (insight-front#239)
state_expired	unknown / expired / already-consumed state	auto-retry once, then error screen
idp_error	IdP redirected back with error= (detail logged, incl. error_description)	auto-retry once, then error screen
invalid_callback	missing code/state	auto-retry once, then error screen
exchange_failed	code exchange / id_token validation failed	auto-retry once, then error screen
access_denied	no tenant resolved, unknown person, unknown view-as target	error screen immediately

The reason vocabulary is fixed — nothing IdP- or caller-supplied reaches the Location header (default_return_to is now also validated: site-relative, no fragment, no control chars). Rate-limit (429) and internal (5xx) responses stay problem+json. The one-shot state consumption, session-fixation guard, and all audit emissions are unchanged; PersonError became unused and is dropped.

Specs updated in the same change: PRD §5.1 + endpoint table, DESIGN §3.3 callback failure contract, regenerated openapi.json (cfs validate green).

Deploy order

Deploy/merge insight-front#239 first: without the FE loop guard a persistent failure such as an unknown person would bounce between the SPA's auto-login and the IdP.

Test plan

• run-e2e.sh — full suite green (12 tests), including a new failed_callback_redirects_into_the_spa_with_auth_error e2e and the updated rate-limit (400→302) and unknown-override-target (403→302, still cookie-less) assertions; endpoint-coverage gate passes.
• cargo test -p authenticator — 52 unit tests (2 new for the redirect builder); clippy/fmt clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Login callback failures now redirect back to the SPA with a consistent auth_error reason, helping users restart authentication.
  • Added clear handling for invalid callbacks, expired state, identity-provider errors, exchange failures, and access denial.
  • Rate-limited and internal server errors continue to use standard error responses.
  • Startup validation now rejects unsafe default redirect destinations.

• Documentation

  • Documented the callback failure redirect contract and supported error reasons in API and design documentation.

[coderabbitai]

📝 Walkthrough

Walkthrough

The authenticator’s /auth/callback now redirects browser-facing failures to default_return_to with fixed auth_error reasons. Configuration validation, API documentation, error types, callback handling, and end-to-end tests were updated accordingly.

Changes

Authenticator callback redirect contract

Layer / File(s)	Summary
Redirect contract and target validation src/backend/services/authenticator/src/config.rs, docs/components/backend/authenticator/*, src/backend/services/authenticator/src/api/mod.rs	Documents callback success and failure redirects, preserves 429 and 5xx problem responses, and validates default_return_to as a safe site-relative path.
Callback failure redirect handling src/backend/services/authenticator/src/api/handlers.rs, src/backend/services/authenticator/src/api/error.rs, src/backend/clippy.toml	Maps IdP errors, invalid parameters, expired state, exchange failures, and access denials to fixed auth_error values and removes PersonError.
Redirect behavior verification src/backend/services/authenticator/src/api/handlers.rs, src/backend/services/authenticator/tests/*	Tests redirect URL construction, callback failure responses, override denial behavior, and rate-limit status handling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant IdP
  participant CallbackHandler
  participant Browser
  participant SPA
  IdP->>CallbackHandler: callback parameters or error
  CallbackHandler->>Browser: 302 redirect with auth_error
  Browser->>SPA: follow default_return_to

Loading

Possibly related PRs

• constructorfabric/insight#1686: Introduced the authenticator callback machinery extended by this redirect contract.
• constructorfabric/insight#1851: Also modified authenticator callback endpoint behavior.
• constructorfabric/insight#1944: Covered related view-as override denial behavior.

Suggested reviewers: aleksdotbar, ktursunov, mitasovr

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: failed /auth/callback requests now redirect to the SPA with an auth_error parameter.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/backend/clippy.toml (1)

40-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the required CF Studio path.

+cf-studio-path = ".cf-studio"
+
 doc-valid-idents = [

As per coding guidelines, **/*.toml: Set cf-studio-path to .cf-studio.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/clippy.toml` around lines 40 - 44, Update the clippy.toml
configuration by adding the required cf-studio-path setting with the value
.cf-studio, alongside the existing top-level configuration entries such as
doc-valid-idents.

Source: Coding guidelines

src/backend/services/authenticator/src/api/handlers.rs (1)

239-245: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Sanitize the exchange error before logging it.

Line 242 logs the full error chain, which the comment says includes the IdP error_description; unlike the direct callback path, it is neither control-character stripped nor capped. Apply the same sanitization to format!("{e:#}") before emitting it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/authenticator/src/api/handlers.rs` around lines 239 -
245, Sanitize the full error-chain string in the Err branch of the OIDC code
exchange/id_token validation handler before passing it to tracing::warn!. Reuse
the existing direct-callback sanitization and length-capping helper or pattern,
applying it to format!("{e:#}") while preserving the existing warning and
login_error_redirect behavior.

src/backend/services/authenticator/tests/e2e_ratelimit.rs (1)

148-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the callback state unique and prove the transition.

The fixed state can retain an exhausted Redis bucket across rapid reruns, letting the first request return 429 and still pass this test. Generate a per-run state and assert at least one 302 occurs before the 429.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/authenticator/tests/e2e_ratelimit.rs` around lines 148 -
161, Update the callback-bucket scenario in the rate-limit end-to-end test to
generate a unique state value for each run, then use that value in every
callback request. Track the response sequence and require at least one 302
before accepting a 429, while preserving the existing request loop and status
handling.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backend/services/authenticator/src/api/mod.rs`:
- Around line 87-93: Update the `/auth/callback` operation builder in
`src/backend/services/authenticator/src/api/mod.rs` to register the canonical
429 rate-limit and internal server-error problem+json responses alongside the
existing 302 response. Regenerate
`docs/components/backend/authenticator/openapi.json` so its callback operation
includes the same response definitions at lines 144-149.

In `@src/backend/services/authenticator/src/config.rs`:
- Around line 415-420: Update the default_return_to validation in the relevant
config validation method to reject any backslash characters before accepting the
site-relative path, preventing values such as `/\evil.example` from bypassing
the scheme-relative redirect guard. Add validation coverage for this
backslash-based redirect case while preserving the existing fragment,
control-character, and slash checks.

---

Outside diff comments:
In `@src/backend/clippy.toml`:
- Around line 40-44: Update the clippy.toml configuration by adding the required
cf-studio-path setting with the value .cf-studio, alongside the existing
top-level configuration entries such as doc-valid-idents.

In `@src/backend/services/authenticator/src/api/handlers.rs`:
- Around line 239-245: Sanitize the full error-chain string in the Err branch of
the OIDC code exchange/id_token validation handler before passing it to
tracing::warn!. Reuse the existing direct-callback sanitization and
length-capping helper or pattern, applying it to format!("{e:#}") while
preserving the existing warning and login_error_redirect behavior.

In `@src/backend/services/authenticator/tests/e2e_ratelimit.rs`:
- Around line 148-161: Update the callback-bucket scenario in the rate-limit
end-to-end test to generate a unique state value for each run, then use that
value in every callback request. Track the response sequence and require at
least one 302 before accepting a 429, while preserving the existing request loop
and status handling.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 32defa91-1b7d-49dd-aa3a-363c856e5036

📥 Commits

Reviewing files that changed from the base of the PR and between b8720c7 and e6eb499.

📒 Files selected for processing (11)

• docs/components/backend/authenticator/DESIGN.md
• docs/components/backend/authenticator/PRD.md
• docs/components/backend/authenticator/openapi.json
• src/backend/clippy.toml
• src/backend/services/authenticator/src/api/error.rs
• src/backend/services/authenticator/src/api/handlers.rs
• src/backend/services/authenticator/src/api/mod.rs
• src/backend/services/authenticator/src/config.rs
• src/backend/services/authenticator/tests/e2e_login_loop.rs
• src/backend/services/authenticator/tests/e2e_override.rs
• src/backend/services/authenticator/tests/e2e_ratelimit.rs

💤 Files with no reviewable changes (1)

• src/backend/services/authenticator/src/api/error.rs

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)src/backend/services/authenticator/src/api/mod.rs$|docs/components/backend/authenticator/openapi\.json$|PRD|DESIGN' || true

echo "== target route registration =="
if [ -f src/backend/services/authenticator/src/api/mod.rs ]; then
  nl -ba src/backend/services/authenticator/src/api/mod.rs | sed -n '1,140p'
fi

echo "== auth error/response mentions in repository =="
rg -n "auth_error|state_expired|idp_error|invalid_callback|exchange_failed|access_denied|429|problem|json|no_content_response|content_response|callback" src backend docs 2>/dev/null | head -n 250

Repository: constructorfabric/insight

Length of output: 6468

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)src/backend/services/authenticator/src/api/mod.rs$|docs/components/backend/authenticator/openapi\.json$|PRD|DESIGN' || true

echo "== target route registration =="
if [ -f src/backend/services/authenticator/src/api/mod.rs ]; then
  nl -ba src/backend/services/authenticator/src/api/mod.rs | sed -n '1,160p'
fi

echo "== relevant docs/PRD/design mentions =="
for f in $(git ls-files | rg '(^|/)(PRD|DESIGN|README|.*\.md)$'); do
  if rg -q "callback|auth_error|state_expired|idp_error|invalid_callback|exchange_failed|access_denied|429|internal problem|problem.*json|rate limit" "$f" 2>/dev/null; then
    echo "--- $f ---"
    rg -n -C 2 "callback|auth_error|state_expired|idp_error|invalid_callback|exchange_failed|access_denied|429|internal problem|problem.*json|rate limit" "$f" || true
  fi
done

echo "== OpenAPI callback endpoints =="
if [ -f docs/components/backend/authenticator/openapi.json ]; then
  python3 - <<'PY'
import json
from pathlib import Path
p=Path('docs/components/backend/authenticator/openapi.json')
data=json.loads(p.read_text())
for name,pathitem in data.get('paths',{}).items():
    for m,(op,) in [(k,v) for k,v in pathitem.items() if k in {'get','post','put','patch','delete'}]:
        if 'callback' in str(pathitem.get('operationId','')).lower() or 'callback' in str(pathitem).lower():
            print(f"{name} {m}: {pathitem.get('operationId')} responses={list(pathitem.get('responses',{}).keys())}")
            if '302' in pathitem.get('responses',{}):
                print(pathitem['responses']['302'])
PY
fi

echo "== response registration declarations =="
rg -n "no_content_response|content_response|problem|json|status_code|ResponseBuilder|responses\\(" src/backend/services/authenticator src/backend 2>/dev/null | head -n 300

echo "== behavioral check for OpenAPI 302 only vs other possible code-emitted responses =="
python3 - <<'PY'
import re, json, subprocess
openapi_path='docs/components/backend/authenticator/openapi.json'
if not __import__('pathlib').Path(openapi_path).exists():
    print("openapi missing")
    raise SystemExit
data=json.load(open(openapi_path))
matches={}
for path,methods in data.get('paths',{}).items():
    for verb,op in methods.items():
        if isinstance(verb,str) and verb.lower() in {'get','post','put','patch','delete','options'}:
            oid=op.get('operationId')
            if oid and 'callback' in oid.lower():
                responses=[]
                for r in op.get('responses',{}):
                    status=int(r) if str(r).isdigit() else -1
                    responses.append((status,r))
                matches[oid]=(path,verb,responses)
print("callback operation responses:")
for k,v in sorted(matches.items()):
    print(k, v[0], v[1], "statuses", sorted(v[2], key=lambda x:x[0]))
print("has_429_callback=", any(429 in {s for s,r in v[2]} for v in matches.values()))
print("has_5xx_callback=", any(any(isinstance(s,int) and 500 <= s < 600 for s,_ in v[2]) for v in matches.values()))
PY

Repository: constructorfabric/insight

Length of output: 6468

---

🏁 Script executed:

#!/bin/bash
set -euo or
echo "== sed route registration =="
sed -n '1,180p' src/backend/services/authenticator/src/api/mod.rs

echo "== relevant PRD/DESIGN refs =="
for f in docs/components/backend/authenticator/PRD.md docs/components/backend/authenticator/DESIGN.md; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    grep -n -C 3 -i "callback\\|429\\|rate limit\\|problem\\|json\\|auth_error\\|state_expired\\|idp_error\\|invalid_callback\\|exchange_failed\\|access_denied" "$f" || true
  fi
done

echo "== openteapi callback response lines =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path('docs/components/backend/authenticator/openapi.json')
d=json.loads(p.read_text())
for path, methods in d.get('paths', {}).items():
    for verb in ('get', 'post', 'put', 'patch', 'delete'):
        op=methods.get(verb)
        if not op:
            continue
        oid=op.get('operationId', '')
        if 'callback' in oid.lower() or 'callback' in path.lower():
            print(f"{verb.upper()} {path} operationId={oid}")
            for k,v in op.get('responses', {}).items():
                print(f"  {k} {v.get('description')}")
PY

Repository: constructorfabric/insight

Length of output: 206

---

🏁 Script executed:

#!/bin/bash
set -eu

echo "== sed route registration =="
sed -n '1,180p' src/backend/services/authenticator/src/api/mod.rs

echo "== relevant PRD/DESIGN refs =="
for f in docs/components/backend/authenticator/PRD.md docs/components/backend/authenticator/DESIGN.md; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    grep -n -C 3 -i "callback\\|429\\|rate limit\\|problem\\|json\\|auth_error\\|state_expired\\|idp_error\\|invalid_callback\\|exchange_failed\\|access_denied" "$f" || true
  fi
done

echo "== OpenAPI callback response lines =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path('docs/components/backend/authenticator/openapi.json')
d=json.loads(p.read_text())
for path, methods in d.get('paths', {}).items():
    for verb in ('get', 'post', 'put', 'patch', 'delete'):
        op=methods.get(verb)
        if not op:
            continue
        oid=op.get('operationId', '')
        if 'callback' in oid.lower() or 'callback' in path.lower():
            print(f"{verb.upper()} {path} operationId={oid}")
            for k,v in op.get('responses', {}).items():
                print(f"  {k} {v.get('description')}")
PY

Repository: constructorfabric/insight

Length of output: 44650

---

Expose callback rate-limit and internal problem responses in OpenAPI.

The authenticator contract leaves /auth/callback as 302 only, while PRD.md and DESIGN.md specify that rate-limit (429) and internal server errors remain the canonical failure responses and serialize as problem+json. Register the callback’s 429 and internal problem responses with the operation builder and regenerate docs/components/backend/authenticator/openapi.json so generated OpenAPI consumers have the full contract.

📍 Affects 2 files

• src/backend/services/authenticator/src/api/mod.rs#L87-L93 (this comment)
• docs/components/backend/authenticator/openapi.json#L144-L149

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/authenticator/src/api/mod.rs` around lines 87 - 93,
Update the `/auth/callback` operation builder in
`src/backend/services/authenticator/src/api/mod.rs` to register the canonical
429 rate-limit and internal server-error problem+json responses alongside the
existing 302 response. Regenerate
`docs/components/backend/authenticator/openapi.json` so its callback operation
includes the same response definitions at lines 144-149.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject backslash-based scheme-relative redirects.

Line 416 accepts /\evil.example; browsers can normalize this to //evil.example, bypassing the // guard and restoring an external redirect. Reject backslashes and add this case to validation coverage.

Proposed fix

             self.default_return_to.starts_with('/')
                 && !self.default_return_to.starts_with("//")
+                && !self.default_return_to.contains('\\')
                 && !self.default_return_to.contains('#')

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	anyhow::ensure!(
	self.default_return_to.starts_with('/')
	&& !self.default_return_to.starts_with("//")
	&& !self.default_return_to.contains('#')
	&& !self.default_return_to.chars().any(char::is_control),
	"default_return_to must be a site-relative path without a fragment"
	anyhow::ensure!(
	self.default_return_to.starts_with('/')
	&& !self.default_return_to.starts_with("//")
	&& !self.default_return_to.contains('\\')
	&& !self.default_return_to.contains('#')
	&& !self.default_return_to.chars().any(char::is_control),
	"default_return_to must be a site-relative path without a fragment"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/authenticator/src/config.rs` around lines 415 - 420,
Update the default_return_to validation in the relevant config validation method
to reject any backslash characters before accepting the site-relative path,
preventing values such as `/\evil.example` from bypassing the scheme-relative
redirect guard. Add validation coverage for this backslash-based redirect case
while preserving the existing fragment, control-character, and slash checks.