[security] fix(approvals): require admin for approval responses

[Hinotoi-agent]

Summary

This PR hardens NanoClaw's approval-response trust boundary: a valid approval questionId is no longer enough to resolve a pending privileged approval.

• Approval responses now require the clicker/responder to be an owner, global admin, or admin for the approval's agent group before OneCLI or registered module approval handlers run.
• Responder identities are normalized to the same <channelType>:<userId> format used by the permissions tables before role checks are evaluated.
• Unauthorized clicks are ignored while the pending approval remains available for a legitimate admin response.
• Regression coverage locks in non-admin rejection and owner/global-admin success paths.

Security issues covered

Issue	Impact	Severity
Approval response handlers trusted a valid questionId before authorizing the responder identity	A non-admin who can cause or replay a valid approval response payload could approve or reject privileged actions guarded by NanoClaw's approval workflow	Medium

Before this PR

• handleApprovalsResponse() looked up a pending approval by questionId and then resolved OneCLI approvals or dispatched module approval handlers.
• The response payload carried responder context (userId, channelType, platformId, threadId), but the responder identity was not checked against owner/admin privileges before the approval decision was accepted.
• A valid approval id could therefore become the effective authorization boundary for privileged approval flows.
• There was no focused regression test proving that a non-admin clicker with a valid approval id is ignored.

After this PR

• The pending approval is loaded before any OneCLI resolver or registered module approval handler can run.
• The responder id is normalized to the same namespaced identity format used by the permissions module.
• Scoped approvals require hasAdminPrivilege(userId, agentGroupId).
• Approvals without an agent-group scope require owner/global-admin privileges.
• Unauthorized clicks return as handled input but leave the pending approval row intact for a legitimate admin.
• New tests cover unauthorized and authorized approval-response paths.

Why this matters

Approval callbacks are transport-facing input. Even when approval cards are intended for admin DMs or controlled channels, the inbound callback payload itself should not be the security decision. Privileged actions such as package/plugin/self-modification style workflows should depend on a server-side role check for the actual responder identity, not only on possession of a questionId.

How this differs from related issue/PR

PR #2308 tightened approval-card prompting and removed a ghost tool reference. This PR addresses a different boundary in the approval response handler itself: the server-side authorization check that decides whether the clicker/responder is allowed to resolve a pending approval.

Attack flow

non-admin obtains or triggers a valid approval response payload/questionId
    -> approval response handler loads the pending approval by questionId
        -> missing responder role check accepts the response
            -> registered approval handler or OneCLI approval is resolved
                -> privileged action can be approved/rejected by an unintended user

Affected code

Issue	Files
Approval responses were resolved before responder authorization	src/modules/approvals/response-handler.ts
Missing regression coverage for non-admin approval clicks	src/modules/approvals/response-handler.test.ts

Root cause

Issue: approval response authorization was keyed too narrowly to the approval id.

• Direct cause: handleApprovalsResponse() treated a matching pending approval row as sufficient to continue to the approval resolver/handler path.
• Boundary failure: the callback's responder identity was available but was not checked against the owner/admin role model before accepting the approval decision.

CVSS assessment

Issue	CVSS v3.1	Vector
Unauthorized approval response acceptance	6.5 Medium	CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:H/A:N

Rationale:

• The attacker is modeled as an already-admitted or transport-reachable low-privilege user who can submit/replay a valid approval response payload, not an unauthenticated internet attacker.
• The primary impact is integrity: accepting or rejecting privileged approval-gated actions as a non-admin.

Safe reproduction steps

1. Seed a pending approval for a privileged action such as install_packages.
2. Register an approval handler for that action.
3. Submit an approval response with the valid questionId but a non-admin responder identity.
4. Before this PR, the handler path could be reached based on the valid approval id.
5. After this PR, the response is ignored, the handler is not called, and the pending approval remains available for a real admin.
6. Submit the same approval from an owner/global-admin identity and observe the handler dispatch succeeds.

The new regression test implements this flow with a local test database and safe fake package payloads.

Expected vulnerable behavior

• A valid approval id should not authorize the clicker by itself.
• Pre-patch behavior allowed the approval response path to proceed before checking the clicker's role.
• The safe proof signal is whether the registered approval handler is invoked for a non-admin responder.

Changes in this PR

• Adds namespacedUserId() to normalize responder ids before permission checks and handler dispatch.
• Adds isAuthorizedApprovalClick() to enforce owner/global-admin or scoped admin authorization before approval resolution.
• Moves the authorization check ahead of OneCLI resolver and module approval-handler dispatch.
• Leaves unauthorized approvals pending instead of deleting them.
• Adds regression tests for:
  • non-admin response with valid approval id is ignored,
  • owner/admin response dispatches the registered handler,
  • global admin response works for approvals without a scoped agent group.

Files changed

Category	Files	What changed
Approval response authorization	src/modules/approvals/response-handler.ts	Checks responder role before resolving OneCLI or registered module approvals
Regression tests	src/modules/approvals/response-handler.test.ts	Covers unauthorized and authorized approval-response behavior

Maintainer impact

• Patch scope is limited to approval response handling and targeted tests.
• Existing owner/global-admin/scoped-admin role semantics are reused rather than introducing a separate approval-specific ACL.
• Unauthorized clicks are ignored without consuming the pending approval, reducing accidental denial-of-service against legitimate admin approval cards.
• Approval handlers now receive the normalized responder id, matching the identity format used elsewhere in the permissions layer.

Fix rationale

• The response handler is the durable server-side boundary where all approval decisions converge, so it is the right place to enforce responder authorization.
• Reusing hasAdminPrivilege, isOwner, and isGlobalAdmin keeps the fix aligned with the existing role model.
• Keeping the pending approval intact for unauthorized clicks lets legitimate admins still resolve the request.
• The tests exercise the actual DB-backed pending approval path rather than only a private helper.

Type of change

• Security fix
• Tests
• Documentation update
• Refactor with no behavior change

Test plan

• Targeted approval-response regression test passed.
• Full Vitest suite passed.
• TypeScript typecheck passed.
• Prettier format check passed.
• Whitespace diff check passed.
• ESLint is currently not clean because of existing unrelated lint errors outside this patch.

Executed with:

• pnpm exec vitest run src/modules/approvals/response-handler.test.ts
• pnpm test -- --run src/modules/approvals/response-handler.test.ts
• pnpm typecheck
• pnpm format:check
• git diff --check HEAD~1..HEAD
• pnpm lint

Notes:

• pnpm test -- --run src/modules/approvals/response-handler.test.ts executed the repository's Vitest suite in this environment; it passed with 30 test files and 326 tests.
• pnpm lint fails on pre-existing unrelated lint issues such as unused imports/variables and broad catch-block warnings in files not touched by this PR.
• Remote CI currently reports successful ci and label checks for this PR.

Disclosure notes

• No production systems were tested.
• No real credentials, secrets, or destructive payloads are included.
• The attacker model assumes a user able to submit or replay a valid approval response payload; unauthenticated reachability is not claimed.
• No unrelated files were changed.