AuthInterceptor returns JSON for HTML navigations — users see raw {"status":"Unauthorized"} #2
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
AuthInterceptor::interceptalways responds withapplication/json(makeUnauthorized/makeForbidden) when a request is rejected. That's correct for/api/*callers, but when a browser navigates to a non-public HTML route (e.g. an expired/invalid/set-password?token=…link, a stale/adminbookmark after logout, or any deep link before login), the user sees a raw JSON blob in the browser window instead of a meaningful page.Observed while debugging the
/set-password401 in palibu (fixed in46971acby stripping the query string — but a valid path with an invalid token still returns JSON to the browser).Proposed approach
Content-negotiate in
AuthInterceptor:/api/, orAcceptheader prefersapplication/json, orX-Requested-With: XMLHttpRequestis present → keep current JSON 401/403.302redirect to a configurable login/error route (e.g./login?next=…or/).The target route should be supplied by
IAuthPolicy/IRuntimeConfigso each downstream app can decide where to send unauthenticated browsers (template apps could default to/).Acceptance criteria
fetch()/axioscalls from the SPA still receive401/403JSON so client-side error handling keeps working./loginpath in the library).Context
Found while investigating the
/set-passwordUX in palibu (see oatpp-authkit commit46971acfor the related query-string fix).Agent Evaluation
Feasibility: Straightforward.
AuthInterceptor::intercept(header-only, ~180 LOC) is the only call site formakeUnauthorized/makeForbidden. Adding a content-negotiation helper plus a policy hook is a localised change.Impact: Direct UX win for any downstream that mixes server-rendered or static HTML routes with the API (palibu, fewo-webapp's set-password and reset flows). Today the user sees a raw
{"status":"Unauthorized"}blob; afterwards they land on a real page. No effect on JSON-only consumers.Effort: Small.
Recommendation: Accept.
Implementation plan
AuthInterceptor.hpp— content-negotiation helper. Addstatic bool wantsJson(const IncomingRequest&): true if path starts with/api/, ORX-Requested-With: XMLHttpRequest, ORAcceptheader prefersapplication/jsonovertext/html(substring scan is fine — the only goal is to distinguishfetch/axiosfrom a browser navigation).IAuthPolicy.hpp— new virtual. Addvirtual std::optional<std::string> unauthenticatedRedirect(const std::string& path) { return std::nullopt; }(default: keep current behaviour). Consumers that want browser redirects override it to return e.g."/login?next=" + urlencode(path)or"/". Returningnulloptmeans "no override — fall through to JSON 401".makeUnauthorized/makeForbidden— branch on negotiation. New signature takes the request. IfwantsJson(req)→ existing JSON response. Else ifpolicy->unauthenticatedRedirect(path)returns a value →302withLocation:set. Else → minimal HTML error page (<!doctype html><title>401</title><h1>Unauthorized</h1>) withContent-Type: text/html; charset=utf-8.makeForbiddenso logged-in-but-unauthorized browser nav also gets a real page rather than JSON. Use a separate policy hookforbiddenRedirect(or reuse the same one — see Decision needed).test/directory; CI/build is header-onlyfind_packageonly). Adding the first integration test means standing up a minimal oatpp test harness — non-trivial scope creep. Pragmatic alternative: add the test in the consumer that motivated this (palibu or fewo-webapp), where a Selenium/HTTP harness already exists. Note this in the PR description.README.mdunder theIAuthPolicysection showing the new override and how to wireAccept-based negotiation.Decision needed
Check one (edit this comment):
unauthenticatedRedirect(path)covers both cases; logged-in-but-forbidden browser nav also redirects there (typical pattern: bounce to/). Simpler API.unauthenticatedRedirect(path)(login page) andforbiddenRedirect(path)(e.g. a/forbiddeninfo page). More flexible, two virtuals to override.Test coverage decision
Check one (edit this comment):
enable_testing()+ a minimal oatpp app fixture; future tests have somewhere to land but this PR grows considerably.Evaluated #2 — Small, recommend accept; two decision checkboxes (single vs split redirect hook; consumer-side test vs bootstrap test harness).
Re-evaluated #2 — owner picked T2 (bootstrap test harness); A/B redirect-hook decision still pending, will default to A next cycle if unchecked.
Re-evaluation
Owner has accepted the issue and selected Option T2 — Bootstrap an oatpp-authkit test harness now for test coverage. Acknowledged: I will land CMake
enable_testing()+ a minimal oatpp app fixture in this PR so future tests have somewhere to live.One decision still pending: Option A vs Option B (single vs split redirect hook). The hook signature is part of the public
IAuthPolicyAPI, so flipping it later is a breaking change for any consumer that overrides it. Worth nailing down before implementation.Default if not picked: Option A (single
unauthenticatedRedirect(path)hook used for both 401 and 403). Rationale: the most common downstream wiring is "bounce browsers to/or/login" regardless of whether the failure was 401 or 403; consumers that want to differentiate can branch inside the hook on whether the request was authenticated. Two hooks add API surface for a case I have not seen in fewo-webapp / palibu / webapp-template.Will hold off on implementation for one more poll cycle; if the A/B box is still unchecked next pass, I will proceed with Option A.
Implemented in
abf6153— content-negotiating JSON vs HTML/redirect (Option A), test harness bootstrapped undertest/(Option T2), README updated. ctest passes.