feat: module pipeline hooks after routing and after authentication; bounded api key lookup cache by alexeyshibanov · Pull Request #3110 · VirtoCommerce/vc-platform

alexeyshibanov · 2026-09-06T05:14:52Z

Description

Modules can add middleware at two more points of the request pipeline. IPlatformStartup gains ConfigureAfterRouting and ConfigureAfterAuthentication, both default interface members with empty bodies. The first is called after UseRouting and the static-file middlewares and before UseAuthentication: use it for middleware that needs the matched endpoint but must run before the caller is identified. The second is called after UseAuthentication and the account-lockout middleware and before UseAuthorization: use it for middleware that needs the authenticated principal and the matched endpoint and must still observe requests authorization is about to reject. Until now a module could reach the pipeline only from IPlatformStartup.Configure, which runs before UseRouting, and from IModule.PostInitialize, which runs after UseAuthorization, so neither position was reachable from a module. The interface's four pre-existing members gain default implementations too, so an implementer declares only the phases it uses.

A module the platform will not initialize no longer contributes a platform startup. Startup discovery was the one phase that did not consult a module's errors. Assemblies are loaded whatever a module's state, so a module rejected by Validate for an incompatible platformVersion, a missing dependency, or a dependency that itself failed still had its IPlatformStartup discovered and its hooks run, while InitializeModules skipped it and left its services unregistered - the middleware it registered then resolved what nobody had registered. The guard sits in discovery rather than in the drivers, so it covers all six RunConfigure* members, including the four that predate this change. A failure raised after discovery, such as a module whose Initialize throws, is not covered; that would need each driver to know which module a startup came from.

