fix: centralize plugin command auth requirements#55395
Open
fix: centralize plugin command auth requirements#55395
Conversation
Contributor
Greptile SummaryThis PR centralizes plugin command authorization by moving owner and gateway-scope enforcement out of individual plugin handlers and into the shared Key observations:
Confidence Score: 4/5Safe to merge; the security hardening is correct and narrows access, with no regressions in existing test coverage. The core authorization logic is sound, the PR correctly removes ad-hoc checks and centralizes enforcement, and the new tests cover the primary dispatcher paths. The three remaining items are all P2: an unguarded throw from resolveRequiredGatewayScopes, an unsafe type cast for gatewayClientScopes, and a missing automated test for the admin-scope bypass that the PR explicitly calls out as a manually verified edge case. src/plugins/commands.ts (unguarded resolveRequiredGatewayScopes throw), src/auto-reply/reply/commands-plugin.ts (type cast), src/plugins/commands.test.ts (missing admin-scope bypass test)
|
| Filename | Overview |
|---|---|
| src/plugins/commands.ts | Adds centralized owner and gateway scope enforcement to executePluginCommand; logic is correct (admin scope bypass works per-scope, external callers skip scope check); resolveRequiredGatewayScopes callback not wrapped in try/catch before the handler's existing error boundary. |
| src/plugins/types.ts | Adds surface, senderIsOwner, typed OperatorScope[] for gatewayClientScopes, PluginCommandAuthorizationContext, and new command definition fields; all additions are additive and backward-compatible. |
| src/plugins/command-registration.ts | Validates requiredGatewayScopes against the known OperatorScope set; resolveRequiredGatewayScopes is type-checked but its runtime return values are not validated at registration time. |
| src/auto-reply/reply/commands-plugin.ts | Now threads surface and senderIsOwner into executePluginCommand; uses an unsafe as-cast for gatewayClientScopes rather than a narrowing filter. |
| src/plugins/commands.test.ts | Good seam-level coverage for new dispatcher auth paths; missing an automated test for the admin-scope bypass explicitly called out as manually verified. |
| extensions/device-pair/index.ts | Inline scope check removed and replaced with declarative resolveRequiredGatewayScopes; the approve-action scope requirement is preserved correctly. |
| extensions/device-pair/index.test.ts | Approve-path tests updated to exercise executePluginCommand directly, correctly locking in centralized enforcement behavior. |
| extensions/phone-control/index.test.ts | Minimal update to add surface and senderIsOwner to the test command context factory; no functional changes. |
| extensions/talk-voice/index.test.ts | Tightens gatewayClientScopes parameter type from string[] to OperatorScope[]; adds surface and senderIsOwner to command context; no behavioral changes. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/plugins/commands.ts
Line: 265
Comment:
**`resolveRequiredGatewayScopes` throw not caught**
`resolveRequiredGatewayScopes?.(authContext)` is spread inline without any error handling. If a plugin's callback throws, the exception propagates out of `executePluginCommand` before it can be caught by the try/catch that wraps `command.handler` (lines 343–356). The caller in `commands-plugin.ts` does not have its own try/catch either, so an unexpected throw here would bubble up unhandled and could crash the request entirely rather than returning a safe error reply.
Consider wrapping the dynamic resolution in a try/catch:
```
let dynamicScopes: readonly OperatorScope[] = [];
try {
dynamicScopes = command.resolveRequiredGatewayScopes?.(authContext) ?? [];
} catch (err) {
logVerbose(`Plugin command /${command.name} resolveRequiredGatewayScopes threw: ${(err as Error).message}`);
return { text: "⚠️ Command failed. Please try again later." };
}
const requiredGatewayScopes = Array.from(
new Set([...(command.requiredGatewayScopes ?? []), ...dynamicScopes]),
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-plugin.ts
Line: 42-44
Comment:
**Unsafe `as` cast for `gatewayClientScopes`**
`params.ctx.GatewayClientScopes` is cast directly to `OperatorScope[] | undefined` without any filtering or validation. If the upstream gateway ever populates this field with a scope string that is not a member of `OperatorScope` (e.g., a future scope added to the gateway but not yet to `method-scopes.ts`), the cast silently hides the mismatch. The runtime effect is safe-fail (the unknown scope simply won't match any requirement), but it makes the mismatch invisible in both compile-time output and logs, which can make debugging difficult.
A narrowing filter would make the intent explicit and self-documenting:
```
const KNOWN_OPERATOR_SCOPES = new Set<string>([
ADMIN_SCOPE, READ_SCOPE, WRITE_SCOPE, APPROVALS_SCOPE, PAIRING_SCOPE,
]);
gatewayClientScopes: (params.ctx.GatewayClientScopes ?? []).filter(
(s): s is OperatorScope => KNOWN_OPERATOR_SCOPES.has(s),
),
```
(Or extract a shared `isOperatorScope` type-guard into `method-scopes.ts` so it can be reused.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/plugins/commands.test.ts
Line: 342-462
Comment:
**Missing test for `operator.admin` scope bypass**
The PR description and the human-verification section both explicitly call out that "admin scope satisfies narrower operator scope requirements" as an edge case that was verified manually. However, there is no automated test that locks this in at the dispatcher seam. Given that this bypass is the mechanism by which CLI/admin callers retain access after the centralized auth change, a missing regression test here is a notable gap — especially since the test file already has dedicated coverage for the analogous "external callers bypass scope requirements" case.
Consider adding a case alongside the existing gateway scope tests:
```ts
it("allows admin-scoped internal callers to bypass narrower scope requirements", async () => {
const handler = vi.fn(async () => ({ text: "ok" }));
const result = await executePluginCommand({
command: {
name: "pairing",
description: "Pairing command",
requiredGatewayScopes: ["operator.pairing"],
handler,
pluginId: "demo-plugin",
},
surface: "webchat",
channel: "webchat",
senderId: "admin-1",
isAuthorizedSender: true,
gatewayClientScopes: ["operator.admin"],
commandBody: "/pairing",
config: {} as never,
});
expect(handler).toHaveBeenCalledTimes(1);
expect(result).toEqual({ text: "ok" });
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Plugins: centralize plugin command auth ..." | Re-trigger Greptile
Comment on lines
+342
to
+462
| config: {} as never, | ||
| }); | ||
|
|
||
| expect(handler).not.toHaveBeenCalled(); | ||
| expect(result).toEqual({ text: "⚠️ This command requires owner authorization." }); | ||
| }); | ||
|
|
||
| it("blocks internal callers missing required gateway scopes", async () => { | ||
| const handler = vi.fn(async () => ({ text: "ok" })); | ||
|
|
||
| const result = await executePluginCommand({ | ||
| command: { | ||
| name: "pairing", | ||
| description: "Pairing command", | ||
| requiredGatewayScopes: ["operator.pairing"], | ||
| handler, | ||
| pluginId: "demo-plugin", | ||
| }, | ||
| surface: "webchat", | ||
| channel: "webchat", | ||
| senderId: "writer-1", | ||
| isAuthorizedSender: true, | ||
| gatewayClientScopes: ["operator.write"], | ||
| commandBody: "/pairing", | ||
| config: {} as never, | ||
| }); | ||
|
|
||
| expect(handler).not.toHaveBeenCalled(); | ||
| expect(result).toEqual({ | ||
| text: "⚠️ This command requires operator.pairing for internal gateway callers.", | ||
| }); | ||
| }); | ||
|
|
||
| it("allows external callers to bypass gateway scope requirements", async () => { | ||
| const handler = vi.fn(async () => ({ text: "ok" })); | ||
|
|
||
| const result = await executePluginCommand({ | ||
| command: { | ||
| name: "pairing", | ||
| description: "Pairing command", | ||
| requiredGatewayScopes: ["operator.pairing"], | ||
| handler, | ||
| pluginId: "demo-plugin", | ||
| }, | ||
| surface: "telegram", | ||
| channel: "telegram", | ||
| senderId: "123", | ||
| isAuthorizedSender: true, | ||
| commandBody: "/pairing", | ||
| config: {} as never, | ||
| }); | ||
|
|
||
| expect(handler).toHaveBeenCalledTimes(1); | ||
| expect(result).toEqual({ text: "ok" }); | ||
| }); | ||
|
|
||
| it("supports context-sensitive gateway scope requirements", async () => { | ||
| const handler = vi.fn(async () => ({ text: "ok" })); | ||
| const command: Parameters<typeof executePluginCommand>[0]["command"] = { | ||
| name: "pair", | ||
| description: "Pair command", | ||
| resolveRequiredGatewayScopes: (ctx) => { | ||
| const action = ctx.args?.trim().split(/\s+/, 1)[0]?.toLowerCase(); | ||
| return action === "approve" ? ["operator.pairing"] : undefined; | ||
| }, | ||
| handler, | ||
| pluginId: "demo-plugin", | ||
| }; | ||
|
|
||
| const denied = await executePluginCommand({ | ||
| command, | ||
| surface: "webchat", | ||
| channel: "webchat", | ||
| senderId: "writer-1", | ||
| isAuthorizedSender: true, | ||
| gatewayClientScopes: ["operator.write"], | ||
| args: "approve latest", | ||
| commandBody: "/pair approve latest", | ||
| config: {} as never, | ||
| }); | ||
|
|
||
| const allowed = await executePluginCommand({ | ||
| command, | ||
| surface: "webchat", | ||
| channel: "webchat", | ||
| senderId: "writer-1", | ||
| isAuthorizedSender: true, | ||
| gatewayClientScopes: ["operator.write"], | ||
| args: "qr", | ||
| commandBody: "/pair qr", | ||
| config: {} as never, | ||
| }); | ||
|
|
||
| expect(handler).toHaveBeenCalledTimes(1); | ||
| expect(denied).toEqual({ | ||
| text: "⚠️ This command requires operator.pairing for internal gateway callers.", | ||
| }); | ||
| expect(allowed).toEqual({ text: "ok" }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
operator.admin scope bypass
The PR description and the human-verification section both explicitly call out that "admin scope satisfies narrower operator scope requirements" as an edge case that was verified manually. However, there is no automated test that locks this in at the dispatcher seam. Given that this bypass is the mechanism by which CLI/admin callers retain access after the centralized auth change, a missing regression test here is a notable gap — especially since the test file already has dedicated coverage for the analogous "external callers bypass scope requirements" case.
Consider adding a case alongside the existing gateway scope tests:
it("allows admin-scoped internal callers to bypass narrower scope requirements", async () => {
const handler = vi.fn(async () => ({ text: "ok" }));
const result = await executePluginCommand({
command: {
name: "pairing",
description: "Pairing command",
requiredGatewayScopes: ["operator.pairing"],
handler,
pluginId: "demo-plugin",
},
surface: "webchat",
channel: "webchat",
senderId: "admin-1",
isAuthorizedSender: true,
gatewayClientScopes: ["operator.admin"],
commandBody: "/pairing",
config: {} as never,
});
expect(handler).toHaveBeenCalledTimes(1);
expect(result).toEqual({ text: "ok" });
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/commands.test.ts
Line: 342-462
Comment:
**Missing test for `operator.admin` scope bypass**
The PR description and the human-verification section both explicitly call out that "admin scope satisfies narrower operator scope requirements" as an edge case that was verified manually. However, there is no automated test that locks this in at the dispatcher seam. Given that this bypass is the mechanism by which CLI/admin callers retain access after the centralized auth change, a missing regression test here is a notable gap — especially since the test file already has dedicated coverage for the analogous "external callers bypass scope requirements" case.
Consider adding a case alongside the existing gateway scope tests:
```ts
it("allows admin-scoped internal callers to bypass narrower scope requirements", async () => {
const handler = vi.fn(async () => ({ text: "ok" }));
const result = await executePluginCommand({
command: {
name: "pairing",
description: "Pairing command",
requiredGatewayScopes: ["operator.pairing"],
handler,
pluginId: "demo-plugin",
},
surface: "webchat",
channel: "webchat",
senderId: "admin-1",
isAuthorizedSender: true,
gatewayClientScopes: ["operator.admin"],
commandBody: "/pairing",
config: {} as never,
});
expect(handler).toHaveBeenCalledTimes(1);
expect(result).toEqual({ text: "ok" });
});
```
How can I resolve this? If you propose a fix, please make it concise.ac56bc7 to
fbe3942
Compare
Move plugin command authorization toward the GHSA's long-term model by preserving richer auth context, supporting declarative owner and gateway scope requirements, and enforcing them in the shared dispatcher. Convert `/pair approve` to use the centralized requirement path and add regression coverage for dispatcher-level auth behavior. Regeneration-Prompt: | This follow-up hardening is for the plugin command auth gap described in GHSA-9gwp-pxfh-w6r5. The immediate exploit path was already fixed by plumbing gateway scopes into the device-pair plugin and checking `/pair approve` inline, but the longer-term goal is to stop relying on lossy, plugin-specific auth checks. Preserve the existing plugin command flow and keep the change additive. Carry richer authorization context into plugin execution, including owner status and command surface, and let commands declare owner or internal gateway-scope requirements that the central dispatcher enforces. Internal callers should fail closed when required scopes are missing, with admin scope still satisfying narrower operator requirements, while non-internal chat surfaces should keep their current auth behavior. Because `/pair` mixes low-risk actions like `qr` and `status` with the privileged `approve` action, use a context-sensitive requirement instead of making the whole command require pairing scope. Add focused regression tests around dispatcher enforcement and update any command-context test helpers that now need the richer fields.
Update the generated Plugin SDK API baseline files after extending plugin command types for centralized owner and gateway-scope authorization. Regeneration-Prompt: | The prior commit intentionally changed exported plugin SDK types in `src/plugins/types.ts` by adding richer plugin command auth context and declarative command requirement fields. CI reported plugin SDK API drift, which means the generated baseline files under `docs/.generated/` no longer matched the exported surface. Regenerate only the plugin SDK API baseline artifacts with the repo's standard generator, verify `pnpm plugin-sdk:api:check` passes, and keep this follow-up scoped to those generated files. Do not fold in unrelated failing tests from untouched surfaces.
Catch dynamic gateway-scope resolver failures in the dispatcher, narrow forwarded gateway scope strings with an explicit operator-scope guard, add regression coverage for admin bypass and resolver-throw behavior, and refresh bundled plugin metadata after main-branch drift. Regeneration-Prompt: | Follow up on review feedback for the centralized plugin command auth change. Keep the scope tightly limited to the three review items: catch exceptions from `resolveRequiredGatewayScopes`, replace the raw `GatewayClientScopes` cast with explicit operator-scope narrowing, and add dispatcher-level tests for the `operator.admin` bypass plus the safe failure path when dynamic scope resolution throws. While landing that patch, the repo hook may report stale bundled plugin metadata generated files because main advanced. Regenerate those standard outputs with the repo generator so the branch is consistent enough to rebase, but do not chase unrelated CI or Discord test failures here.
fbe3942 to
c94f10b
Compare
25 tasks
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.
Summary
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
isAuthorizedSender, which was not rich enough to express owner-only or internal scope-based requirements in the shared dispatcher.git blame, prior PR, issue, or refactor if known): the immediate device-pair fix added scope plumbing and an inline/pair approvecheck, but left the broader plugin command model unchanged.Regression Test Plan (if applicable)
src/plugins/commands.test.ts,extensions/device-pair/index.test.tsUser-visible / Behavior Changes
Internal callers now get centralized authorization failures for plugin commands that declare required owner or gateway scopes.
/pair approvestill requires the same privileged access, but that requirement is now enforced by shared plugin command auth instead of an inline plugin-only check.Diagram (if applicable)
Security Impact (required)
No)No)No)Yes)Yes)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
/pair approveand non-privileged/pairactions through the plugin command path.Expected
/pairactions continue to work without requiring pairing scope.Actual
Evidence
Attach at least one:
Human Verification (required)
pnpm tsgo,pnpm build,pnpm check, and fullpnpm test; focused regression tests for plugin command dispatcher auth and device-pair auth behavior./pairactions do not over-restrict non-privileged subcommands.Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations