Skip to content

fix(plugins): resolve gateway auth against active http route registry#48769

Open
MarsDoge wants to merge 1 commit intoopenclaw:mainfrom
MarsDoge:fix/issue-48734-plugin-http-active-registry
Open

fix(plugins): resolve gateway auth against active http route registry#48769
MarsDoge wants to merge 1 commit intoopenclaw:mainfrom
MarsDoge:fix/issue-48734-plugin-http-active-registry

Conversation

@MarsDoge
Copy link
Copy Markdown

Summary

  • resolve plugin webhook auth checks against the active HTTP route registry instead of a stale captured registry
  • keep the provided registry as fallback when the active route registry has no routes yet
  • add a regression test covering stale explicit registries after later plugin route registration

Change Type

  • bug fix
  • test coverage

Scope

  • gateway plugin HTTP route auth enforcement
  • no behavior changes outside plugin-owned HTTP route matching/auth

User-visible / Behavior Changes

  • plugin webhook routes registered after startup no longer fall through auth/path checks because of a stale registry capture
  • gateway auth enforcement now follows the same active route registry resolution already used by request dispatch

Test plan

  • pnpm exec vitest run --config vitest.gateway.config.ts src/gateway/server/plugins-http.test.ts -t "plugin HTTP route auth checks|prefers the pinned route registry over a stale explicit registry|handles routes registered into the pinned startup registry after the active registry changes"
  • Note: running the full src/gateway/server/plugins-http.test.ts file in this clean clone still hits an existing unrelated failure in caps unauthenticated plugin routes to non-admin subagent scopes (Expected subagent runtime from loadGatewayPlugins)

Closes #48734

@MarsDoge MarsDoge requested a review from a team as a code owner March 17, 2026 05:58
@openclaw-barnacle openclaw-barnacle bot added gateway Gateway runtime size: XS labels Mar 17, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes a bug where gateway auth enforcement for plugin HTTP routes was evaluated against a stale, caller-supplied PluginRegistry rather than the live active route registry, causing routes registered after startup to bypass auth checks. The fix in route-auth.ts routes shouldEnforceGatewayAuthForPluginPath through the existing resolveActivePluginHttpRouteRegistry helper (already used by the request dispatcher), with a correct fallback to the provided registry when the active one has no routes yet.

Key changes:

  • route-auth.ts: shouldEnforceGatewayAuthForPluginPath now calls resolveActivePluginHttpRouteRegistry(registry) before route matching, mirroring the resolution strategy used during request dispatch.
  • plugins-http.test.ts: Adds two regression tests — one for the request handler path and one for the auth-check path — covering the scenario where a route is registered into the pinned startup registry after a stale explicit registry was captured.

Minor concern: The new auth-check test ("prefers the active route registry for auth checks when the explicit registry is stale") modifies shared global state (setActivePluginRegistry, pinActivePluginHttpRouteRegistry) but does not clean it up in its finally block. The describe("createGatewayPluginRequestHandler") suite has a guarding afterEach, but describe("plugin HTTP route auth checks") does not. This is safe today (the test is last in its suite and the fallback logic compensates), but adds fragility for future tests in that block.

Confidence Score: 4/5

  • Safe to merge; the core logic change is correct and minimal, with a single low-severity test-hygiene concern.
  • The production code change is a focused one-liner that correctly delegates registry resolution to the existing resolveActivePluginHttpRouteRegistry helper, which already handles the active-vs-fallback logic. The new tests exercise the fix and the fallback. The only issue is the missing global-state cleanup in the new auth-check test, which is a P2 style concern with no current runtime impact.
  • src/gateway/server/plugins-http.test.ts — the new auth-check test leaves pinned registry state unreleased after execution.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/server/plugins-http.test.ts
Line: 465-490

Comment:
**Missing global state cleanup after test**

This test calls `setActivePluginRegistry` and `pinActivePluginHttpRouteRegistry`, modifying shared global state, but the `finally` block only unregiters the route — the pinned registry and active registry are never restored. While the `describe("createGatewayPluginRequestHandler")` block has a dedicated `afterEach` for this, the `describe("plugin HTTP route auth checks")` block has no equivalent.

In practice this is currently safe because this test is last in its suite and the fallback logic in `resolveActivePluginHttpRouteRegistry` compensates when the active registry is empty. However, if tests are reordered or new tests are added to this suite after this one, the leaked `httpRouteRegistryPinned = true` + a stale `startupRegistry` could cause subtle failures (e.g. a test that passes a non-empty local registry would still fall back to it, but a test that expects no routes in the active registry would not).

Consider adding cleanup to the `finally` block or adding an `afterEach` to this describe block:

```suggestion
  it("prefers the active route registry for auth checks when the explicit registry is stale", () => {
    const startupRegistry = createTestRegistry();
    const staleExplicitRegistry = createTestRegistry({
      httpRoutes: [createRoute({ path: "/plugins/diffs", auth: "plugin" })],
    });

    setActivePluginRegistry(createTestRegistry());
    pinActivePluginHttpRouteRegistry(startupRegistry);

    const unregister = registerPluginHttpRoute({
      path: "/bluebubbles-webhook",
      auth: "gateway",
      handler: () => true,
    });

    try {
      expect(
        shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/bluebubbles-webhook"),
      ).toBe(true);
      expect(shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/plugins/diffs")).toBe(
        false,
      );
    } finally {
      unregister();
      releasePinnedPluginHttpRouteRegistry();
      setActivePluginRegistry(createEmptyPluginRegistry());
    }
  });
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: ec92054

Comment on lines +465 to +490
it("prefers the active route registry for auth checks when the explicit registry is stale", () => {
const startupRegistry = createTestRegistry();
const staleExplicitRegistry = createTestRegistry({
httpRoutes: [createRoute({ path: "/plugins/diffs", auth: "plugin" })],
});

setActivePluginRegistry(createTestRegistry());
pinActivePluginHttpRouteRegistry(startupRegistry);

const unregister = registerPluginHttpRoute({
path: "/bluebubbles-webhook",
auth: "gateway",
handler: () => true,
});

try {
expect(
shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/bluebubbles-webhook"),
).toBe(true);
expect(shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/plugins/diffs")).toBe(
false,
);
} finally {
unregister();
}
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing global state cleanup after test

This test calls setActivePluginRegistry and pinActivePluginHttpRouteRegistry, modifying shared global state, but the finally block only unregiters the route — the pinned registry and active registry are never restored. While the describe("createGatewayPluginRequestHandler") block has a dedicated afterEach for this, the describe("plugin HTTP route auth checks") block has no equivalent.

In practice this is currently safe because this test is last in its suite and the fallback logic in resolveActivePluginHttpRouteRegistry compensates when the active registry is empty. However, if tests are reordered or new tests are added to this suite after this one, the leaked httpRouteRegistryPinned = true + a stale startupRegistry could cause subtle failures (e.g. a test that passes a non-empty local registry would still fall back to it, but a test that expects no routes in the active registry would not).

Consider adding cleanup to the finally block or adding an afterEach to this describe block:

Suggested change
it("prefers the active route registry for auth checks when the explicit registry is stale", () => {
const startupRegistry = createTestRegistry();
const staleExplicitRegistry = createTestRegistry({
httpRoutes: [createRoute({ path: "/plugins/diffs", auth: "plugin" })],
});
setActivePluginRegistry(createTestRegistry());
pinActivePluginHttpRouteRegistry(startupRegistry);
const unregister = registerPluginHttpRoute({
path: "/bluebubbles-webhook",
auth: "gateway",
handler: () => true,
});
try {
expect(
shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/bluebubbles-webhook"),
).toBe(true);
expect(shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/plugins/diffs")).toBe(
false,
);
} finally {
unregister();
}
});
it("prefers the active route registry for auth checks when the explicit registry is stale", () => {
const startupRegistry = createTestRegistry();
const staleExplicitRegistry = createTestRegistry({
httpRoutes: [createRoute({ path: "/plugins/diffs", auth: "plugin" })],
});
setActivePluginRegistry(createTestRegistry());
pinActivePluginHttpRouteRegistry(startupRegistry);
const unregister = registerPluginHttpRoute({
path: "/bluebubbles-webhook",
auth: "gateway",
handler: () => true,
});
try {
expect(
shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/bluebubbles-webhook"),
).toBe(true);
expect(shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/plugins/diffs")).toBe(
false,
);
} finally {
unregister();
releasePinnedPluginHttpRouteRegistry();
setActivePluginRegistry(createEmptyPluginRegistry());
}
});
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server/plugins-http.test.ts
Line: 465-490

Comment:
**Missing global state cleanup after test**

This test calls `setActivePluginRegistry` and `pinActivePluginHttpRouteRegistry`, modifying shared global state, but the `finally` block only unregiters the route — the pinned registry and active registry are never restored. While the `describe("createGatewayPluginRequestHandler")` block has a dedicated `afterEach` for this, the `describe("plugin HTTP route auth checks")` block has no equivalent.

In practice this is currently safe because this test is last in its suite and the fallback logic in `resolveActivePluginHttpRouteRegistry` compensates when the active registry is empty. However, if tests are reordered or new tests are added to this suite after this one, the leaked `httpRouteRegistryPinned = true` + a stale `startupRegistry` could cause subtle failures (e.g. a test that passes a non-empty local registry would still fall back to it, but a test that expects no routes in the active registry would not).

Consider adding cleanup to the `finally` block or adding an `afterEach` to this describe block:

```suggestion
  it("prefers the active route registry for auth checks when the explicit registry is stale", () => {
    const startupRegistry = createTestRegistry();
    const staleExplicitRegistry = createTestRegistry({
      httpRoutes: [createRoute({ path: "/plugins/diffs", auth: "plugin" })],
    });

    setActivePluginRegistry(createTestRegistry());
    pinActivePluginHttpRouteRegistry(startupRegistry);

    const unregister = registerPluginHttpRoute({
      path: "/bluebubbles-webhook",
      auth: "gateway",
      handler: () => true,
    });

    try {
      expect(
        shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/bluebubbles-webhook"),
      ).toBe(true);
      expect(shouldEnforceGatewayAuthForPluginPath(staleExplicitRegistry, "/plugins/diffs")).toBe(
        false,
      );
    } finally {
      unregister();
      releasePinnedPluginHttpRouteRegistry();
      setActivePluginRegistry(createEmptyPluginRegistry());
    }
  });
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec920546fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

});

setActivePluginRegistry(createTestRegistry());
pinActivePluginHttpRouteRegistry(startupRegistry);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Release pinned plugin HTTP registry after auth regression test

This test pins the global plugin HTTP route registry but never unpins it; the finally block only calls unregister(). Because plugin runtime state is process-global, later tests in the same worker can inherit httpRouteRegistryPinned=true, and subsequent setActivePluginRegistry(...) calls (including the shared test/setup.ts afterEach reset) will stop updating the active route registry, creating order-dependent failures in suites that rely on fresh route state.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gateway Gateway runtime size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin HTTP route dispatch uses stale registry - webhook POST returns 404

1 participant