The api key lookup cache no longer lets a caller size or extend its own entry. UserApiKeyService.GetApiKeyByKeyAsync cached hits and misses under a key built from the candidate string as presented, with the platform's default sliding expiration. A caller supplying arbitrary candidates therefore chose the cache key's length, and a repeated guess kept its miss entry resident indefinitely. Now a candidate longer than the stored column (DbContextBase.Length128, the ApiKey column's HasMaxLength) is keyed by its SHA-256 digest instead of its text, and a miss expires absolutely after at most 30 seconds. Both are reductions, not bounds: the digest bounds the size of one entry, not the number of entries; the absolute expiration bounds residency, not the attempt rate. Every novel attempt still costs one indexed read. The repository is still queried with the candidate exactly as presented, so the method returns for every input what it returned before.

The api key parameter name is a shared constant. PlatformConstants.Security.ApiKeyAuthentication.ParamName ("api_key") names the query-string and header parameter the api key scheme reads, so a module can reference it without depending on the host assembly, where ApiKeyAuthenticationOptions lives. The option's default now reads the constant; its value is unchanged.

The pipeline segment from forwarded headers through authorization is extracted into public static Startup.ConfigureRequestPipeline, so the tests drive the shipped composition rather than a copy of it. VirtoCommerce.Platform.Web is not packable and the only caller outside Startup is the test harness, so internal static plus InternalsVisibleTo would serve equally well — a question for the review, not a preference.

No schema change, no migration, no new setting or configuration key, no new dependency.

New GraphQL queries and mutations

None — the platform host exposes no GraphQL schema and this change adds none.

Changes to existing GraphQL queries and mutations

None.

New public services / interfaces / methods

IPlatformStartup.ConfigureAfterRouting(IApplicationBuilder app, IConfiguration config) and IPlatformStartup.ConfigureAfterAuthentication(IApplicationBuilder app, IConfiguration config) — default interface members with empty bodies. Override one to register middleware at that point; an implementer that overrides neither is unaffected.
ModuleBootstrapper.RunConfigureAfterRouting and ModuleBootstrapper.RunConfigureAfterAuthentication — the drivers Startup calls; each invokes the hook on every registered startup in registration order. Non-virtual, matching the four existing RunConfigure* members: the type is registered only as IModuleService, which declares none of them, and is reached through the static Instance, so an override would have no path the platform itself uses. The tests populate Startups, which already returns the live list, through the public constructor; the one visibility change is DiscoverStartupsInternal, now internal so a test can drive the error filter, and the project already grants InternalsVisibleTo to the test assembly.
Startup.ConfigureRequestPipeline(IApplicationBuilder app, IConfiguration configuration, IWebHostEnvironment webHostEnvironment) — the extracted segment, public so a test project can drive it.
PlatformConstants.Security.ApiKeyAuthentication.ParamName — "api_key".

New protected methods (extensibility)

None.

Breaking changes

None, checked at each of the three points where a change can break a consumer:

Compile. Both new interface members have default implementations, so no existing IPlatformStartup implementer needs a code change. The four pre-existing members gained defaults as well, which is source and binary compatible: an existing implementation still wins over the default. No member was removed, renamed or resignatured.
Package upgrade without recompilation. A module compiled against an earlier VirtoCommerce.Platform.Core loads unchanged; a default interface member does not require the implementer to be recompiled.
Runtime. GetApiKeyByKeyAsync returns the same result for every input; only the cache key and the lifetime of a miss entry changed. ApiKeyAuthenticationOptions.ApiKeyParamName keeps its default value.

Two runtime effects worth knowing before upgrading:

A module carrying errors loses its hooks. An incompatible platformVersion, a missing dependency, or a dependency that itself failed now also stops that module's IPlatformStartup from being discovered, so none of its Configure* hooks run. Before this change they ran even though IModule.Initialize was skipped for the same module. Nothing in the toolchain reports it: a deployment that relies on a module which loads with errors and still registers middleware will find that middleware gone.
Forward compatibility fails silently in the other direction. A module compiled against this VirtoCommerce.Platform.Core that overrides a new hook and is deployed on an older platform host loads fine and its hook is never called — the older ModuleBootstrapper has no driver for it. A module that relies on either hook must raise platformVersion in its manifest to the version that ships this change.

No [Obsolete] was introduced — nothing was removed or renamed.

New dependencies

None. SHA-256 comes from System.Security.Cryptography in the shared framework.

Note (hook positions). The first hook sits after UseDefaultFiles rather than immediately after UseRouting, so nothing registered there runs on requests the static-file middlewares answer. The second sits immediately after UseAccountLockoutMiddleware; immediately after UseAuthentication would satisfy the same design, since the lockout middleware is cookie-gated and does nothing for a server-to-server call — the chosen position is the more general one, not the necessary one. No test moves if the review prefers the other.

Note (default interface members). All six IPlatformStartup members now carry an empty default, not only the two new ones: an implementer that wants one hook no longer declares four members it does not use, and the platform has no implementer of its own to migrate. The cost is that the compiler no longer refuses a mistyped member - it becomes an ordinary new method, the empty default silently wins, the phase is never entered and nothing says so; an implementer that needs the guarantee holds its instance as IPlatformStartup or asserts over GetInterfaceMap. The alternative considered for the two new hooks was a separate opt-in interface; the default-member form was chosen so an existing implementer sees no change at all.

Note (the 30 seconds). The miss is still cached at all because not caching it would turn every repeat of one guessed key into a query against a store shared with every other module; what changes is how long it stays. Applied as a clamp, not an assignment: the miss entry gets the shorter of the configured CachingOptions.CacheAbsoluteExpiration and 30 seconds, so a deployment that already configures a shorter absolute expiration keeps it — the change reduces residency and never extends it. Whether the value should instead be an option on CachingOptions is open. The digest threshold is not a new number; it is the column's own length.

Note (two obligations the tests cannot enforce). The harness drives ConfigureRequestPipeline, not Configure, so it cannot check that Configure still calls the extracted method between UseSecurityHeaders and ExecuteSynchronized; and because the platform reads ASPNETCORE_FORWARDEDHEADERS_ENABLED from the process environment, the two test classes that build a host through AddForwardedHeaders must stay in [Collection(PlatformPipelineCollection.Name)] so they never run in parallel with each other. Both hold on this branch; they are named here so a later edit knows what to re-check.

Note (raised, not fixed). Three things this change surfaces and deliberately leaves alone:

Basic and api key authentication consume lockout state without contributing to it. Both handlers read IsLockedOutAsync, but neither writes AccessFailedAsync: BasicAuthenticationHandler verifies with UserManager.CheckPasswordAsync, which on a wrong password returns false without touching the failure count (CustomUserManager does not override it), and the api key handler has no password to fail. Lockout does trip elsewhere — SecurityController.Login, the OpenIddict password grant and ChangePassword all reach AccessFailedAsync — so these two surfaces bypass the limit rather than disable it. A Basic attempt also costs a password-hash verification where an api key attempt costs an index seek, so the cheaper-looking surface is the more expensive one. Left alone because lockout accounting for these schemes is a policy decision, not a cache fix.

Startup.Configure's own app.UseForwardedHeaders() is inert on the shipped host. Program.cs builds through Host.CreateDefaultBuilder(args).ConfigureWebHostDefaults(...), which registers ForwardedHeadersStartupFilter. With ASPNETCORE_FORWARDEDHEADERS_ENABLED=true that filter calls UseForwardedHeaders() before Startup.Configure is entered and the second call is a no-op by the framework's duplicate-registration marker; with the variable unset the call registers a middleware whose ForwardedHeaders is None. The platform's AddForwardedHeaders still does real work — it runs after the framework's own options setup and widens the processed headers to All — so it is the configuration under test here. The line is kept because deleting it inside an extraction would turn the extraction into a behaviour change; its removal is proposed as a separate follow-up.

Swagger still spells the parameter name as string literals (SwaggerServiceCollectionExtensions uses "api_key" four times) rather than the new constant. Cosmetic, and left out to keep this diff to its two subjects.

References

QA-test:

Jira-link:

Artifact URL:

Image tag:
ghcr.io/VirtoCommerce/platform:3.1067.0-pr-3110-4eca-pipeline-hooks-and-credential-cache-4ecabddd

Note

Medium Risk
Changes sit in the authentication/authorization middleware pipeline and API key resolution caching; modules with load errors lose startup hooks at runtime, which can break deployments that relied on that behavior.

Overview
Modules can register middleware at two new IPlatformStartup phases: ConfigureAfterRouting (after routing/static files, before authentication) and ConfigureAfterAuthentication (after authentication/lockout, before authorization). All six interface methods now have empty default implementations, so implementers only override the phases they need. Startup.ConfigureRequestPipeline extracts the forwarded-headers→authorization segment and invokes ModuleBootstrapper.RunConfigureAfterRouting / RunConfigureAfterAuthentication in that order.

Failed modules no longer contribute startups: discovery skips modules with validation errors so their middleware cannot run while their DI services were never registered. Startup discovery is refactored into TryCreateStartup.

API key lookup caching is tightened in UserApiKeyService.GetApiKeyByKeyAsync: overlong candidates use a SHA-256 digest in the cache key (DB lookup still uses the raw value), and cache misses get a short absolute TTL (≤30s, no sliding) instead of indefinite sliding renewal. PlatformConstants.Security.ApiKeyAuthentication.ParamName centralizes the api_key parameter name for modules.

Tests cover hook ordering, discovery filtering, cache behavior, and the real pipeline composition via RequestPipelineHarness.

^{Reviewed by Cursor Bugbot for commit 4ecabdd. Bugbot is set up for automated code reviews on this repo. Configure here.}

…a test

… test server

…ks observe

…s the resolved address

…changed

…the host can reference

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…he key

… sliding window

…ess permission constant Replace spec identifiers (AC15/16/17/39/40b) with the test method names they referred to, so the reference resolves in this repository. Reword the two IPlatformStartup xmldoc summaries so they no longer imply a matched endpoint or an authenticated principal is guaranteed. Split RequestPipelineHarness's PermissionClaimType into a separate PermissionHeader constant for the header lookup, keeping PermissionClaimType for the claim type.

… the cache-key comments Fold the two existing clamp facts in UserApiKeyServiceTests into one Theory with three rows, adding the untested case (configured absolute above 30s) that carries the change's actual security claim. Trim BuildApiKeyCacheKey's comment to drop the restated defect and the stray direction word, and hedge the negative-entry comment's "will never be read again" into a stated assumption rather than a fact.

…s them

vc-ci · 2026-09-06T05:15:01Z

Review task created: https://virtocommerce.atlassian.net/browse/VCST-5889

…, not by when they were written

…le requests never reach it

…ook precedes UseAuthorization Both facts were reachable only from a test name or from another assembly. The negative-entry block reads as two operations over empty fields until you know GetDefaultCacheEntryOptions fills sliding rather than absolute, so it now states what a tidy-up would break: replacing the clamp with an assignment lengthens negative entries wherever CacheAbsoluteExpiration is shorter, and dropping the sliding reset returns a probe to the repository wherever the configured sliding window is shorter than the bound. The RunConfigureAfterAuthentication call named its guarding test but not the reason, while its neighbour above named the reason but no test; both now carry the mechanism - authorization short-circuits what it rejects, so a hook placed later sees only the requests that passed.

One assertion restated the initializer it guarded - ApiKeyParamName is declared as PlatformConstants.Security.ApiKeyAuthentication.ParamName, so breaking it takes a deliberate edit to that same line. The other pinned the literal, which nothing else in the repository does, but the handler that reads the parameter has no tests at all: pinning its name while its behaviour is uncovered guards the cheaper half. Extracting the constant in ea48a03 moved no risk, it only made a long-standing gap visible; covering ApiKeyAuthenticationHandler belongs to its own change.

The lambda-over-options form is what SonarCloud flags on this pull request; the builder is the .NET 8 replacement and registers the same services. Only the harness moves - the platform's own AddAuthorization in Startup sets DefaultPolicy and FallbackPolicy, which the builder spells differently, and the analyzer does not flag it because the branch never touched it.

IsAbstract read through reflection is the same metadata fact the compiler already enforces: NoHookPlatformStartup implements neither hook, so a hook losing its default would stop the whole test project from compiling. The assignability assertion restated that class's own base clause. What is left - the four members stay abstract - is a change detector on the declaration, and the next commit changes that declaration deliberately.

An implementer had to declare the four members it does not use: a module that wants only the two routing and authentication hooks carried four empty methods to get them. The platform has no implementer of its own - only test doubles - and adding a default to an abstract interface member is source and binary compatible, so every existing implementer keeps compiling with its own implementation still winning. The cost is that the compiler no longer refuses a mistyped member: it becomes an ordinary new method and the empty default silently wins, so the phase is never entered and nothing says so. That trade was already accepted for the two hooks; an implementer that needs the guarantee holds its instance as IPlatformStartup or asserts over GetInterfaceMap. The doubles shed the members they only declared in order to compile.

Startups are discovered by walking the module list, which the catalog has sorted by dependency, so a module's middleware is registered after that of the modules it depends on. Nothing held that: sorting the list any other way, or running the loop concurrently, would reorder middleware inside other people's modules with nothing to announce it. One shared log replaces the counters, so a single sequence carries the order, the one call per implementation, and a runner invoking the wrong hook - the last of which the counters caught only because they were read in pairs. Verified by reversing the loop, which reddens the routing test and leaves the authentication one green.

Startup discovery was the one phase that did not consult the module's errors. Assemblies are loaded whatever a module's state - LoadModule never reads Errors - so a module rejected by Validate for an incompatible platform version, a missing dependency, or a dependency that itself failed still had its IPlatformStartup discovered and its hooks run, while InitializeModules skipped it and left its services unregistered. The middleware it registered then resolved what nobody had registered, and the symptom was a failure on every request through the pipeline rather than a module that quietly does nothing. The guard belongs at discovery rather than in the runners: filtering the list where it is built covers all six of them, including the four that predate the routing and authentication hooks, without touching their bodies. Discovery becomes internal so the test can drive it; the project already grants InternalsVisibleTo to the test assembly. Failures raised after discovery - a module whose Initialize throws - are not covered by this and would need the runners to know which module each startup came from.

…ason The theory fed 'k' repeated at six lengths and asserted a raw candidate cannot key the same entry as a digested one. A run of 'k' cannot equal a hex digest at any length, so the assertion held because the inputs differed, not because the discriminator separates them - a name stating a hazard over data that cannot reach it makes the hazard look covered. The case beside it feeds a raw candidate that IS the digest of the long one and carries the whole claim. The boundary case pinned the threshold at the column length rather than one below it. Either way the repository is queried with the candidate as presented and the key stays stable across calls, so crossing that boundary changes the shape of a key and nothing observable. Both remaining branches of BuildApiKeyCacheKey stay covered: the collision case sends a short candidate and a long one.

The case compared status, body and headers against a baseline to show that a startup overriding neither hook changes nothing observable. Now that every member carries a default, the class it drove is empty, so the comparison asserts that empty bodies are empty - and the two assertions worth keeping, that a startup without hooks does not short-circuit the dispatch loop, moved to the unit test where the same double sits in the middle of the list. Deleting it leaves the harness's extraStartup parameter, its body reader and this project's copy of the no-hook double with no callers, so those go too. The four remaining cases each pin a position no other test can: the matched endpoint, the straddle over authentication, the invocation on a request authorization rejects, and the resolved client address.

Adding the error guard pushed DiscoverStartupsInternal from a cognitive complexity of exactly 15 to 17, which SonarCloud reports as a new critical issue and the quality gate refuses. Moving the guard into the enumeration would buy the two points back and leave the method sitting on the threshold again, so the per-module work becomes TryCreateStartup: resolve the type, reject what is missing or does not implement the interface, construct it, and hand back the instance or null. The loop is then the discovery policy alone - skip a module with errors, keep what comes back. Behaviour is unchanged. The construction check reads as an early return rather than a nested success branch, which is what removes the deepest nesting the rule was counting.

…d-credential-cache

The error guard sat in the loop while every other reason a module yields no startup - no declared type, no assembly, type not found, not an implementer, construction failed - sat in the helper. That split put the invariant in the caller rather than in the one method that decides whether a module contributes a startup, which is the shape the fix was about: discovery was the phase that forgot the check while its siblings kept it. The line drawn was not even consistent, since module.Assembly is equally a product of an earlier phase and was already inside. No record is written when the guard rejects. Validation has already logged the module and every one of its errors by name, so a second line here would repeat what the operator can already read.

sonarqubecloud · 2026-09-08T12:12:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

…ounded api key lookup cache (#3110)

alexeyshibanov and others added 13 commits September 5, 2026 15:41

feat: let a module add middleware after routing and after authentication

2386fb5

feat: make the routing-to-authorization pipeline segment drivable by …

1f6b846

…a test

feat: add a harness that drives the extracted pipeline segment over a…

d9ba05e

… test server

feat: assert the endpoint and the authentication straddle the two hoo…

e6559e1

…ks observe

feat: assert the second hook sees rejected requests and the first see…

1d052b4

…s the resolved address

feat: assert a startup overriding neither new hook serves requests un…

f83fc99

…changed

feat: share the api key parameter name as a constant callers outside …

ea48a03

…the host can reference

fix: stop an oversized api key candidate sizing its own cache entry

f9314d8

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

fix: assert a raw candidate cannot produce a digested candidate's cac…

e09d22f

…he key

fix: expire an unresolved api key lookup in 30 seconds instead of the…

0d52cb2

… sliding window

fix: trim comments that restate the code or a test that already guard…

528ec89

…s them

vc-ci added the review task created label Sep 6, 2026

alexeyshibanov added 12 commits September 6, 2026 08:44

fix: name the hook-free startup double and its tests by what they are…

cc00227

…, not by when they were written

fix: say on each hook which middlewares precede it and that static-fi…

07921d7

…le requests never reach it

style: put each default hook implementation's braces on their own lines

9e8d0a0

alexeyshibanov marked this pull request as ready for review September 8, 2026 04:20

alexeyshibanov added 2 commits September 8, 2026 07:55

Merge remote-tracking branch 'origin/dev' into feat/pipeline-hooks-an…

7e4421b

…d-credential-cache

alexeyshibanov requested a review from OlegoO September 8, 2026 05:14

OlegoO approved these changes Sep 8, 2026

View reviewed changes

Merge branch 'dev' into feat/pipeline-hooks-and-credential-cache

4ecabdd

OlegoO merged commit d97794b into dev Sep 8, 2026
17 checks passed

OlegoO deleted the feat/pipeline-hooks-and-credential-cache branch September 8, 2026 12:26

OlegoO pushed a commit that referenced this pull request Sep 8, 2026

feat: module pipeline hooks after routing and after authentication; b…

1e3d3ee

…ounded api key lookup cache (#3110)

Navigation Menu

Uh oh!

feat: module pipeline hooks after routing and after authentication; bounded api key lookup cache - #3110

feat: module pipeline hooks after routing and after authentication; bounded api key lookup cache#3110
OlegoO merged 29 commits into
devfrom
feat/pipeline-hooks-and-credential-cache

alexeyshibanov commented Sep 6, 2026 •

edited by vc-ci

Loading

Uh oh!

vc-ci commented Sep 6, 2026

Uh oh!

sonarqubecloud Bot commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

alexeyshibanov commented Sep 6, 2026 • edited by vc-ci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

New GraphQL queries and mutations

Changes to existing GraphQL queries and mutations

New public services / interfaces / methods

New protected methods (extensibility)

Breaking changes

New dependencies

References

QA-test:

Jira-link:

Artifact URL:

Uh oh!

vc-ci commented Sep 6, 2026

Uh oh!

sonarqubecloud Bot commented Sep 8, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexeyshibanov commented Sep 6, 2026 •

edited by vc-ci

Loading