#2: Browser-friendly 401/403 — content-negotiate JSON vs HTML/redirect

AuthInterceptor previously returned application/json for every rejection, which is wrong for browser navigation: the user followed a /set-password link and saw a raw {"status":"Unauthorized"} blob. Add wantsJson() negotiation (path /api/* OR X-Requested-With OR Accept prefers application/json over text/html) and an IAuthPolicy hook unauthenticatedRedirect(path) that lets consumers bounce browser navigations to a landing/login page. JSON callers (fetch/axios) still get JSON 401/403. Default policy returns nullopt → minimal HTML error page, never raw JSON to a browser. Same hook covers both 401 and 403 (decision Option A on the issue) so consumers wire one redirect target for both unauth and forbidden cases. Bootstrap a minimal test harness (decision Option T2): CMake option OATPP_AUTHKIT_BUILD_TESTS gates enable_testing() + a tests subdir. Adds test_negotiation covering wantsJson + urlEncode. No third-party test framework — assertions use <cassert> + a tiny REQUIRE macro so the suite stays dependency-free for future tests. Closes #2 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-25 13:23:08 +02:00 · 2026-04-25 13:23:08 +02:00 · abf6153439
commit abf6153439
parent 46971acf99
6 changed files with 255 additions and 11 deletions
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -44,3 +44,12 @@ install(FILES
    "${CMAKE_CURRENT_BINARY_DIR}/oatpp-authkit-config.cmake"
    "${CMAKE_CURRENT_BINARY_DIR}/oatpp-authkit-config-version.cmake"
    DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/oatpp-authkit)
+
+# ─── Tests ───────────────────────────────────────────────────────────────────
+# Off by default so consumers pulling us in via FetchContent don't pay the
+# cost. Enable with -DOATPP_AUTHKIT_BUILD_TESTS=ON.
+option(OATPP_AUTHKIT_BUILD_TESTS "Build oatpp-authkit unit tests" OFF)
+if(OATPP_AUTHKIT_BUILD_TESTS)
+    enable_testing()
+    add_subdirectory(test)
+endif()
--- a/README.md
+++ b/README.md
@ -34,6 +34,43 @@ find_package(oatpp-authkit 0.1 REQUIRED)
 target_link_libraries(app PRIVATE oatpp::authkit)
 ```

+## Browser-friendly 401/403
+
+By default `AuthInterceptor` returns `application/json` for every rejection,
+which is correct for `/api/*` callers but breaks browser navigation: a user
+following a stale link or an expired password-reset URL sees a raw
+`{"status":"Unauthorized"}` instead of a real page.
+
+Override `IAuthPolicy::unauthenticatedRedirect(path)` to redirect browser
+navigations to a login or landing page while keeping JSON responses for
+`fetch`/`axios` callers (detected via path prefix `/api/`,
+`X-Requested-With: XMLHttpRequest`, or an `Accept` header that prefers
+`application/json`):
+
+```cpp
+class AppAuthPolicy : public oatpp_authkit::IAuthPolicy {
+public:
+    std::optional<std::string>
+    unauthenticatedRedirect(const std::string& path) override {
+        return "/?next=" + oatpp_authkit::AuthInterceptor::urlEncode(path);
+    }
+};
+```
+
+Returning `std::nullopt` (the default) preserves the legacy JSON behaviour
+for all responses.
+
+## Tests
+
+```bash
+cmake -B build -DOATPP_AUTHKIT_BUILD_TESTS=ON
+cmake --build build
+ctest --test-dir build --output-on-failure
+```
+
+Tests are off by default so consumers pulling the library in via
+`FetchContent` don't pay the cost.
+
 ## Roadmap

 - **v0.2** — `AuthInterceptor` + `requireAdmin` ported onto three seams
--- a/include/oatpp-authkit/auth/AuthInterceptor.hpp
+++ b/include/oatpp-authkit/auth/AuthInterceptor.hpp
@ -52,14 +52,102 @@ private:
        r->putHeader("Content-Type", "application/json");
        return r;
    }
-    std::shared_ptr<OutgoingResponse> makeUnauthorized() {
-        return makeJsonError(Status::CODE_401, "{\"status\":\"Unauthorized\"}");
+
+    std::shared_ptr<OutgoingResponse> makeHtmlError(Status status, const std::string& title) {
+        std::string body = "<!doctype html><meta charset=\"utf-8\"><title>"
+                         + title + "</title><h1>" + title + "</h1>";
+        auto r = ResponseFactory::createResponse(status, body.c_str());
+        r->putHeader("Content-Type", "text/html; charset=utf-8");
+        return r;
    }
-    std::shared_ptr<OutgoingResponse> makeForbidden(const std::string& msg = "") {
+
+    std::shared_ptr<OutgoingResponse> makeRedirect(const std::string& location) {
+        auto r = ResponseFactory::createResponse(Status::CODE_302, "");
+        r->putHeader("Location", location.c_str());
+        r->putHeader("Cache-Control", "no-store");
+        return r;
+    }
+
+public:
+    /**
+     * @brief Heuristic: does this caller expect a JSON error body?
+     *
+     * True when any of:
+     *   - path begins with `/api/` (API surface — always JSON)
+     *   - `X-Requested-With: XMLHttpRequest` (jQuery/axios/explicit AJAX)
+     *   - `Accept` mentions `application/json` and does NOT prefer `text/html`
+     *
+     * Otherwise treated as a browser navigation that should get HTML or a
+     * redirect. Exposed as a static so the negotiation rule is unit-testable
+     * without spinning up a request.
+     */
+    static bool wantsJson(const std::string& path,
+                          const std::string& xRequestedWith,
+                          const std::string& accept)
+    {
+        if (path.size() >= 5 && path.compare(0, 5, "/api/") == 0) return true;
+        if (!xRequestedWith.empty()) return true;
+        bool hasJson = accept.find("application/json") != std::string::npos;
+        bool hasHtml = accept.find("text/html") != std::string::npos;
+        if (hasJson && !hasHtml) return true;
+        return false;
+    }
+
+    /**
+     * @brief Percent-encode the unreserved subset for use in a `next=` param.
+     *        Static + side-effect-free so consumers and tests can reuse it.
+     */
+    static std::string urlEncode(const std::string& s) {
+        static const char* hex = "0123456789ABCDEF";
+        std::string out;
+        out.reserve(s.size());
+        for (unsigned char c : s) {
+            if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') ||
+                (c >= '0' && c <= '9') || c == '-' || c == '_' || c == '.' || c == '~') {
+                out.push_back(static_cast<char>(c));
+            } else {
+                out.push_back('%');
+                out.push_back(hex[c >> 4]);
+                out.push_back(hex[c & 0xF]);
+            }
+        }
+        return out;
+    }
+
+private:
+    bool requestWantsJson(const std::shared_ptr<IncomingRequest>& req,
+                          const std::string& path)
+    {
+        auto xrw    = req->getHeader("X-Requested-With");
+        auto accept = req->getHeader("Accept");
+        return wantsJson(path,
+                         xrw    ? *xrw    : std::string{},
+                         accept ? *accept : std::string{});
+    }
+
+    std::shared_ptr<OutgoingResponse> makeUnauthorized(
+        const std::shared_ptr<IncomingRequest>& req, const std::string& path)
+    {
+        if (requestWantsJson(req, path))
+            return makeJsonError(Status::CODE_401, "{\"status\":\"Unauthorized\"}");
+        if (auto loc = m_policy->unauthenticatedRedirect(path))
+            return makeRedirect(*loc);
+        return makeHtmlError(Status::CODE_401, "Unauthorized");
+    }
+
+    std::shared_ptr<OutgoingResponse> makeForbidden(
+        const std::shared_ptr<IncomingRequest>& req, const std::string& path,
+        const std::string& msg = "")
+    {
+        if (requestWantsJson(req, path)) {
            if (msg.empty()) return makeJsonError(Status::CODE_403, "{\"status\":\"Forbidden\"}");
            return makeJsonError(Status::CODE_403,
                "{\"status\":\"Forbidden\",\"message\":\"" + msg + "\"}");
        }
+        if (auto loc = m_policy->unauthenticatedRedirect(path))
+            return makeRedirect(*loc);
+        return makeHtmlError(Status::CODE_403, "Forbidden");
+    }

    void writeBundle(const std::shared_ptr<IncomingRequest>& req, const AuthPrincipal& p) {
        req->putBundleData("auth_user_id",   oatpp::String(std::to_string(p.id).c_str()));
@ -127,7 +215,7 @@ public:
                writeBundle(request, *p);
                if (isReadonly(p->role) && isMutation(method)) {
                    logEvent(403, method, path, "readonly cert user mutation");
-                    return makeForbidden();
+                    return makeForbidden(request, path);
                }
                return nullptr;
            }
@ -137,7 +225,7 @@ public:
        std::string token = extractToken(request);
        if (token.empty()) {
            logEvent(401, method, path, "no token");
-            return makeUnauthorized();
+            return makeUnauthorized(request, path);
        }
        std::string hash = m_hashToken(token);

@ -149,7 +237,7 @@ public:
            viaSession = false;
        } else {
            logEvent(401, method, path, "invalid token");
-            return makeUnauthorized();
+            return makeUnauthorized(request, path);
        }

        // CSRF defence-in-depth: session cookie + mutation requires X-Requested-With.
@ -157,7 +245,7 @@ public:
            auto xrw = request->getHeader("X-Requested-With");
            if (!xrw || xrw->empty()) {
                logEvent(403, method, path, "missing X-Requested-With");
-                return makeForbidden("Missing X-Requested-With header");
+                return makeForbidden(request, path, "Missing X-Requested-With header");
            }
        }

@ -165,7 +253,7 @@ public:

        if (isReadonly(p->role) && isMutation(method)) {
            logEvent(403, method, path, "readonly user mutation");
-            return makeForbidden();
+            return makeForbidden(request, path);
        }
        return nullptr;
    }
--- a/include/oatpp-authkit/auth/IAuthPolicy.hpp
+++ b/include/oatpp-authkit/auth/IAuthPolicy.hpp
@ -1,6 +1,7 @@
 #ifndef OATPP_AUTHKIT_AUTH_IAUTH_POLICY_HPP
 #define OATPP_AUTHKIT_AUTH_IAUTH_POLICY_HPP

+#include <optional>
 #include <set>
 #include <string>

@ -40,6 +41,27 @@ public:
     *        the presence of a `SETUP_MODE` sentinel file.
     */
    virtual bool setupModeActive() { return false; }
+
+    /**
+     * @brief Where to send a browser navigation that hits a 401/403, instead
+     *        of the default JSON error body.
+     *
+     * Returning `std::nullopt` (the default) keeps the legacy behaviour:
+     * `AuthInterceptor` always responds with `application/json`. Returning a
+     * URL makes the interceptor emit a `302` with `Location:` set whenever
+     * the request looks like a browser navigation (HTML `Accept`, no
+     * `X-Requested-With`, path outside `/api/`). API callers (`fetch`,
+     * `axios`, anything sending `X-Requested-With: XMLHttpRequest` or
+     * targeting `/api/*`) still receive the JSON 401/403 so client-side
+     * error handling keeps working.
+     *
+     * The same hook covers both 401 (no/invalid auth) and 403 (logged in
+     * but not allowed) — typical wiring is to bounce both to `/` or
+     * `/login?next=...`. Consumers that need to differentiate can branch
+     * inside the override.
+     */
+    virtual std::optional<std::string>
+    unauthenticatedRedirect(const std::string& /*path*/) { return std::nullopt; }
 };

 } // namespace oatpp_authkit
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@ -0,0 +1,11 @@
+# Minimal test harness for oatpp-authkit.
+#
+# Adds plain executable tests linked against the INTERFACE library and oatpp.
+# No third-party test framework — assertions use <cassert> and a tiny REQUIRE
+# macro so the suite stays portable and dependency-free.
+
+find_package(oatpp REQUIRED)
+
+add_executable(test_negotiation test_negotiation.cpp)
+target_link_libraries(test_negotiation PRIVATE oatpp::authkit oatpp::oatpp)
+add_test(NAME negotiation COMMAND test_negotiation)
--- a/test/test_negotiation.cpp
+++ b/test/test_negotiation.cpp
@ -0,0 +1,77 @@
+// Tests for AuthInterceptor::wantsJson + urlEncode (the negotiation primitives
+// that decide whether a 401/403 returns JSON vs HTML/redirect).
+//
+// Kept dependency-free on purpose — the harness exists so future tests have
+// somewhere to land, not to pull in doctest/Catch2.
+
+#include "oatpp-authkit/auth/AuthInterceptor.hpp"
+
+#include <cstdio>
+#include <cstdlib>
+#include <string>
+
+namespace {
+
+int g_failures = 0;
+
+#define REQUIRE(expr) do {                                                     \
+    if (!(expr)) {                                                             \
+        std::fprintf(stderr, "FAIL %s:%d  %s\n", __FILE__, __LINE__, #expr);   \
+        ++g_failures;                                                          \
+    }                                                                          \
+} while (0)
+
+using oatpp_authkit::AuthInterceptor;
+
+void test_wantsJson_api_path() {
+    // /api/* always wants JSON, no matter what Accept says.
+    REQUIRE( AuthInterceptor::wantsJson("/api/users", "", "text/html"));
+    REQUIRE( AuthInterceptor::wantsJson("/api/", "", ""));
+}
+
+void test_wantsJson_xrequested_with() {
+    // Explicit AJAX wins regardless of path/Accept.
+    REQUIRE( AuthInterceptor::wantsJson("/admin", "XMLHttpRequest", "text/html"));
+}
+
+void test_wantsJson_accept_header() {
+    // application/json without text/html → JSON.
+    REQUIRE( AuthInterceptor::wantsJson("/admin", "", "application/json"));
+    // text/html present → browser navigation.
+    REQUIRE(!AuthInterceptor::wantsJson("/admin", "", "text/html,application/xhtml+xml"));
+    REQUIRE(!AuthInterceptor::wantsJson("/admin", "", "text/html,application/json"));
+    // No Accept → assume browser (HTML/redirect).
+    REQUIRE(!AuthInterceptor::wantsJson("/set-password", "", ""));
+}
+
+void test_wantsJson_set_password_browser() {
+    // The motivating regression: a browser following the password-reset link
+    // must NOT be served JSON. (Path is public so it shouldn't reach this in
+    // normal flow, but if auth ever rejects it the user sees HTML/redirect.)
+    REQUIRE(!AuthInterceptor::wantsJson("/set-password",
+                                        "",
+                                        "text/html,application/xhtml+xml,application/xml;q=0.9"));
+}
+
+void test_urlEncode() {
+    REQUIRE(AuthInterceptor::urlEncode("/admin")           == "%2Fadmin");
+    REQUIRE(AuthInterceptor::urlEncode("/set-password?t=1")== "%2Fset-password%3Ft%3D1");
+    REQUIRE(AuthInterceptor::urlEncode("abc-_.~123")        == "abc-_.~123");
+    REQUIRE(AuthInterceptor::urlEncode(" ")                 == "%20");
+}
+
+} // namespace
+
+int main() {
+    test_wantsJson_api_path();
+    test_wantsJson_xrequested_with();
+    test_wantsJson_accept_header();
+    test_wantsJson_set_password_browser();
+    test_urlEncode();
+    if (g_failures) {
+        std::fprintf(stderr, "%d test(s) failed\n", g_failures);
+        return 1;
+    }
+    std::puts("ok");
+    return 0;
+}