fix(platform-api): enforce OpenAPI scopes - #2942
Conversation
…and tests - Changed endpoint paths in CreateSecretRequest and UpdateSecretRequest from /api/v1/secrets to /api/v0.9/secrets. - Updated integration tests to reflect the new API version for all secret-related operations. - Refactored SecretHandler to use a constant for the API base path, ensuring consistency across route registrations. - Added validation to ensure scope registry routes match the registered routes, enhancing security checks.
Thushani-Jayasekera
changed the title
📝 WalkthroughWalkthroughSecret endpoints now register only under the shared API base path. OpenAPI scope loading normalizes path parameters and validates route coverage. Scope enforcement resolves router patterns, bypasses validated paths, fails closed for undeclared routes, and validates configuration during startup. ChangesScope routing and authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant ScopeRegistry
participant RouteMatcher
participant ScopeEnforcer
participant Handler
Server->>ScopeRegistry: load merged OpenAPI scopes
Server->>RouteMatcher: validate registered route patterns
Server->>ScopeEnforcer: configure Routes and SkipPaths
ScopeEnforcer->>RouteMatcher: resolve request route pattern
ScopeEnforcer->>ScopeRegistry: look up normalized method and path
ScopeEnforcer->>Handler: allow request or return 403
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Thushani-Jayasekera
changed the title
ScopeEnforcer never matched a route
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platform-api/internal/middleware/openapi_scope_registry.go (1)
37-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize ServeMux exact anchors separately from path parameters.
Since this module targets Go 1.26, ServeMux can use
{$}as an exact-path anchor. The current regex also matches{$}and normalizes it to{}, so a route like/gateways/{$}would collide with ordinary/gateways/{gatewayId}entries in the scope registry. Update the normalization comment/exclusion to keep exact anchors distinct from named parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/middleware/openapi_scope_registry.go` around lines 37 - 53, Update normalizePathParams and its pathParamName pattern so the ServeMux exact-path anchor `{$}` is excluded from parameter normalization and remains distinct from named placeholders such as `{gatewayId}`. Revise the adjacent comment to document this exclusion while preserving the existing wildcard distinction and normalization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/middleware/authorization.go`:
- Around line 109-113: Update hasPathPrefix, used by the authorization
middleware’s SkipPaths bypass, to normalize the request path and require either
an exact match or a segment-boundary match against each trimmed skip prefix;
preserve valid nested paths while preventing similarly prefixed routes such as
secrets-admin or health-probe-fake from bypassing authorization.
---
Nitpick comments:
In `@platform-api/internal/middleware/openapi_scope_registry.go`:
- Around line 37-53: Update normalizePathParams and its pathParamName pattern so
the ServeMux exact-path anchor `{$}` is excluded from parameter normalization
and remains distinct from named placeholders such as `{gatewayId}`. Revise the
adjacent comment to document this exclusion while preserving the existing
wildcard distinction and normalization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bb69b0f-e4c7-48bb-a45c-ce68ab796895
📒 Files selected for processing (11)
platform-api/internal/dto/secret.goplatform-api/internal/handler/secret.goplatform-api/internal/handler/secret_integration_test.goplatform-api/internal/middleware/authorization.goplatform-api/internal/middleware/openapi_scope_registry.goplatform-api/internal/middleware/openapi_scope_registry_test.goplatform-api/internal/middleware/scope_enforcer_test.goplatform-api/internal/middleware/scope_route_validation.goplatform-api/internal/middleware/scope_route_validation_test.goplatform-api/internal/server/scope_route_coverage_test.goplatform-api/internal/server/server.go
| if p == "/" { | ||
| // A root prefix skips everything — preserved from the previous | ||
| // behaviour rather than silently narrowed here. | ||
| return true | ||
| } |
There was a problem hiding this comment.
This means a prefix inside prefixes list is empty? The prefix list is the auth skip paths right? Does this mean if a segment in the skip paths is empty, we skip the whole validation? Not sure we allow empty paths in the skip list, but we ideally shouldn't do that if thats the case. We should fail that at the startup itself.
There was a problem hiding this comment.
Bdw, shall we also check if we really need to expose skip paths in the config toml? Ideally we don't need to do that, unless if there's a specific use case. Because this is the product's implementation and nobody who's configuring shouldn't need to change it. Having the explicit scope validation on/off config should be sufficient IMHO.
There was a problem hiding this comment.
Because this is the product's implementation and nobody who's configuring shouldn't need to change it. Having the explicit scope validation on/off config should be sufficient IMHO.
- 1 will remove allowing to configure.
Ideally we don't need to do that, unless if there's a specific use case.
Yes, we can remove.
There was a problem hiding this comment.
Just to be sure, I mean, ONLY from config toml. External plugins may inject paths which they expose as skip auth. They are via plugins, not via the config toml.
There was a problem hiding this comment.
changes commited
Make the auth skip-path list non-configurable
platform-api/config/config.go
- Auth.SkipPaths retagged koanf:"-" — no longer bindable from config. The list now comes solely from DefaultConfig plus prefixes plugins declare via AuthSkipPaths().
- LoadConfig fails startup if a config file still carries platform_api.auth.skip_paths (koanf ignores unknown keys, so a stale entry would otherwise look effective while doing nothing).
- New exported ValidateAuthSkipPath — rejects "", /, non-/-prefixed paths, and ... Moved here from internal/server/plugins.go so config-sourced and plugin-sourced prefixes share one check.
- validateAuthConfig now runs it over every SkipPaths entry.
platform-api/internal/middleware/authorization.go
- Removed the p == "/" early-return in hasPathPrefix that let a root prefix bypass every route — now unreachable since / fails validation at startup.
Config surface / docs
- config-template.toml: removed the 20-entry skip_paths block.
- Helm values.yaml: removed auth.skipPaths; configmap.yaml: removed the with $auth.skipPaths template block.
- README.md: dropped skip_paths from the [platform_api.auth] row; replaced the "setting it replaces the default list" note with an explanation that the list is built-in (a wrong entry is an auth bypass), that plugins declare their own prefixes, that scope_validation is the enforcement knob, and that a stale key fails startup.
Tests
- TestLoadConfig_RemovedSkipPathsKey_Errors — stale key errors.
- TestLoadConfig_SkipPathsDefaultsSurvive — built-in defaults (/health, /api/internal/v1/secrets) still populate.
- Four new validateAuthConfig cases: /, empty, relative, traversal — all rejected.
- TestLoadConfig_MultiFile_ArrayReplaceNotAppend retargeted from skip_paths to server.cors.allowed_origins, since the old key is no longer bindable.
There was a problem hiding this comment.
Just to be sure, I mean, ONLY from config toml. External plugins may inject paths which they expose as skip auth. They are via plugins, not via the config toml.
Config TOML: removed. SkipPaths []string \koanf:"-" (config/config.go:171) — koanf can't populate it, and auth.skip_pathsin a config file aborts startup viaremovedConfigKeys.
Plugins: still supported. plugin.AuthSkipPathProvider (internal/plugin/plugin.go:111) is an optional interface a plugin implements. initPlugins collects each declared path, runs it through config.ValidateAuthSkipPath — startup aborts on an invalid/over-broad one (empty, no leading /, bare /, or containing ..) — and server.go:462 appends them to cfg.Auth.SkipPaths before the auth middleware is built, so the list is complete when the chain is assembled.
So the effective list is: built-in defaults (default_config.go:55) + plugin-declared prefixes. Operators get auth.authorization.enabled for on/off, not path editing.
One nuance worth knowing, since you said "external plugins": there are two plugin tiers, and only the internal tier can declare skip paths. The external tier deliberately cannot — TestInitPlugins_ExternalTierCannotDeclareAuthSkipPaths asserts that an external plugin with an AuthSkipPaths() method returning / contributes zero skip paths, because externalPlugin doesn't forward that interface. Middleware is forwarded for the external tier; skip paths are not. The asymmetry is intentional: external BeforePlatformChain middleware runs before auth and can't mark a request authenticated, whereas a skip path removes the auth check outright (GO-AUTH-004).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/config/config.go`:
- Around line 692-704: Replace ValidateAuthSkipPath in
platform-api/config/config.go:692-704 with structural validation of exact router
patterns, including methods where applicable, rather than accepting broad path
prefixes. In platform-api/internal/server/plugins.go:119, validate each plugin
exemption against the mux’s registered route patterns before adding it to the
auth chain, and ensure auth exemptions match resolved route identities rather
than raw r.URL.Path prefixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d354a490-274c-4c3f-9115-355b8423b26a
📒 Files selected for processing (14)
kubernetes/helm/platform-api-helm-chart/templates/configmap.yamlkubernetes/helm/platform-api-helm-chart/values.yamlplatform-api/README.mdplatform-api/config/config-template.tomlplatform-api/config/config.goplatform-api/config/config_multifile_test.goplatform-api/config/config_test.goplatform-api/internal/middleware/auth.goplatform-api/internal/middleware/authorization.goplatform-api/internal/middleware/openapi_scope_registry.goplatform-api/internal/middleware/scope_enforcer_test.goplatform-api/internal/server/plugin_scope_enforcement_test.goplatform-api/internal/server/plugins.goplatform-api/internal/server/scope_route_coverage_test.go
💤 Files with no reviewable changes (3)
- kubernetes/helm/platform-api-helm-chart/templates/configmap.yaml
- platform-api/config/config-template.toml
- kubernetes/helm/platform-api-helm-chart/values.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- platform-api/internal/middleware/openapi_scope_registry.go
- platform-api/internal/middleware/authorization.go
- platform-api/internal/middleware/scope_enforcer_test.go
…tication middleware and add corresponding tests.
|
Authorization settings were scattered across two places and one of them was the The server never read those two keys as IDP-specific, but the startup path did This PR groups all authorization settings into one Before / AfterConfig# BEFORE
[platform_api.auth]
mode = "idp"
scope_validation = true
[platform_api.auth.idp]
jwks_url = "https://accounts.example.com/oauth2/jwks"
issuer = ["https://accounts.example.com"]
validation_mode = "role"
role_mappings = "/etc/platform-api/roles.yaml"# AFTER
[platform_api.auth]
mode = "idp"
[platform_api.auth.authorization]
enabled = true
mode = "role"
role_mappings = "/etc/platform-api/roles.yaml"
[platform_api.auth.idp]
jwks_url = "https://accounts.example.com/oauth2/jwks"
issuer = ["https://accounts.example.com"]Key mapping
Helm Go types
Behaviour changesOld spellings fail startup, naming their replacement. koanf ignores unknown Role mode now works in Role mode is rejected with Role mode requires
Validation runs in every auth mode. The authorization block was previously Defaults are unchanged. Authorization is enabled and scope-based by default; @malinthaprasan @Krishanx92 shall I add this change in this PR. I think this improves clarity in authorization part. WDYT? |
Thushani-Jayasekera
changed the title
ScopeEnforcer never matched a route
2 participants
Root cause
1. The route pattern was never available (the reported bug)
r.Patternis populated byServeMuxduring its ownServeHTTP. The enforcer sitsoutside it (
gohttpkit.Chain(chain...)(mux)), so it can never observe a pattern thatway.
This is a regression with a known origin. Commit
a232853ac("removing gin golibrary from source", 26 Jun 2026) migrated this middleware off gin. The gin version
read
c.FullPath(), and gin'srouter.Usemiddleware runs inside the engine afterroute matching, so it returned the matched template correctly:
The port substituted
r.Pattern— the correctnet/httpequivalent — but the chainnow wraps the mux from outside, where that value is never set. Scope enforcement
worked before that commit and has been ineffective since.
Changes
Resolve the route pattern from the router
ScopeEnforcerConfiggains aRoutes RouteMatcherfield, satisfied by*http.ServeMux. The enforcer asks the router which pattern the request will match,without serving it.
r.Patternis still preferred when non-empty, so the middlewarekeeps working if it is ever moved inside the router.
Fail closed on an unenforceable configuration
ScopeEnforcernow returns an error when scope validation is enabled but no registryor no route matcher was supplied. The server refuses to start rather than degrading
to a pass-through (GO-AUTH-011).
Normalize path-parameter names out of the registry key
/gateways/{gatewayId}and/gateways/{apiId}now produce the same key. The wildcard{p...}form stays distinct, since it matches a different set of paths.Parse only real operations from a path item
Path-item parsing decodes only keys that are HTTP methods, ignoring
parameters,summary,$refand friends.Drop the legacy
/api/v1/secretsaliasSecretHandler.RegisterRoutesnow registers onconstants.APIBasePathonly, matchingevery other handler. The integration tests and two stale DTO doc comments are migrated
to
/api/v0.9. With the alias gone, deny-by-default has no exceptions on the publicbase path.
Validate the spec↔route mapping at startup
New
ValidateScopeRegistryRoutesprobes every declared operation against the router.If a declared operation resolves to a structurally different route, the real route
has no entry of its own and would be denied at runtime — so startup fails loudly
instead. A declared operation matching nothing is not an error: it is a dead registry
entry protecting a route that does not exist, and such requests already get a 404.
Testing
The hardening breaks nothing
internal/server/scope_route_coverage_test.goregisters every real handler — coreplus event-gateway plugin — onto a mux and validates it against the shipped
resources/openapi.yamland the plugin's embedded spec.All 114 core operations and all 34 plugin operations map exactly, so no live
endpoint regresses to a 403 under deny-by-default. The same check runs at startup, so
future drift is caught at boot as well as at build time.
The plugin defects, proven end to end
internal/server/plugin_scope_enforcement_test.gocovers defects 2 and 3 with threetests, since neither was observable at runtime — both presented exactly like the
reported bug, as "no scope requirement found":
TestPluginSpecLoadsWithItsScopes— the plugin spec now yields 34 operations;before the parser fix it yielded an error, and therefore zero. The merged registry
goes 114 → 148.
TestEveryPluginRouteResolvesToDeclaredScopes— all 32 registered plugin routes,probed as live requests through the real mux, resolve to a declared scope despite the
spec saying
{apiId}where the handlers register{webSubApiId}/{webBrokerApiId}.TestScopeEnforcementOnRealPluginRoutes— drives the real enforcer over the realrouter and real merged registry on
DELETE /api/v0.9/websub-apis/{...}, the exactroute where spec and handler disagree on the parameter name:
ap:websub_api:readap:webbroker_api:deleteap:websub_api:deleteap:websub_api:manageap:websub_api:*nextis a sentinel rather than the mux — handlers built with nil services wouldpanic if a request reached one — but the mux is still the route matcher, so pattern
resolution follows the production path.
The secrets alias removal
TestSecretsRoutesAreRegisteredOnTheBasePathasserts the five real routes are stillregistered on
/api/v0.9and that each resolves to a pattern the registry has ascope for, so removing the alias cannot have silently taken the real API with it.
The ~25 call sites in
secret_integration_test.gonow exercise/api/v0.9/secrets,and the whole
internal/handlersuite passes against the new path.Results
gofmtreports no issues on any file touched here.Notes for reviewers
Per-handler organization scoping was not audited here. Handlers derive the
tenant from token claims independently of this middleware, so that control is
untouched either way — but this PR makes no claim about which operations were
reachable cross-tenant while enforcement was broken. Worth a separate pass before
the advisory states a blast radius.
go build -tags experimentalis broken on a clean tree(
api.OrganizationSubscription/api.OrganizationQuotaundefined inplugins/eventgateway/plugin.go). Pre-existing and unrelated to this change, soleft alone. The plugin path above is therefore compile-verified by the tests but not
runnable end-to-end until that is fixed.
The event-gateway plugin still serves its HMAC-secret routes from
/api/v1(
/api/v1/websub-apis/{id}/secrets). These are declared in its own spec and areproperly enforced, so they are left as-is — but unifying the plugin onto
/api/v0.9is a reasonable follow-up.Nothing fails if the
/api/v1/secretsalias is re-added. It would be a dead403 route rather than an unprotected one, since deny-by-default neutralizes any
route with no declared scope. The security property holds; there is simply no early
warning.
Behavioural changes to be aware of
/api/v1/secretsnow returns 404. Callers must use/api/v0.9/secrets. Noin-repo consumer was on the old path, but any external caller still pointing at it
will break — worth calling out in release notes.
securityblock now returns 403 instead ofbeing served unauthenticated. This is the intended hardening, but it is a behaviour
change for any such route outside
SkipPaths.or if a declared operation resolves to a structurally different registered route.