fix(plugins): resolve gateway auth against active http route registry#48769
fix(plugins): resolve gateway auth against active http route registry#48769MarsDoge wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a bug where gateway auth enforcement for plugin HTTP routes was evaluated against a stale, caller-supplied Key changes:
Minor concern: The new auth-check test ( Confidence Score: 4/5
Prompt To Fix All With AIThis 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 |
| 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(); | ||
| } | ||
| }); |
There was a problem hiding this 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:
| 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.There was a problem hiding this comment.
💡 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); |
There was a problem hiding this comment.
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 👍 / 👎.
Summary
Change Type
Scope
User-visible / Behavior Changes
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"src/gateway/server/plugins-http.test.tsfile in this clean clone still hits an existing unrelated failure incaps unauthenticated plugin routes to non-admin subagent scopes(Expected subagent runtime from loadGatewayPlugins)Closes #48734