Skip to content

fix(gateway): enforce session kill HTTP scopes#58467

Closed
eleqtrizit wants to merge 3 commits into
openclaw:mainfrom
eleqtrizit:fix/gateway-session-kill-http-scopes
Closed

fix(gateway): enforce session kill HTTP scopes#58467
eleqtrizit wants to merge 3 commits into
openclaw:mainfrom
eleqtrizit:fix/gateway-session-kill-http-scopes

Conversation

@eleqtrizit
Copy link
Copy Markdown
Contributor

Summary

  • Aligns HTTP session-kill authorization with the gateway operator scope model already enforced on WebSocket RPC
  • Preserves the requester-owned kill flow while removing shared-secret bearer access to admin and write paths without trusted scopes

Changes

  • Switched src/gateway/session-kill-http.ts to the hardened HTTP request-auth path that only trusts declared scopes on trusted auth surfaces
  • Required sessions.delete scope for local admin kills and sessions.abort scope for requester-owned kills
  • Added regression coverage for bearer-auth denial, untrusted x-openclaw-scopes headers, and trusted-scope success cases on both branches

Validation

  • Ran corepack pnpm test -- src/gateway/session-kill-http.test.ts
  • Ran PATH="/app/.local/bin:$PATH" corepack pnpm build
  • Ran local agentic review with claude -p "/review"; the command requested PR context and returned no actionable code feedback

Notes

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S labels Mar 31, 2026
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: 9e1525fb44

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/gateway/session-kill-http.ts
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 31, 2026

Greptile Summary

This PR hardens the HTTP session-kill endpoint by enforcing the operator scope model already present on the WebSocket RPC path. It switches from authorizeGatewayBearerRequestOrReply (which returned a plain boolean) to authorizeGatewayHttpRequestOrReply (which returns an AuthorizedGatewayHttpRequest carrying trustDeclaredOperatorScopes), then gates admin kills behind sessions.deleteoperator.admin and requester-owned kills behind sessions.abortoperator.write. Shared-secret bearer auth can no longer self-assert scopes via x-openclaw-scopes headers, aligning this endpoint with the broader gateway security model.

Key changes:

  • session-kill-http.ts: Replaced authorizeGatewayBearerRequestOrReply with authorizeGatewayHttpRequestOrReply and added a post-path-gate scope check using authorizeOperatorScopesForMethod
  • session-kill-http.test.ts: Updated existing admin-kill tests to supply method: "trusted-proxy" auth + the x-openclaw-scopes: operator.admin header; added four new regression tests covering bearer-auth denial, untrusted scope header rejection, and trusted-scope success on both kill branches

Logic correctness: The requiredOperatorMethod ternary (sessions.abort when requester-owned, sessions.delete otherwise) is exactly consistent with the execution branch further down (if (!allowLocalAdminKill && requesterSessionKey)), so scope requirement and code path are always in sync.

No issues found that would block merging.

Confidence Score: 5/5

This PR is safe to merge; the scope enforcement is logically sound and the new regression tests cover all critical paths.

All findings are P2 or below. The scope-method mapping is consistent with the execution branches, resolveTrustedHttpOperatorScopes correctly drops scopes for shared-secret bearer auth, and the test suite covers bearer-denial, untrusted-header rejection, and trusted-scope success for both kill paths.

No files require special attention.

Important Files Changed

Filename Overview
src/gateway/session-kill-http.ts Replaced boolean auth helper with AuthorizedGatewayHttpRequest-returning helper; added scope enforcement gate for both admin and requester-owned kill paths using correct scope names.
src/gateway/session-kill-http.test.ts Updated existing tests to supply trusted-proxy auth method and admin scope header; added four new regression tests covering bearer-auth denial and trusted-scope success on both kill branches.

Reviews (1): Last reviewed commit: "fix(gateway): enforce session kill HTTP ..." | Re-trigger Greptile

@eleqtrizit eleqtrizit force-pushed the fix/gateway-session-kill-http-scopes branch 2 times, most recently from 9f781b0 to 1831f3d Compare March 31, 2026 19:09
eleqtrizit and others added 3 commits April 1, 2026 15:40
Co-authored-by: Jacob Tomlinson <jtomlinson@nvidia.com>
Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
@eleqtrizit eleqtrizit force-pushed the fix/gateway-session-kill-http-scopes branch from 1831f3d to 6e73376 Compare April 1, 2026 15:41
@openclaw-barnacle openclaw-barnacle Bot added the r: too-many-prs Auto-close: author has more than twenty active PRs. label Apr 1, 2026
@openclaw-barnacle
Copy link
Copy Markdown

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

@openclaw-barnacle openclaw-barnacle Bot closed this Apr 1, 2026
@jacobtomlinson
Copy link
Copy Markdown
Contributor

It looks like this was already fixed in 54a0878

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

Labels

gateway Gateway runtime r: too-many-prs Auto-close: author has more than twenty active PRs. size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants