From 421d572573a2319454ad214acfdb7e05cb8b45ff Mon Sep 17 00:00:00 2001 From: Alessandro Sacilotto Date: Wed, 26 Aug 2026 20:48:17 +0200 Subject: [PATCH] Replace core interfaces with templates, views and ASCII folding Applies the three findings from the dispatch micro-benchmark on branch perf/dispatch-microbenchmark (Release, 2M iterations x 7 repetitions, generic path, min ns per call including per-request adapter construction): virtual interfaces + std::wstring/std::string copies 139.00 ns same interfaces, zero-copy + AsciiToLower 22.49 ns templates + zero-copy + AsciiToLower 9.98 ns pre-refactor direct implementation (PR #8) 10.86 ns Copies cost ~116 ns and virtual dispatch ~4-13 ns, so the allocations were the story and dispatch the tiebreaker. The result is faster than the original direct implementation, because inline ASCII folding beats the _wcsnicmp CRT call. Templates: the IRequest/IResponse/IContext/IEventProvider interfaces become C++20 concepts (files renamed accordingly - the "I" prefix means abstract base in this codebase and would mislead on a concept), and ModuleHandler::OnBeginRequest becomes a template constrained by them. IModule is gone: with compile-time binding there is nothing left to abstract. Adapters and mocks now satisfy the contracts by shape alone, each confirmed by a static_assert, which is where a broken contract surfaces now that there is no base class to inherit from. string_view: RawHttpRequest::absPath is a non-owning wstring_view over the buffer IIS owns, and IISResponse forwards .data() straight to IIS instead of materializing std::strings. Both hot-path allocations are gone. core::Response documents the null-termination precondition this relies on; every call site passes views over string literals. AsciiToLower: replaces std::towlower, which consults the active C locale and can therefore fold the same URL differently depending on process configuration - flagged as a correctness problem in the security review, and a per-character CRT call besides. Only OnBeginRequest itself lives in a header: a template must be visible where it is instantiated, and it is instantiated with the IIS adapter types, which core must not include. Everything else - the EndsWithForbidden matching algorithm and all four adapters - stays in its .cpp, keeping the one .h/.cpp pair per class convention. Tests go from 2 to 9, covering case-insensitive matching, the suffix-only semantics, short URLs, and AsciiToLower itself (including that it leaves U+0130 alone, the classic locale-fold divergence). Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 9 ++- src/core/AsciiToLower.h | 22 +++++++ src/core/Context.h | 35 ++++++++++ src/core/EventProvider.h | 17 +++++ src/core/IContext.h | 19 ------ src/core/IEventProvider.h | 17 ----- src/core/IModule.h | 20 ------ src/core/IRequest.h | 23 ------- src/core/IResponse.h | 25 ------- src/core/ModuleHandler.cpp | 72 +++++--------------- src/core/ModuleHandler.h | 85 ++++++++++++++++++++++-- src/core/Request.h | 31 +++++++++ src/core/Response.h | 26 ++++++++ src/module/IISContext.cpp | 12 +++- src/module/IISContext.h | 27 ++++---- src/module/IISEventProvider.h | 22 ++++--- src/module/IISRequest.cpp | 5 +- src/module/IISRequest.h | 18 +++-- src/module/IISRequestLevelModule.cpp | 19 +++--- src/module/IISRequestLevelModule.h | 24 ++++--- src/module/IISResponse.cpp | 16 ++--- src/module/IISResponse.h | 19 ++++-- test/core/AsciiToLowerTest.cpp | 46 +++++++++++++ test/core/ModuleHandlerTest.cpp | 98 +++++++++++++++++++++++----- 24 files changed, 457 insertions(+), 250 deletions(-) create mode 100644 src/core/AsciiToLower.h create mode 100644 src/core/Context.h create mode 100644 src/core/EventProvider.h delete mode 100644 src/core/IContext.h delete mode 100644 src/core/IEventProvider.h delete mode 100644 src/core/IModule.h delete mode 100644 src/core/IRequest.h delete mode 100644 src/core/IResponse.h create mode 100644 src/core/Request.h create mode 100644 src/core/Response.h create mode 100644 test/core/AsciiToLowerTest.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 7d1a7cc..eed210a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,9 +62,11 @@ Report lands at `_build\windows-x86-64-coverage\coverage-report\index.html`. ## Architecture - **`src/`** — holds no sources of its own; `src/CMakeLists.txt` only aggregates the two real targets below. There is no application executable: the deliverable is the DLL that IIS loads. - - `src/core/` is the IIS/Windows-independent layer: static library target `IISNativeModuleCore` (namespace `core`), exporting its headers as `` via a PUBLIC include dir. It holds `Verdict` (the platform-free mirror of `REQUEST_NOTIFICATION_STATUS`), the interfaces `IRequest`/`IResponse`/`IContext`/`IEventProvider`/`IModule` mirroring their IIS counterparts, and `ModuleHandler` — the actual request logic (403 + reason `Forbidden by IISRequestLevelModule` for cooked URL paths ending in `/forbidden`, `X-IISRequestLevelModule: 1` header otherwise). Nothing in this directory may include Windows or IIS headers; HRESULT-shaped values travel as `std::int32_t`. - - `src/module/` is the IIS native module itself: target `IISNativeModule`, a `SHARED` library (DLL) that exports `RegisterModule` via `iis_native_module.def` and links `IISNativeModuleCore`. Files here — as in `src/core` and `test/core` — are named after the class they hold (`IISRequestLevelModule.h/.cpp` etc., PascalCase). `iis::IISRequestLevelModule` is a thin adapter: its constructor takes a non-owning `core::IModule*`, and `OnBeginRequest` wraps the live IIS objects in the `IISContext`/`IISRequest`/`IISResponse`/`IISEventProvider` adapters (one file pair per class), delegates to the injected module, and maps the returned `Verdict` back to a `REQUEST_NOTIFICATION_STATUS`. `iis::IISRequestLevelModuleFactory` owns the shared (stateless) `core::ModuleHandler` and injects it into every instance it creates; `register_module.cpp` subscribes to `RQ_BEGIN_REQUEST` only — add further `RQ_*` flags there as methods gain implementations. -- **`test/`** — GTest-based tests, target/project `IISNativeModuleTest`. Globs `test/*.cpp` and `test/core/*.cpp`, and links `IISNativeModuleCore`. `test/main.cpp` is a standard GTest `main()` running `RUN_ALL_TESTS()`. Mirror the `src/` layout under `test/` for new tests (one GTest `TEST` per behavior, wrapped in the same namespace as the code under test). `test/core/ModuleHandlerTest.cpp` exercises `core::ModuleHandler` through hand-rolled mocks of the core interfaces — the pattern to follow for testing module logic without IIS. + - `src/core/` is the IIS/Windows-independent layer: static library target `IISNativeModuleCore` (namespace `core`), exporting its headers as `` via a PUBLIC include dir. It holds `Verdict` (the platform-free mirror of `REQUEST_NOTIFICATION_STATUS`), the C++20 **concepts** `Request`/`Response`/`Context`/`EventProvider` mirroring their IIS counterparts, `AsciiToLower` (locale-independent folding — never use `std::towlower` for URL matching), and `ModuleHandler` — the actual request logic (403 + reason `Forbidden by IISRequestLevelModule` for cooked URL paths ending in `/forbidden`, `X-IISRequestLevelModule: 1` header otherwise). Nothing in this directory may include Windows or IIS headers; HRESULT-shaped values travel as `std::int32_t`. + - **The layer is compile-time polymorphic, and deliberately so** (measured: see below). `ModuleHandler::OnBeginRequest` is a template constrained by the concepts, so adapters and mocks bind with no vtable and no allocation. It is the one thing that **must** live in `ModuleHandler.h`: a template has to be visible where it is instantiated, and it is instantiated with the IIS adapter types in `src/module`, which core must not know about. Everything not type-dependent — the `EndsWithForbidden` matching algorithm — lives in `ModuleHandler.cpp`, per the project's one-`.h`/`.cpp`-pair-per-class convention. + - `core::RawHttpRequest::absPath` is a **non-owning `std::wstring_view`** over the server's buffer, and `Response`'s `string_view` parameters carry a **null-termination precondition** (the IIS adapter forwards `.data()` straight to IIS rather than materializing a `std::string`); every call site passes views over string literals. Do not reintroduce copies here — that is where the cost was. + - `src/module/` is the IIS native module itself: target `IISNativeModule`, a `SHARED` library (DLL) that exports `RegisterModule` via `iis_native_module.def` and links `IISNativeModuleCore`. Files here — as in `src/core` and `test/core` — are named after the class they hold (`IISRequestLevelModule.h/.cpp` etc., PascalCase). The four adapters `IISContext`/`IISRequest`/`IISResponse`/`IISEventProvider` are each a `.h`/`.cpp` pair and each carries a `static_assert` proving it satisfies its core concept — with no base class to inherit from, that assert is the only place a broken contract surfaces, so keep it. `iis::IISRequestLevelModule` is a thin adapter: its constructor takes a non-owning `const core::ModuleHandler*`, and `OnBeginRequest` wraps the live IIS objects in the adapters, delegates, and maps the returned `Verdict` back to a `REQUEST_NOTIFICATION_STATUS`. `iis::IISRequestLevelModuleFactory` owns the shared (stateless, `const`) `core::ModuleHandler` and injects it into every instance it creates; `register_module.cpp` subscribes to `RQ_BEGIN_REQUEST` only — add further `RQ_*` flags there as methods gain implementations. +- **`test/`** — GTest-based tests, target/project `IISNativeModuleTest`. Globs `test/*.cpp` and `test/core/*.cpp`, and links `IISNativeModuleCore`. `test/main.cpp` is a standard GTest `main()` running `RUN_ALL_TESTS()`. Mirror the `src/` layout under `test/` for new tests (one GTest `TEST` per behavior, wrapped in the same namespace as the code under test). `test/core/ModuleHandlerTest.cpp` exercises `core::ModuleHandler` through hand-rolled mocks — the pattern to follow for testing module logic without IIS. The mocks satisfy the core concepts **by shape alone**: no base class, no `override`, no vtable, each confirmed by a `static_assert`. - Tests include core headers as ``, which resolves because `IISNativeModuleCore` exports `src/` as a PUBLIC include directory — not through any include path set on the test target itself. - Because every target uses `file(GLOB ...)`, CMake must be **re-run** (re-configure, not just rebuild) after adding or removing source files for them to be picked up — GLOB results are cached at configure time. @@ -78,5 +80,6 @@ Report lands at `_build\windows-x86-64-coverage\coverage-report\index.html`. - Minimum CMake version required is 4.2 (unusually new — verify your local CMake before assuming a configure failure is a code issue). CI installs `cmake >= 4.2` explicitly since GitHub-hosted runners don't have it preinstalled. - The toolchain pins Visual Studio 2026 (`Visual Studio 18 2026`); local build presets additionally pin an exact MSVC toolset version (`14.44.35207` for build presets, `host=x64,version=14.50` for the static-analysis configure preset) — build failures on other VS/MSVC versions are expected without adjusting `CMakePresets.json`. CI sidesteps this (see above). - `vcpkg.json` still declares `fmt` and `spdlog` (and the top-level `CMakeLists.txt` still `find_package`s them), but no target links them any more — they are kept available for future logging/formatting work. +- **Why core is templates rather than virtual interfaces**: measured on branch `perf/dispatch-microbenchmark` (`bench/`, Release, 2M iterations x 7 reps, generic path, min ns per call including per-request adapter construction). Virtual interfaces copying into `std::wstring`/`std::string`: **139.00 ns**. Same interfaces, zero-copy + `AsciiToLower`: **22.49 ns**. Templates + zero-copy + `AsciiToLower`: **9.98 ns** — faster than the pre-refactor direct implementation (10.86 ns), because inline ASCII folding beats the `_wcsnicmp` CRT call. Copies cost ~116 ns, virtual dispatch ~4–13 ns: **the allocations were the story, dispatch was the tiebreaker.** Don't reintroduce either. Note the 9.98 ns figure came from a fully-inlined variant; here the adapters and `EndsWithForbidden` are in `.cpp` files, so cross-TU calls remain unless `/GL`+`/LTCG` is enabled — the copies and the vtable, which dominated, are gone either way. - All targets compile with `/guard:cf` (Control Flow Guard, also passed at link) and, under MSVC, `/sdl` — hardening for a DLL that runs network-facing inside w3wp.exe. `IISRequestLevelModule::OnBeginRequest` wraps its body in `try/catch(...)`: IIS notification methods must never let a C++ exception escape (it would crash the worker process), so keep that boundary when adding new notifications. - CI actions in `.github/workflows/ci.yml` are pinned by commit SHA (supply-chain); Dependabot (`.github/dependabot.yml`) proposes pin bumps. When adding an action, pin it by SHA with a `# vX` comment. diff --git a/src/core/AsciiToLower.h b/src/core/AsciiToLower.h new file mode 100644 index 0000000..85508de --- /dev/null +++ b/src/core/AsciiToLower.h @@ -0,0 +1,22 @@ +#ifndef CORE_ASCII_TO_LOWER_H_ +#define CORE_ASCII_TO_LOWER_H_ + +namespace core { + // Locale-independent ASCII case folding. + // + // Deliberately NOT std::towlower: that consults the active C locale, + // so the same URL can fold differently depending on process + // configuration (the Turkish dotted/dotless I is the classic way this + // breaks a security comparison). URL path matching must behave + // identically everywhere, so only A-Z is folded and every other code + // point — including non-ASCII — is left untouched. + // + // Being constexpr and branch-simple, it also inlines into the + // comparison loop instead of calling into the CRT once per character. + constexpr auto AsciiToLower(wchar_t character) -> wchar_t { + return (character >= L'A' && character <= L'Z') + ? static_cast(character - L'A' + L'a') + : character; + } +} +#endif // CORE_ASCII_TO_LOWER_H_ diff --git a/src/core/Context.h b/src/core/Context.h new file mode 100644 index 0000000..a601123 --- /dev/null +++ b/src/core/Context.h @@ -0,0 +1,35 @@ +#ifndef CORE_CONTEXT_H_ +#define CORE_CONTEXT_H_ + +#include +#include + +#include "Request.h" +#include "Response.h" + +namespace core { + + namespace detail { + // GetRequest()/GetResponse() hand back pointers; this strips the + // pointer and cv-qualifiers so the concepts below can be applied + // to what they point at. + template + using Pointee = std::remove_cvref_t>; + } + + // Compile-time counterpart of IHttpContext: exposes a request and a + // response that themselves satisfy the Request/Response concepts. + // + // Both getters return nullable pointers, mirroring an IIS context + // whose request or response is unavailable. + template + concept Context = + requires(T & context) { + context.GetRequest(); + context.GetResponse(); + } && + Request().GetRequest())>> && + Response().GetResponse())>>; + +} +#endif // CORE_CONTEXT_H_ diff --git a/src/core/EventProvider.h b/src/core/EventProvider.h new file mode 100644 index 0000000..b327840 --- /dev/null +++ b/src/core/EventProvider.h @@ -0,0 +1,17 @@ +#ifndef CORE_EVENT_PROVIDER_H_ +#define CORE_EVENT_PROVIDER_H_ + +#include +#include + +namespace core { + // Compile-time counterpart of IHttpEventProvider. + // + // errorStatus carries the HRESULT-shaped error code as a plain 32-bit + // integer so core stays independent from . + template + concept EventProvider = requires(T & provider) { + { provider.SetErrorStatus(std::int32_t{}) } -> std::same_as; + }; +} +#endif // CORE_EVENT_PROVIDER_H_ diff --git a/src/core/IContext.h b/src/core/IContext.h deleted file mode 100644 index fd5f397..0000000 --- a/src/core/IContext.h +++ /dev/null @@ -1,19 +0,0 @@ -#ifndef CORE_I_CONTEXT_H_ -#define CORE_I_CONTEXT_H_ - -#include "IRequest.h" -#include "IResponse.h" - -namespace core { - // IIS-independent mapping of IHttpContext. - class IContext { - public: - virtual ~IContext() = default; - - // Either may return nullptr, mirroring an IIS context whose - // request or response is unavailable. - virtual auto GetRequest() -> IRequest * = 0; - virtual auto GetResponse() -> IResponse * = 0; - }; -} -#endif // CORE_I_CONTEXT_H_ diff --git a/src/core/IEventProvider.h b/src/core/IEventProvider.h deleted file mode 100644 index 86064d9..0000000 --- a/src/core/IEventProvider.h +++ /dev/null @@ -1,17 +0,0 @@ -#ifndef CORE_I_EVENT_PROVIDER_H_ -#define CORE_I_EVENT_PROVIDER_H_ - -#include - -namespace core { - // IIS-independent mapping of IHttpEventProvider. - class IEventProvider { - public: - virtual ~IEventProvider() = default; - - // errorStatus carries the HRESULT-shaped error code as a plain - // 32-bit integer so core stays independent from . - virtual auto SetErrorStatus(std::int32_t errorStatus) -> void = 0; - }; -} -#endif // CORE_I_EVENT_PROVIDER_H_ diff --git a/src/core/IModule.h b/src/core/IModule.h deleted file mode 100644 index 2442966..0000000 --- a/src/core/IModule.h +++ /dev/null @@ -1,20 +0,0 @@ -#ifndef CORE_I_MODULE_H_ -#define CORE_I_MODULE_H_ - -#include "IContext.h" -#include "IEventProvider.h" -#include "Verdict.h" - -namespace core { - // IIS-independent mapping of CHttpModule, reduced to the - // notifications the module actually handles. - class IModule { - public: - virtual ~IModule() = default; - - virtual auto OnBeginRequest( - IContext * context, - IEventProvider * provider) -> Verdict = 0; - }; -} -#endif // CORE_I_MODULE_H_ diff --git a/src/core/IRequest.h b/src/core/IRequest.h deleted file mode 100644 index 2328878..0000000 --- a/src/core/IRequest.h +++ /dev/null @@ -1,23 +0,0 @@ -#ifndef CORE_I_REQUEST_H_ -#define CORE_I_REQUEST_H_ - -#include - -namespace core { - // IIS-independent mapping of the parts of HTTP_REQUEST the module - // logic reads: the cooked URL's absolute path (no query string). - struct RawHttpRequest { - std::wstring absPath; - }; - - // IIS-independent mapping of IHttpRequest. - class IRequest { - public: - virtual ~IRequest() = default; - - // May return nullptr, mirroring an IIS request without a raw - // request or cooked URL path. - virtual auto GetRawHttpRequest() const -> const RawHttpRequest * = 0; - }; -} -#endif // CORE_I_REQUEST_H_ diff --git a/src/core/IResponse.h b/src/core/IResponse.h deleted file mode 100644 index 5746ca6..0000000 --- a/src/core/IResponse.h +++ /dev/null @@ -1,25 +0,0 @@ -#ifndef CORE_I_RESPONSE_H_ -#define CORE_I_RESPONSE_H_ - -#include -#include - -namespace core { - // IIS-independent mapping of IHttpResponse. - class IResponse { - public: - virtual ~IResponse() = default; - - // errorStatus carries the HRESULT-shaped error code as a plain - // 32-bit integer so core stays independent from . - virtual auto SetStatus( - int statusCode, - std::string_view reason, - std::int32_t errorStatus) -> void = 0; - - virtual auto SetHeader( - std::string_view name, - std::string_view value) -> void = 0; - }; -} -#endif // CORE_I_RESPONSE_H_ diff --git a/src/core/ModuleHandler.cpp b/src/core/ModuleHandler.cpp index 1a14e9d..1d89308 100644 --- a/src/core/ModuleHandler.cpp +++ b/src/core/ModuleHandler.cpp @@ -1,74 +1,34 @@ #include "ModuleHandler.h" -#include -#include +#include #include -namespace core { +#include "AsciiToLower.h" + +namespace core::detail { namespace { constexpr std::wstring_view kForbiddenSuffix = L"/forbidden"; - constexpr std::string_view kForbiddenReason = "Forbidden by IISRequestLevelModule"; - constexpr std::string_view kHeaderName = "X-IISRequestLevelModule"; - constexpr std::string_view kHeaderValue = "1"; - constexpr int kForbiddenStatusCode = 403; - // HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) / E_ACCESSDENIED, spelled - // as a plain value so core stays independent from . - constexpr std::int32_t kAccessDeniedError = - static_cast(0x80070005U); - - // Case-insensitive suffix check on the cooked URL path, which - // carries the path without the query string. - // - // SECURITY NOTE: this is demo logic, not an access-control - // mechanism. A suffix-only match doesn't block a subtree - // ("/forbidden/", "/forbidden/x" pass), and std::towlower is - // locale-dependent (invariant ASCII folding is what a security - // comparison would need). Replace the matching semantics before - // building any real ACL on this. - auto EndsWithForbidden(const RawHttpRequest * rawRequest) -> bool { - if (rawRequest == nullptr) { - return false; - } - const std::wstring_view path = rawRequest->absPath; - if (path.length() < kForbiddenSuffix.length()) { - return false; - } - const std::wstring_view tail = path.substr(path.length() - kForbiddenSuffix.length()); - for (size_t i = 0; i < kForbiddenSuffix.length(); ++i) { - if (std::towlower(tail[i]) != std::towlower(kForbiddenSuffix[i])) { - return false; - } - } - return true; - } } -auto ModuleHandler::OnBeginRequest( - IContext * context, - IEventProvider * provider) -> Verdict { - if (context == nullptr) { - return Verdict::Continue; +auto EndsWithForbidden(const RawHttpRequest * rawRequest) -> bool { + if (rawRequest == nullptr) { + return false; } - - IRequest * request = context->GetRequest(); - IResponse * response = context->GetResponse(); - if (request == nullptr || response == nullptr) { - return Verdict::Continue; + const std::wstring_view path = rawRequest->absPath; + if (path.length() < kForbiddenSuffix.length()) { + return false; } - - if (EndsWithForbidden(request->GetRawHttpRequest())) { - response->SetStatus(kForbiddenStatusCode, kForbiddenReason, kAccessDeniedError); - if (provider != nullptr) { - provider->SetErrorStatus(kAccessDeniedError); + const std::wstring_view tail = + path.substr(path.length() - kForbiddenSuffix.length()); + for (std::size_t i = 0; i < kForbiddenSuffix.length(); ++i) { + if (AsciiToLower(tail[i]) != AsciiToLower(kForbiddenSuffix[i])) { + return false; } - return Verdict::Finish; } - - response->SetHeader(kHeaderName, kHeaderValue); - return Verdict::Continue; + return true; } } diff --git a/src/core/ModuleHandler.h b/src/core/ModuleHandler.h index b2a8078..e29be76 100644 --- a/src/core/ModuleHandler.h +++ b/src/core/ModuleHandler.h @@ -1,18 +1,89 @@ #ifndef CORE_MODULE_HANDLER_H_ #define CORE_MODULE_HANDLER_H_ -#include "IModule.h" +#include +#include + +#include "Context.h" +#include "EventProvider.h" +#include "Verdict.h" namespace core { + + namespace detail { + + // Constants stay in the header because the OnBeginRequest + // template below is expanded in the caller's translation unit and + // has to see them. + inline constexpr std::string_view kForbiddenReason = "Forbidden by IISRequestLevelModule"; + inline constexpr std::string_view kHeaderName = "X-IISRequestLevelModule"; + inline constexpr std::string_view kHeaderValue = "1"; + inline constexpr int kForbiddenStatusCode = 403; + // HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED) / E_ACCESSDENIED, + // spelled as a plain value so core stays independent from + // . + inline constexpr std::int32_t kAccessDeniedError = + static_cast(0x80070005U); + + // Case-insensitive suffix check on the cooked URL path, which + // carries the path without the query string. Compares over a view + // of the server-owned buffer: no copy is made. Defined in + // ModuleHandler.cpp. + // + // SECURITY NOTE: this is demo logic, not an access-control + // mechanism. A suffix-only match doesn't block a subtree + // ("/forbidden/", "/forbidden/x" pass). Replace the matching + // semantics before building any real ACL on this. The folding + // itself is locale-independent — see AsciiToLower. + auto EndsWithForbidden(const RawHttpRequest * rawRequest) -> bool; + + } + // The IIS-independent request-handling logic: rejects requests whose // URL path ends with "/forbidden" and stamps every other response - // with the X-IISRequestLevelModule header. Stateless, so a single instance - // can be shared across concurrent requests. - class ModuleHandler : public IModule { + // with the X-IISRequestLevelModule header. + // + // OnBeginRequest is a template resolved against whatever context and + // event provider it is handed, so production adapters and test mocks + // both bind with no virtual dispatch and no allocation. + // + // It is defined here rather than in ModuleHandler.cpp because a + // template must be visible where it is instantiated, and the types it + // is instantiated with (the IIS adapters) live in src/module, which + // core must not know about. Everything that is NOT type-dependent — + // the matching algorithm — is in the .cpp. + // + // Stateless and const — a single instance is safely shared across + // concurrent requests. + class ModuleHandler { public: - auto OnBeginRequest( - IContext * context, - IEventProvider * provider) -> Verdict override; + template + auto OnBeginRequest(TContext * context, TEventProvider * provider) const -> Verdict { + if (context == nullptr) { + return Verdict::Continue; + } + + auto * request = context->GetRequest(); + auto * response = context->GetResponse(); + if (request == nullptr || response == nullptr) { + return Verdict::Continue; + } + + if (detail::EndsWithForbidden(request->GetRawHttpRequest())) { + response->SetStatus( + detail::kForbiddenStatusCode, + detail::kForbiddenReason, + detail::kAccessDeniedError); + if (provider != nullptr) { + provider->SetErrorStatus(detail::kAccessDeniedError); + } + return Verdict::Finish; + } + + response->SetHeader(detail::kHeaderName, detail::kHeaderValue); + return Verdict::Continue; + } }; + } #endif // CORE_MODULE_HANDLER_H_ diff --git a/src/core/Request.h b/src/core/Request.h new file mode 100644 index 0000000..7648fca --- /dev/null +++ b/src/core/Request.h @@ -0,0 +1,31 @@ +#ifndef CORE_REQUEST_H_ +#define CORE_REQUEST_H_ + +#include +#include + +namespace core { + // IIS-independent mapping of the parts of HTTP_REQUEST the module + // logic reads: the cooked URL's absolute path (no query string). + // + // absPath is a NON-OWNING view over a buffer the server owns. It is + // valid only for the notification the request object was built for, + // which is exactly that object's lifetime — never store it beyond + // that. Holding a view rather than a std::wstring is what keeps the + // hot path allocation-free. + struct RawHttpRequest { + std::wstring_view absPath; + }; + + // Compile-time counterpart of IHttpRequest: any type exposing + // GetRawHttpRequest() satisfies it, so production adapters and test + // mocks are interchangeable without a vtable. + // + // The pointer is nullable, mirroring an IIS request that has no raw + // request or no cooked URL path. + template + concept Request = requires(const T & request) { + { request.GetRawHttpRequest() } -> std::same_as; + }; +} +#endif // CORE_REQUEST_H_ diff --git a/src/core/Response.h b/src/core/Response.h new file mode 100644 index 0000000..02cc8df --- /dev/null +++ b/src/core/Response.h @@ -0,0 +1,26 @@ +#ifndef CORE_RESPONSE_H_ +#define CORE_RESPONSE_H_ + +#include +#include +#include + +namespace core { + // Compile-time counterpart of IHttpResponse. + // + // The string_view parameters must be NULL-TERMINATED at the character + // one past their end: the IIS adapter forwards .data() straight to + // the server's PCSTR-taking API rather than materializing a + // std::string, which is what keeps the hot path allocation-free. All + // current call sites pass views over string literals, which satisfy + // this. Do not pass a view into the middle of a larger buffer. + // + // errorStatus carries the HRESULT-shaped error code as a plain 32-bit + // integer so core stays independent from . + template + concept Response = requires(T & response) { + { response.SetStatus(int{}, std::string_view{}, std::int32_t{}) } -> std::same_as; + { response.SetHeader(std::string_view{}, std::string_view{}) } -> std::same_as; + }; +} +#endif // CORE_RESPONSE_H_ diff --git a/src/module/IISContext.cpp b/src/module/IISContext.cpp index 85170e5..5be7747 100644 --- a/src/module/IISContext.cpp +++ b/src/module/IISContext.cpp @@ -2,11 +2,19 @@ namespace iis { -auto IISContext::GetRequest() -> core::IRequest * { +// Both adapters tolerate a null IIS pointer at construction; the getters +// below only hand them out when it is non-null. +IISContext::IISContext(_In_ IHttpContext * pHttpContext) + : pRequest_{pHttpContext->GetRequest()}, + request_{pRequest_}, + pResponse_{pHttpContext->GetResponse()}, + response_{pResponse_} {} + +auto IISContext::GetRequest() -> IISRequest * { return pRequest_ != nullptr ? &request_ : nullptr; } -auto IISContext::GetResponse() -> core::IResponse * { +auto IISContext::GetResponse() -> IISResponse * { return pResponse_ != nullptr ? &response_ : nullptr; } diff --git a/src/module/IISContext.h b/src/module/IISContext.h index 6789cfa..d55cef4 100644 --- a/src/module/IISContext.h +++ b/src/module/IISContext.h @@ -6,26 +6,22 @@ #include #include -#include +#include #include "IISRequest.h" #include "IISResponse.h" namespace iis { - // Adapter translating a live IHttpContext into core::IContext for - // the duration of one notification. Both member adapters tolerate a - // null IIS pointer at construction; GetRequest/GetResponse only - // hand them out when it is non-null. - class IISContext final : public core::IContext { + // Adapter satisfying core::Context over a live IHttpContext, for the + // duration of one notification. Both member adapters tolerate a null + // IIS pointer at construction; GetRequest/GetResponse only hand them + // out when it is non-null. + class IISContext final { public: - explicit IISContext(_In_ IHttpContext * pHttpContext) - : pRequest_{pHttpContext->GetRequest()}, - request_{pRequest_}, - pResponse_{pHttpContext->GetResponse()}, - response_{pResponse_} {} + explicit IISContext(_In_ IHttpContext * pHttpContext); - auto GetRequest() -> core::IRequest * override; - auto GetResponse() -> core::IResponse * override; + auto GetRequest() -> IISRequest *; + auto GetResponse() -> IISResponse *; private: IHttpRequest * pRequest_; @@ -33,5 +29,10 @@ namespace iis { IHttpResponse * pResponse_; IISResponse response_; }; + + // Fails here, with a readable message, if the class ever stops + // matching the contract the handler template expects. Transitively + // checks IISRequest and IISResponse too. + static_assert(core::Context); } #endif // IIS_CONTEXT_H_ diff --git a/src/module/IISEventProvider.h b/src/module/IISEventProvider.h index 4fef0f9..6ad22c1 100644 --- a/src/module/IISEventProvider.h +++ b/src/module/IISEventProvider.h @@ -6,23 +6,27 @@ #include #include -#include +#include namespace iis { - // Adapter translating a live IHttpEventProvider into - // core::IEventProvider for the duration of one notification. - // Tolerates a null IIS pointer at construction; the caller must - // only invoke SetErrorStatus when the wrapped pointer is non-null - // (IISRequestLevelModule enforces this by passing a null provider - // to the handler instead). - class IISEventProvider final : public core::IEventProvider { + // Adapter satisfying core::EventProvider over a live + // IHttpEventProvider, for the duration of one notification. + // Tolerates a null IIS pointer at construction; the caller must only + // invoke SetErrorStatus when the wrapped pointer is non-null + // (IISRequestLevelModule enforces this by passing a null provider to + // the handler instead). + class IISEventProvider final { public: explicit IISEventProvider(_In_opt_ IHttpEventProvider * pProvider) : pProvider_{pProvider} {} - auto SetErrorStatus(std::int32_t errorStatus) -> void override; + auto SetErrorStatus(std::int32_t errorStatus) -> void; private: IHttpEventProvider * pProvider_; }; + + // Fails here, with a readable message, if the class ever stops + // matching the contract the handler template expects. + static_assert(core::EventProvider); } #endif // IIS_EVENT_PROVIDER_H_ diff --git a/src/module/IISRequest.cpp b/src/module/IISRequest.cpp index 34252f0..12e406f 100644 --- a/src/module/IISRequest.cpp +++ b/src/module/IISRequest.cpp @@ -10,8 +10,9 @@ IISRequest::IISRequest(_In_opt_ IHttpRequest * pRequest) { if (pRawRequest == nullptr || pRawRequest->CookedUrl.pAbsPath == nullptr) { return; } - // The cooked URL length is in bytes. - rawRequest_.absPath.assign( + // The cooked URL length is in bytes. A view, not a copy: the buffer + // belongs to IIS and outlives this adapter. + rawRequest_.absPath = std::wstring_view( pRawRequest->CookedUrl.pAbsPath, pRawRequest->CookedUrl.AbsPathLength / sizeof(WCHAR)); hasRawRequest_ = true; diff --git a/src/module/IISRequest.h b/src/module/IISRequest.h index 784ebf1..925d43e 100644 --- a/src/module/IISRequest.h +++ b/src/module/IISRequest.h @@ -6,21 +6,29 @@ #include #include -#include +#include namespace iis { - // Adapter translating a live IHttpRequest into core::IRequest for - // the duration of one notification. Tolerates a null IIS pointer at + // Adapter satisfying core::Request over a live IHttpRequest, for the + // duration of one notification. Tolerates a null IIS pointer at // construction (GetRawHttpRequest then reports no raw request). - class IISRequest final : public core::IRequest { + // + // The cooked URL path is VIEWED, never copied: IIS owns that buffer + // and keeps it alive for the whole notification, which outlives this + // stack-allocated adapter. + class IISRequest final { public: explicit IISRequest(_In_opt_ IHttpRequest * pRequest); - auto GetRawHttpRequest() const -> const core::RawHttpRequest * override; + auto GetRawHttpRequest() const -> const core::RawHttpRequest *; private: core::RawHttpRequest rawRequest_; bool hasRawRequest_ = false; }; + + // Fails here, with a readable message, if the class ever stops + // matching the contract the handler template expects. + static_assert(core::Request); } #endif // IIS_REQUEST_H_ diff --git a/src/module/IISRequestLevelModule.cpp b/src/module/IISRequestLevelModule.cpp index 1a71a83..369f0a4 100644 --- a/src/module/IISRequestLevelModule.cpp +++ b/src/module/IISRequestLevelModule.cpp @@ -10,22 +10,25 @@ namespace iis { auto IISRequestLevelModule::OnBeginRequest( _In_ IHttpContext * pHttpContext, _In_ IHttpEventProvider * pProvider) -> REQUEST_NOTIFICATION_STATUS { - if (module_ == nullptr || pHttpContext == nullptr) { + if (handler_ == nullptr || pHttpContext == nullptr) { return RQ_NOTIFICATION_CONTINUE; } // IIS notification methods must not throw: an escaping C++ exception - // unwinds into the native pipeline and crashes the worker process - // (the adapters allocate, so std::bad_alloc is possible under memory - // pressure). Failing open (Continue) is a deliberate tradeoff for - // this demo rule: a dropped forbidden-check beats taking down the - // whole app pool. catch(...) under /EHsc catches C++ exceptions - // only, not SEH. + // unwinds into the native pipeline and crashes the worker process. + // The adapters no longer allocate, so there is no longer a plausible + // throw on this path, but the boundary stays — it is the contract for + // every notification, and future logic must not be able to break it. + // Failing open (Continue) is a deliberate tradeoff for this demo + // rule: a dropped forbidden-check beats taking down the whole app + // pool. catch(...) under /EHsc catches C++ exceptions only, not SEH. try { IISContext context{pHttpContext}; IISEventProvider provider{pProvider}; - const core::Verdict verdict = module_->OnBeginRequest( + // Resolved at compile time against IISContext/IISEventProvider, + // so this whole chain inlines with no virtual dispatch. + const core::Verdict verdict = handler_->OnBeginRequest( &context, pProvider != nullptr ? &provider : nullptr); diff --git a/src/module/IISRequestLevelModule.h b/src/module/IISRequestLevelModule.h index d36956f..30c5915 100644 --- a/src/module/IISRequestLevelModule.h +++ b/src/module/IISRequestLevelModule.h @@ -6,21 +6,27 @@ #include #include -#include +#include namespace iis { // Request-level HTTP module: the only notification it handles is // RQ_BEGIN_REQUEST (see register_module.cpp). The actual request - // logic lives in the injected core::IModule, keeping this class a - // thin IIS adapter; module is non-owning and must outlive this - // instance (the factory owns the handler). + // logic lives in the injected core::ModuleHandler, keeping this class + // a thin IIS adapter; handler is non-owning and must outlive this + // instance (the factory owns it). + // + // The handler is held by concrete pointer rather than through an + // abstract base: its OnBeginRequest is a template, so substituting a + // different implementation happens at compile time (which is what + // lets the whole call chain inline and stay allocation-free) instead + // of through a vtable. class IISRequestLevelModule : public CHttpModule { public: - explicit IISRequestLevelModule(core::IModule * module) : module_{module} {} + explicit IISRequestLevelModule(const core::ModuleHandler * handler) : handler_{handler} {} - // RQ_BEGIN_REQUEST: translates the IIS context/provider into - // their core counterparts, delegates to module_, and maps the - // returned Verdict back to a REQUEST_NOTIFICATION_STATUS. + // RQ_BEGIN_REQUEST: wraps the IIS context/provider in the core + // adapters, delegates to handler_, and maps the returned Verdict + // back to a REQUEST_NOTIFICATION_STATUS. auto OnBeginRequest( _In_ IHttpContext * pHttpContext, _In_ IHttpEventProvider * pProvider) -> REQUEST_NOTIFICATION_STATUS override; @@ -29,7 +35,7 @@ namespace iis { auto Dispose() -> VOID override; private: - core::IModule * module_; + const core::ModuleHandler * handler_; }; } #endif // IIS_REQUEST_LEVEL_MODULE_H_ diff --git a/src/module/IISResponse.cpp b/src/module/IISResponse.cpp index af74ba5..573c915 100644 --- a/src/module/IISResponse.cpp +++ b/src/module/IISResponse.cpp @@ -1,19 +1,19 @@ #include "IISResponse.h" -#include - namespace iis { auto IISResponse::SetStatus( int statusCode, std::string_view reason, std::int32_t errorStatus) -> void { - const std::string reasonString{reason}; + // reason.data() is forwarded directly rather than copied into a + // std::string: core::Response requires these views to be + // null-terminated for exactly this reason. // fTrySkipCustomErrors: keep our own reason phrase instead of // letting the custom-errors module replace the response. pResponse_->SetStatus( static_cast(statusCode), - reasonString.c_str(), + reason.data(), 0, static_cast(errorStatus), nullptr, @@ -21,12 +21,10 @@ auto IISResponse::SetStatus( } auto IISResponse::SetHeader(std::string_view name, std::string_view value) -> void { - const std::string nameString{name}; - const std::string valueString{value}; pResponse_->SetHeader( - nameString.c_str(), - valueString.c_str(), - static_cast(valueString.length()), + name.data(), + value.data(), + static_cast(value.length()), TRUE); } diff --git a/src/module/IISResponse.h b/src/module/IISResponse.h index a69193c..4448b7b 100644 --- a/src/module/IISResponse.h +++ b/src/module/IISResponse.h @@ -6,27 +6,36 @@ #include #include -#include +#include namespace iis { - // Adapter translating a live IHttpResponse into core::IResponse for + // Adapter satisfying core::Response over a live IHttpResponse, for // the duration of one notification. Tolerates a null IIS pointer at // construction; the caller must only invoke the methods when the // wrapped pointer is non-null (IISContext enforces this by not // handing the adapter out otherwise). - class IISResponse final : public core::IResponse { + // + // The incoming views are forwarded to IIS with .data() rather than + // being copied into a std::string, which is what keeps this + // allocation-free. That relies on core::Response's null-termination + // precondition: every caller passes a view over a string literal. + class IISResponse final { public: explicit IISResponse(_In_opt_ IHttpResponse * pResponse) : pResponse_{pResponse} {} auto SetStatus( int statusCode, std::string_view reason, - std::int32_t errorStatus) -> void override; + std::int32_t errorStatus) -> void; - auto SetHeader(std::string_view name, std::string_view value) -> void override; + auto SetHeader(std::string_view name, std::string_view value) -> void; private: IHttpResponse * pResponse_; }; + + // Fails here, with a readable message, if the class ever stops + // matching the contract the handler template expects. + static_assert(core::Response); } #endif // IIS_RESPONSE_H_ diff --git a/test/core/AsciiToLowerTest.cpp b/test/core/AsciiToLowerTest.cpp new file mode 100644 index 0000000..437c14b --- /dev/null +++ b/test/core/AsciiToLowerTest.cpp @@ -0,0 +1,46 @@ +#include + +#include + +namespace core { + +TEST(TestCore, TestAsciiToLowerFoldsAsciiUppercase) +{ + EXPECT_EQ(L'a', AsciiToLower(L'A')); + EXPECT_EQ(L'z', AsciiToLower(L'Z')); + EXPECT_EQ(L'm', AsciiToLower(L'M')); +} + +TEST(TestCore, TestAsciiToLowerLeavesEverythingElseAlone) +{ + EXPECT_EQ(L'a', AsciiToLower(L'a')); + EXPECT_EQ(L'/', AsciiToLower(L'/')); + EXPECT_EQ(L'7', AsciiToLower(L'7')); + // Just outside the A-Z range on either side. + EXPECT_EQ(L'@', AsciiToLower(L'@')); + EXPECT_EQ(L'[', AsciiToLower(L'[')); +} + +// The reason this exists instead of std::towlower: folding must not +// depend on the active locale. U+0130 (LATIN CAPITAL LETTER I WITH DOT +// ABOVE) is the classic case where a locale-aware fold diverges; here it +// is left untouched because it is not ASCII. Written as numeric code +// points so the test does not depend on the source file's encoding or +// the compiler's active code page. +TEST(TestCore, TestAsciiToLowerIgnoresNonAsciiRegardlessOfLocale) +{ + constexpr wchar_t kCapitalIWithDotAbove = static_cast(0x0130); + constexpr wchar_t kCapitalAWithDiaeresis = static_cast(0x00C4); + + EXPECT_EQ(kCapitalIWithDotAbove, AsciiToLower(kCapitalIWithDotAbove)); + EXPECT_EQ(kCapitalAWithDiaeresis, AsciiToLower(kCapitalAWithDiaeresis)); +} + +TEST(TestCore, TestAsciiToLowerIsUsableAtCompileTime) +{ + static_assert(AsciiToLower(L'A') == L'a'); + static_assert(AsciiToLower(L'/') == L'/'); + SUCCEED(); +} + +} diff --git a/test/core/ModuleHandlerTest.cpp b/test/core/ModuleHandlerTest.cpp index 7069e92..e82953e 100644 --- a/test/core/ModuleHandlerTest.cpp +++ b/test/core/ModuleHandlerTest.cpp @@ -11,32 +11,42 @@ namespace core { namespace { - class MockRequest : public IRequest { + // The mocks satisfy the core concepts by shape alone — no base class + // and no vtable — which is exactly how the production adapters bind. + class MockRequest { public: - explicit MockRequest(std::wstring url) : rawRequest_{std::move(url)} {} + explicit MockRequest(std::wstring url) : url_{std::move(url)} { + rawRequest_.absPath = url_; + } + + // rawRequest_ views url_, so copying would leave the view pointing + // at the moved-from string. + MockRequest(const MockRequest &) = delete; + auto operator=(const MockRequest &) -> MockRequest & = delete; - auto GetRawHttpRequest() const -> const RawHttpRequest * override { + auto GetRawHttpRequest() const -> const RawHttpRequest * { return &rawRequest_; } private: + std::wstring url_; RawHttpRequest rawRequest_; }; - // Records every call so the tests can assert on what the handler - // did to the response. - class MockResponse : public IResponse { + // Records every call so the tests can assert on what the handler did + // to the response. + class MockResponse { public: auto SetStatus( int statusCode, std::string_view reason, - std::int32_t errorStatus) -> void override { + std::int32_t errorStatus) -> void { statusCode_ = statusCode; reason_ = std::string{reason}; errorStatus_ = errorStatus; } - auto SetHeader(std::string_view name, std::string_view value) -> void override { + auto SetHeader(std::string_view name, std::string_view value) -> void { headers_.emplace_back(std::string{name}, std::string{value}); } @@ -46,28 +56,33 @@ namespace { std::vector> headers_; }; - class MockContext : public IContext { + class MockContext { public: - MockContext(IRequest * request, IResponse * response) + MockContext(MockRequest * request, MockResponse * response) : request_{request}, response_{response} {} - auto GetRequest() -> IRequest * override { return request_; } - auto GetResponse() -> IResponse * override { return response_; } + auto GetRequest() -> MockRequest * { return request_; } + auto GetResponse() -> MockResponse * { return response_; } private: - IRequest * request_; - IResponse * response_; + MockRequest * request_; + MockResponse * response_; }; - class MockEventProvider : public IEventProvider { + class MockEventProvider { public: - auto SetErrorStatus(std::int32_t errorStatus) -> void override { + auto SetErrorStatus(std::int32_t errorStatus) -> void { errorStatus_ = errorStatus; } std::int32_t errorStatus_ = 0; }; + static_assert(Request); + static_assert(Response); + static_assert(Context); + static_assert(EventProvider); + // HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED), as the plain value the // handler reports for forbidden requests. constexpr std::int32_t kAccessDeniedError = @@ -81,7 +96,7 @@ TEST(TestCore, TestModuleHandlerOnBeginRequestForbiddenUrl) MockResponse response; MockContext context{&request, &response}; MockEventProvider provider; - ModuleHandler handler; + const ModuleHandler handler; const Verdict verdict = handler.OnBeginRequest(&context, &provider); @@ -99,7 +114,7 @@ TEST(TestCore, TestModuleHandlerOnBeginRequestGenericUrl) MockResponse response; MockContext context{&request, &response}; MockEventProvider provider; - ModuleHandler handler; + const ModuleHandler handler; const Verdict verdict = handler.OnBeginRequest(&context, &provider); @@ -111,4 +126,51 @@ TEST(TestCore, TestModuleHandlerOnBeginRequestGenericUrl) EXPECT_EQ("1", response.headers_[0].second); } +// The suffix match folds case, and must do so identically regardless of +// the active locale — see core::AsciiToLower. +TEST(TestCore, TestModuleHandlerOnBeginRequestForbiddenUrlIsCaseInsensitive) +{ + MockRequest request{L"/app/FoRbIdDeN"}; + MockResponse response; + MockContext context{&request, &response}; + MockEventProvider provider; + const ModuleHandler handler; + + const Verdict verdict = handler.OnBeginRequest(&context, &provider); + + EXPECT_EQ(Verdict::Finish, verdict); + EXPECT_EQ(403, response.statusCode_); +} + +// A path merely containing "/forbidden" further up does not end with it, +// so it is not rejected (documents the suffix-only semantics called out +// in ModuleHandler.h). +TEST(TestCore, TestModuleHandlerOnBeginRequestForbiddenPrefixIsAllowed) +{ + MockRequest request{L"/forbidden/child.html"}; + MockResponse response; + MockContext context{&request, &response}; + MockEventProvider provider; + const ModuleHandler handler; + + const Verdict verdict = handler.OnBeginRequest(&context, &provider); + + EXPECT_EQ(Verdict::Continue, verdict); + ASSERT_EQ(1U, response.headers_.size()); +} + +TEST(TestCore, TestModuleHandlerOnBeginRequestShortUrl) +{ + MockRequest request{L"/a"}; + MockResponse response; + MockContext context{&request, &response}; + MockEventProvider provider; + const ModuleHandler handler; + + const Verdict verdict = handler.OnBeginRequest(&context, &provider); + + EXPECT_EQ(Verdict::Continue, verdict); + ASSERT_EQ(1U, response.headers_.size()); +} + }