Adding external plugin support#2812
Closed
malinthaprasan wants to merge 3 commits into
Closed
Conversation
malinthaprasan
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Induwara04,
Krishanx92,
PasanT9,
Piumal1999,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub,
tharindu1st and
thivindu
as code owners
July 23, 2026 07:35
Contributor
📝 WalkthroughWalkthroughThe PR introduces a public plugin development kit, explicit internal and external plugin wiring, plugin-provided authentication and middleware hooks, and a platform façade for configuring and running the server. ChangesPlugin runtime and platform façade
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformApp
participant StartPlatformAPIServer
participant ExternalPlugin
participant MiddlewareChain
PlatformApp->>StartPlatformAPIServer: start with configured pdk plugins
StartPlatformAPIServer->>ExternalPlugin: initialize with pdk.Deps
ExternalPlugin->>StartPlatformAPIServer: provide routes, auth paths, and middleware
StartPlatformAPIServer->>MiddlewareChain: assemble platform and plugin middleware
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/server/server.go`:
- Around line 458-460: Update the plugin initialization loop in the server setup
to track successfully initialized plugins and, when a later plugin’s Init fails,
shut down all previously initialized plugins before returning the initialization
error. Reuse each plugin’s existing cleanup or shutdown API, preserve reverse
initialization order if supported, and keep the current wrapped error from the
failing plugin.
- Around line 473-477: Update LocalJWTAuthMiddleware to validate and normalize
the request’s EscapedPath before matching, rejecting forbidden traversal or
malformed sequences before authentication bypass evaluation. Canonicalize each
configured config.SkipPaths entry and replace the raw strings.HasPrefix
comparison with a path-aware prefix check that only matches complete path
segments, preserving valid public-path skips.
In `@platform-api/pdk/deps.go`:
- Around line 38-45: Update Deps and the StartPlatformAPIServer plugin wiring to
remove the live Config *config.Server from the shared dependency object. Expose
only immutable, narrowly scoped configuration values or interfaces required by
plugins, and ensure shared capability fields cannot be replaced or used to
mutate server configuration while preserving the existing plugin functionality.
- Around line 47-64: Update platform-api/pdk/deps.go lines 47-64 so the Gateways
capability derives organization scope from the request context/organization
claim instead of accepting caller-supplied orgID parameters, while preserving
the CRUD operations. Update platform-api/pdk/middleware/auth.go lines 30-40 so
external AuthSkipPaths are not appended to the server-managed authentication
skip-list; keep bypass prefixes controlled by the platform/admin configuration.
In `@platform-api/pdk/plugin.go`:
- Around line 59-65: Centralize validation for authentication skip paths and
apply it to both plugin tiers: update AuthSkipPathProvider handling in
platform-api/pdk/plugin.go (lines 59-65) and the internal plugin path handling
in platform-api/internal/plugin/plugin.go (lines 100-109) to accept only
normalized, route-bound allowlisted prefixes. Reject malformed or overly broad
declarations and fail closed before adding paths to the authentication skip
list; ensure authentication decisions use normalized request paths rather than
unsafe raw prefix matching.
🪄 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: bb85e271-bdd6-4b4d-bb0c-ccbdaa88b40c
📒 Files selected for processing (14)
platform-api/cmd/main.goplatform-api/cmd/plugins_experimental.goplatform-api/cmd/plugins_oss.goplatform-api/internal/plugin/plugin.goplatform-api/internal/plugin/registry.goplatform-api/internal/server/external_plugin.goplatform-api/internal/server/server.goplatform-api/pdk/deps.goplatform-api/pdk/middleware.goplatform-api/pdk/middleware/auth.goplatform-api/pdk/plugin.goplatform-api/platform/middleware.goplatform-api/platform/options.goplatform-api/platform/platform.go
💤 Files with no reviewable changes (1)
- platform-api/internal/plugin/registry.go
Comment on lines
+458
to
460
| for _, p := range plugins { | ||
| if err := p.Init(pluginDeps); err != nil { | ||
| return nil, fmt.Errorf("plugin %q failed to initialize: %w", p.Name(), err) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back initialized plugins when a later plugin fails.
A later Init error returns immediately, leaving earlier external/internal plugins running. platform.App.Run returns that error rather than terminating the process, so plugin goroutines or connections can leak.
Proposed rollback
+ initializedPlugins := make([]plugin.Plugin, 0, len(plugins))
+ startupSucceeded := false
+ defer func() {
+ if startupSucceeded {
+ return
+ }
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ for i := len(initializedPlugins) - 1; i >= 0; i-- {
+ if err := initializedPlugins[i].Shutdown(ctx); err != nil {
+ slogger.Error("Plugin rollback shutdown error", "plugin", initializedPlugins[i].Name(), "error", err)
+ }
+ }
+ }()
+
for _, p := range plugins {
if err := p.Init(pluginDeps); err != nil {
return nil, fmt.Errorf("plugin %q failed to initialize: %w", p.Name(), err)
}
+ initializedPlugins = append(initializedPlugins, p)
// ...
}
+ startupSucceeded = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, p := range plugins { | |
| if err := p.Init(pluginDeps); err != nil { | |
| return nil, fmt.Errorf("plugin %q failed to initialize: %w", p.Name(), err) | |
| initializedPlugins := make([]plugin.Plugin, 0, len(plugins)) | |
| startupSucceeded := false | |
| defer func() { | |
| if startupSucceeded { | |
| return | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| for i := len(initializedPlugins) - 1; i >= 0; i-- { | |
| if err := initializedPlugins[i].Shutdown(ctx); err != nil { | |
| slogger.Error("Plugin rollback shutdown error", "plugin", initializedPlugins[i].Name(), "error", err) | |
| } | |
| } | |
| }() | |
| for _, p := range plugins { | |
| if err := p.Init(pluginDeps); err != nil { | |
| return nil, fmt.Errorf("plugin %q failed to initialize: %w", p.Name(), err) | |
| } | |
| initializedPlugins = append(initializedPlugins, p) | |
| // ... | |
| } | |
| startupSucceeded = true |
🤖 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/server/server.go` around lines 458 - 460, Update the
plugin initialization loop in the server setup to track successfully initialized
plugins and, when a later plugin’s Init fails, shut down all previously
initialized plugins before returning the initialization error. Reuse each
plugin’s existing cleanup or shutdown API, preserve reverse initialization order
if supported, and keep the current wrapped error from the failing plugin.
Comment on lines
+473
to
+477
| // Declared public paths are appended before the auth middleware is built | ||
| // below, so the skip-path list is complete when the chain is assembled. | ||
| if sp, ok := p.(plugin.AuthSkipPathProvider); ok { | ||
| cfg.Auth.SkipPaths = append(cfg.Auth.SkipPaths, sp.AuthSkipPaths()...) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline platform-api/internal/middleware/auth.go --items all
rg -n -C3 'SkipPaths|HasPrefix|URL\.Path|EscapedPath|path\.Clean' platform-api/internal/middlewareRepository: wso2/api-platform
Length of output: 6946
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
# Inspect the auth middleware skip-path comparison and surrounding normalization imports/usages.
sed -n '1,150p' platform-api/internal/middleware/auth.go
# Search all middleware files for path normalization helpers and path-based comparisons.
rg -n '\b(HasPrefix|TrimPrefix|TrimSuffix|path\.Clean|EscapedPath|URL\.Path|TrimRight|path\.)\b' platform-api/internal/middleware platform-api/internal/serverRepository: wso2/api-platform
Length of output: 6205
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
# Behavioral probe for Go’s path HasPrefix semantics over the same inputs used by the middleware check.
# This does not run repository code; it matches how strings.HasPrefix handles raw path prefixes.
cat >/tmp/skip_prefix_probe.go <<'EOF'
package main
import (
"fmt"
"strings"
)
type skip = []string
func bypass(providedSkip string, requestURLPath string, skipList skip) bool {
for _, path := range skipList {
if strings.HasPrefix(requestURLPath, path) {
return true
}
}
return false
}
func main() {
cases := []struct {
name string
skipList skip
requestPath string
}{
{"legitimate exact", nil, "/api/public/a"},
{"raw prefix match", skip{"/api/public"}, "/api/public-a"},
{"malicious dotdot bypass", skip{"/api/public"}, "/api/public/../secret"},
{"malicious encoded traversal", skip{"/api/public"}, "/api/public/%2e%2e/secret"},
{"canonical segment looks safe", skip{"/api/public"}, "/api/public%2f.."},
}
skipList := skip{"/api/public"}
for _, c := range cases {
list := c.skipList
if list == nil {
list = skipList
}
fmt.Printf("%s: skip=%q request=%q skip=%v bypass=%v\n", c.name, list[0], c.requestPath, list, bypass(list[0], c.requestPath, list))
}
}
EOF
go run /tmp/skip_prefix_probe.goRepository: wso2/api-platform
Length of output: 668
Normalize paths before matching skip paths.
LocalJWTAuthMiddleware currently compares config.SkipPaths against the raw r.URL.Path with strings.HasPrefix, so configured prefix skips are not normalized and malformed request paths such as /api/public/../secret can bypass authentication. Canonicalize skip paths and normalize EscapedPath (rejecting forbidden sequences first), then compare using a path-aware prefix check that respects path segments.
🤖 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/server/server.go` around lines 473 - 477, Update
LocalJWTAuthMiddleware to validate and normalize the request’s EscapedPath
before matching, rejecting forbidden traversal or malformed sequences before
authentication bypass evaluation. Canonicalize each configured config.SkipPaths
entry and replace the raw strings.HasPrefix comparison with a path-aware prefix
check that only matches complete path segments, preserving valid public-path
skips.
Source: Coding guidelines
Comment on lines
+38
to
+45
| type Deps struct { | ||
| Gateways Gateways | ||
| // add more capability groups as external plugins need them | ||
| // (APIs, Subscriptions, Applications, Projects, Organizations, LLM, MCP, …) | ||
|
|
||
| Config *config.Server | ||
| Logger *slog.Logger | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not expose the mutable server configuration to external plugins.
StartPlatformAPIServer passes the same *pdk.Deps to every external plugin. Exported fields plus Config *config.Server allow one plugin to mutate authentication, database, or listener configuration—or replace capabilities for later plugins. Pass immutable, narrowly scoped configuration values/interfaces instead of the live server config.
The server wiring confirms that this dependency object is shared across external plugins.
🤖 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/pdk/deps.go` around lines 38 - 45, Update Deps and the
StartPlatformAPIServer plugin wiring to remove the live Config *config.Server
from the shared dependency object. Expose only immutable, narrowly scoped
configuration values or interfaces required by plugins, and ensure shared
capability fields cannot be replaced or used to mutate server configuration
while preserving the existing plugin functionality.
Comment on lines
+47
to
+64
| // Gateways exposes CRUD access to the platform's gateways, scoped by organization. | ||
| // Every method mirrors an existing GatewayService method verbatim and takes the | ||
| // organization id explicitly — handlers MUST pass the org resolved from the | ||
| // request context, never one from request input (GO-AUTH-005). | ||
| type Gateways interface { | ||
| // RegisterGateway creates a gateway in an organization (Create). | ||
| RegisterGateway(orgID string, id *string, displayName, description string, endpoints []string, | ||
| isCritical bool, functionalityType, version, createdBy string, properties map[string]any) (*api.GatewayResponse, error) | ||
|
|
||
| // GetGateway returns a single gateway by id within an organization (Read). | ||
| GetGateway(gatewayID, orgID string) (*api.GatewayResponse, error) | ||
|
|
||
| // UpdateGateway updates a gateway within an organization (Update). | ||
| UpdateGateway(gatewayID, orgID, updatedBy string, req *api.GatewayResponse) (*api.GatewayResponse, error) | ||
|
|
||
| // DeleteGateway removes a gateway within an organization (Delete). | ||
| DeleteGateway(gatewayID, orgID, deletedBy string) error | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)deps\.go$|(^|/)middleware/auth\.go$|(^|/)plugin\.go$|platform-api/pdk' | sed -n '1,120p'
echo "== relevant outlines =="
for f in platform-api/pdk/deps.go platform-api/pdk/middleware/auth.go platform-api/pdk/plugin.go platform-api/internal/plugin/plugin.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" --view compact 2>/dev/null | sed -n '1,160p' || true
fi
done
echo "== deps.go relevant =="
sed -n '1,140p' platform-api/pdk/deps.go
echo "== middleware auth relevant =="
sed -n '1,100p' platform-api/pdk/middleware/auth.go
echo "== pdk plugin relevant =="
sed -n '1,120p' platform-api/pdk/plugin.go
echo "== internal plugin relevant =="
sed -n '1,150p' platform-api/internal/plugin/plugin.goRepository: wso2/api-platform
Length of output: 13776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal middleware auth =="
sed -n '1,220p' platform-api/internal/middleware/auth.go
echo "== auth skip path usage =="
rg -n "AuthSkipPaths|SkipPaths|GetOrganizationFromRequest|organization" platform-api/internal platform-api/pdk platform-api/plugins -S | sed -n '1,240p'
echo "== service gateway methods =="
rg -n "type GatewayService|func \(.*GatewayService.*\).*(Register|Get|Update|Delete)Gateway|Create|Read|Update|Delete" platform-api/internal/service -S | sed -n '1,220p'
echo "== gateway service implementation slices =="
for f in $(rg -l "type GatewayService|func \(.*GatewayService.*\).*(Register|Get|Update|Delete)Gateway" platform-api/internal/service -S); do
echo "--- $f ---"
wc -l "$f"
if [ $(wc -l < "$f") -le 260 ]; then
sed -n '1,260p' "$f"
else
rg -n -C 20 "func \(.*GatewayService.*\).*(Register|Get|Update|Delete)Gateway" "$f" | sed -n '1,260p'
fi
done
echo "== pdk Deps assignment =="
rg -n "Deps\b|Gateways\b|pdk.Deps|Server\(" platform-api -S | sed -n '1,240p'Repository: wso2/api-platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gateway service relevant methods =="
sed -n '540,790p' platform-api/internal/service/gateway.go
echo "== gateway repo relevant =="
rg -n -C 12 "func .*Create\\(|func .*GetByUUID|func .*UpdateGateway\\(|func .*Delete \\(|HasGateway|organization_uuid|organizationUUID" platform-api/internal/repository/gateway.go platform -S --glob '*.go' | sed -n '1,220p'
echo "== pdk deps assignment =="
rg -n "pdk\.Deps\{|Gateways:\s+.*GatewayService|&GatewayService\{|Deps: \{|\*GatewayService" platform-api -SRepository: wso2/api-platform
Length of output: 26530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pdk deps assignment =="
rg -n "pdk\.Deps\{|Gateways:\s+\*service\.GatewayService|GatewayService\{|Deps: \{|\*GatewayService" platform-api -S
echo "== all AddAuthSkipPath/Plugin.AuthSkipPaths wiring =="
rg -n -C 8 "AddAuthSkipPath|AuthSkipPaths|SkipPaths" platform-api -SRepository: wso2/api-platform
Length of output: 26112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server pdk deps creation =="
sed -n '400,455p' platform-api/internal/server/server.go
echo "== server middleware auth build =="
sed -n '540,710p' platform-api/internal/server/server.goRepository: wso2/api-platform
Length of output: 9278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== platform client uses for pdk AuthSkipPathProvider =="
rg -n -C 6 "implements|pdk\.Plugin|AuthSkipPaths\(\).*\[\]string|func \\(.*\\) AuthSkipPaths" . -S --glob '*.go' --glob '!**/*test.go' | sed -n '1,220p'Repository: wso2/api-platform
Length of output: 18368
Derive tenant context at the pdk capability boundary.
The pdk contract currently lets plugins pass any orgID into pdk.Gateways, and external AuthSkipPaths is appended to the server skip-list before auth is built, both relying on callers/plugins to stay in tenant scope. Tie these operations to the request organization claim and keep bypass prefixes under platform/admin control.
📍 Affects 2 files
platform-api/pdk/deps.go#L47-L64(this comment)platform-api/pdk/middleware/auth.go#L30-L40
🤖 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/pdk/deps.go` around lines 47 - 64, Update
platform-api/pdk/deps.go lines 47-64 so the Gateways capability derives
organization scope from the request context/organization claim instead of
accepting caller-supplied orgID parameters, while preserving the CRUD
operations. Update platform-api/pdk/middleware/auth.go lines 30-40 so external
AuthSkipPaths are not appended to the server-managed authentication skip-list;
keep bypass prefixes controlled by the platform/admin configuration.
Source: Coding guidelines
Comment on lines
+59
to
+65
| // AuthSkipPathProvider is an optional interface a Plugin may also implement to | ||
| // declare public (unauthenticated) path prefixes. The server appends them to the | ||
| // auth skip-path list before the auth middleware is built. Keep prefixes narrow | ||
| // and specific — this is an auth-bypass surface (GO-AUTH-004). | ||
| type AuthSkipPathProvider interface { | ||
| AuthSkipPaths() []string | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Centralize validation of plugin authentication bypass paths.
Both plugin tiers can return arbitrary path prefixes, which the server adds directly to its authentication skip list. A broad or malformed prefix can bypass authentication for protected endpoints.
platform-api/pdk/plugin.go#L59-L65: constrain external plugin paths to a validated, route-bound allowlist.platform-api/internal/plugin/plugin.go#L100-L109: apply the identical validation to internal plugin paths and fail closed on invalid declarations.
As per coding guidelines, request paths must be normalized before authentication decisions and raw prefix matching must not permit authorization bypasses.
📍 Affects 2 files
platform-api/pdk/plugin.go#L59-L65(this comment)platform-api/internal/plugin/plugin.go#L100-L109
🤖 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/pdk/plugin.go` around lines 59 - 65, Centralize validation for
authentication skip paths and apply it to both plugin tiers: update
AuthSkipPathProvider handling in platform-api/pdk/plugin.go (lines 59-65) and
the internal plugin path handling in platform-api/internal/plugin/plugin.go
(lines 100-109) to accept only normalized, route-bound allowlisted prefixes.
Reject malformed or overly broad declarations and fail closed before adding
paths to the authentication skip list; ensure authentication decisions use
normalized request paths rather than unsafe raw prefix matching.
Source: Coding guidelines
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
1 participant
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.
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment