fix(portal): P1 behaviour fixes from v0.2.0 audit (CORS deny-cache, validator merge, PUT→PATCH, disable preserves origins)#104
Merged
Conversation
…alidator merge, PUT→PATCH, disable preserves origins) Closes the four P1 behaviour findings from the v0.2.0 portal audit. Tur 1 (#101) shipped the P0 security fixes, Tur 2 (#102) shipped the test coverage, Tur 3 (#103) shipped the docs; this Tur 4 closes the behaviour delta. 1. PortalCorsMiddleware deny-cache. HandlePreflightAsync now caches both allow and deny outcomes via IMemoryCache for the same TTL as the per-app signing-key lookup (default 60s). Browsers don't cache rejected preflights, so every OPTIONS from a disallowed origin used to re-scan the portal-enabled app set + deserialize the JSON allowlist — a free DB hammer vector for any caller that knew the portal prefix. Cache key is lowercased-origin scoped (RFC 6454 §4 case-insensitive). New test Preflight_Deny_Decision_Is_Cached_Within_Ttl pins the behaviour by mutating the DB to allow the origin after a 403 and asserting the second call still 403s within the TTL window. 2. EndpointValidationRules helper consolidation. New extension methods (EndpointUrlSyntax, EndpointDescription, EndpointTransformExpression, EndpointCustomHeaders, EndpointAllowedIpsCidrs, EndpointSecretOverride) become the single source of truth for the field-shape rules shared between the 4 admin endpoint validators and the 2 portal endpoint validators. Without this consolidation, tightening a rule on one surface silently leaves the other surface weaker — exactly the drift pattern the audit flagged. Async DNS host-safety check stays in each validator (DependentRules + CustomAsync needs the full property selector). Behaviour unchanged. 3. PortalEndpointsController.Update → [HttpPatch]. The action's body semantics were always partial-replace (every field optional, only non-null fields applied) — that's PATCH, not PUT. Switching the verb aligns the wire surface with reality and stops misleading REST consumers that expect PUT to be full-replace. Route, request shape, response shape unchanged. Two existing tests ported from PutAsJsonAsync to PatchAsync + JsonContent.Create. 4. DashboardPortalController.Disable preserves origins. Removed the line that nulled AllowedPortalOriginsJson on disable. Disable now revokes only the auth surface (PortalSigningKey, PortalRotatedAt) and keeps the operator-curated CORS allowlist so re-enable doesn't force re-curation. Explicit clear path remains: PUT /portal/origins with {origins: []}. Renamed test Disable_Clears_SigningKey_And_Origins → Disable_Clears_SigningKey_But_Preserves_Origins with assertion flip.
4 tasks
voyvodka
added a commit
that referenced
this pull request
May 11, 2026
…e TTL) (#105) Locks in two v0.2.0 audit decisions that were marked 'worth recording' in the reviewer agent's punch list. Both are pure documentation — behaviour was already shipped in PRs #101 and #104. ADR-004 — Portal Signing Key Storage: - Plaintext varchar(64) at rest, mirroring the existing SigningSecret column. No application-level encryption; relies on operator's Postgres deployment posture. - Instant invalidation on rotate / disable. No grace window for overlapping old + new key validity. - One-shot reveal on enable / rotate; subsequent reads return only portalEnabled bool + rotated-at + origins (never the key). - Documents alternatives (pgcrypto, rotation grace, external KMS) and revisit triggers (compliance regimes, host-integration friction, key reuse beyond JWT verification). ADR-005 — Portal CORS Preflight Deny-Cache TTL: - 60s default (LookupCacheTtlSeconds reuse); cache both allow and deny outcomes; key is lowercased-origin scoped. - No synchronous invalidation hook from PUT /portal/origins: cache key is origin-scoped, operator action is app-scoped — bookkeeping cost outweighs the <=60s staleness. - Documents alternatives (per-app invalidation, shorter/longer TTL, negative-only cache) and revisit triggers (operator UX complaints, memory pressure via origin enumeration).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tur 4 of the v0.2.0 portal audit follow-up. Closes the four P1 behaviour findings; previous turns already shipped P0 security (#101), test coverage (#102), and docs (#103).
What's fixed
1.
PortalCorsMiddlewaredeny-cache (security hardening)HandlePreflightAsyncnow caches both allow and deny outcomes viaIMemoryCachefor the same TTL as the per-app signing-key lookup (default 60s). Browsers don't cache rejected preflights, so before this change everyOPTIONSfrom a disallowed origin re-scanned the portal-enabled app set + deserialized the JSON allowlist — a free DB hammer vector for any caller that knew the portal prefix. Cache key is lowercased-origin scoped (RFC 6454 §4 case-insensitive).Preflight_Deny_Decision_Is_Cached_Within_Ttlmutates the DB to allow the origin after a 403 and asserts the second call still 403s within the TTL window.2.
EndpointValidationRuleshelper consolidation (drift elimination)New extension methods (
EndpointUrlSyntax,EndpointDescription,EndpointTransformExpression,EndpointCustomHeaders,EndpointAllowedIpsCidrs,EndpointSecretOverride) become the single source of truth for the field-shape rules shared between the 4 admin endpoint validators (CreateEndpoint,UpdateEndpoint,DashboardCreateEndpoint,DashboardUpdateEndpoint) and the 2 portal endpoint validators (PortalCreateEndpoint,PortalUpdateEndpoint). Without this, tightening a rule on one surface silently leaves the other weaker — exactly the drift pattern the audit flagged. Async DNS host-safety check stays in each validator (theDependentRules+CustomAsyncpattern needs the full property selector). Behaviour unchanged in this commit — the consolidation is a precondition for the next round of rule tightening.3.
PortalEndpointsController.Update→[HttpPatch](REST semantic fix — minor breaking)The action's body semantics were always partial-replace (every field optional, only non-null fields applied) — that's
PATCH, notPUT. Switching the verb aligns the wire surface with reality and stops misleading REST consumers that expectPUTto be full-replace. Route, request shape, response shape unchanged. The embeddable<EndpointManager />component already issuesPATCH. Two existing tests ported fromPutAsJsonAsynctoPatchAsync+JsonContent.Create.4.
DashboardPortalController.Disablepreserves origins (operator UX fix)Removed the line that nulled
AllowedPortalOriginsJsonon disable. Disable now revokes only the auth surface (PortalSigningKey,PortalRotatedAt) and keeps the operator-curated CORS allowlist so re-enable doesn't force re-curation. Explicit clear path remains:PUT /portal/originswith{origins: []}. Renamed testDisable_Clears_SigningKey_And_Origins→Disable_Clears_SigningKey_But_Preserves_Originswith assertion flip.Test plan
dotnet build— 0 warnings, 0 errors.dotnet test— 278 / 278 passing post-refactor (+1 new CORS test = 279 total after this PR).Breaking change classification
Minor breaking — only #3 (
PUT→PATCHon/api/v1/portal/endpoints/{id}). Direct HTTP callers that were sendingPUTwill receive405 Method Not Allowed. The shipped npm component (@webhookengine/endpoint-manager 0.1.0) already usesPATCH. Documented in CHANGELOG; no SemVer bump required because the portal is still pre-1.0 and the change happens within the same minor (v0.2.x → next).Follow-ups (tracked, not in this PR)
DOCKERHUB_TOKENscope fix + ~50 lines redundant comment cleanup.