fix(security): enforce slash invocation write authority by jason-allen-oneal · Pull Request #146 · openclaw/clickclack

jason-allen-oneal · 2026-08-03T15:33:03Z

What Problem This Solves

Follow-up to #145. Registered slash-command invocations must use the same
rolling 24-hour guest write budget as guest messages, and authorization must
remain authoritative at the persistence boundary. The previous revision still
left a PostgreSQL revocation lock race and did not directly cover all
cross-workspace and stale-authorization inputs.

The required behavior is:

the first three combined guest writes can persist;
the fourth combined guest write returns HTTP 429;
denied, revoked, stale, hidden-channel, blocked, timed-out, and
scope-mismatched invocations create no invocation row and reach no callback;
callback failures after persistence still consume the budget slot.

What Changed

PostgreSQL GetActiveSlashCommandWorkspace now uses FOR SHARE. An invocation
transaction that acquires the command row first may finish persistence; a
concurrent revoked_at update waits for that transaction to commit.
Regenerated PostgreSQL and SQLite SQLC output.
Added deterministic PostgreSQL lock-order coverage using transaction
synchronization and pg_stat_activity blocking detection.
Added PostgreSQL and SQLite regressions for command/workspace/channel scope
mismatches, forged workspace IDs, revoked commands, membership removal after
lookup, channel deletion after lookup, and unrelated-row preservation.
Added an HTTP cross-workspace denial regression asserting no callback, no
persistence, and no sensitive command details in the response.
Removed the redundant CanPublishEphemeral call from the registered
slash-command HTTP path. Lookup authorization and transactional persistence
authorization remain.
Bounded guest-budget counting with LIMIT write_limit after the complete
messages/invocations UNION ALL.
Added forward-only index upgrades to
(workspace_id, user_id, channel_id, created_at) for the invocation budget
query.
Documented that persisted registered invocation attempts consume a guest
budget slot even when callback delivery later fails.

Verification

Current branch HEAD:

f350c66

Base:

origin/main at 601224d

Commands passed:

pnpm check
pnpm generate:sqlc
go test ./... -count=1
go vet ./...
pnpm fmt:go:check
go test -v ./apps/api/internal/store/postgres -run 'TestPostgresSlashCommand(GuestBudgetIndexMigration|RevocationWaitsForInvocationLock|InvocationRequiresChannelWriteAuthority|InvocationRejectsScopeAndStaleAuthorization)' -count=1
go test ./apps/api/internal/store/postgres -run TestPostgresSlashCommandRevocationWaitsForInvocationLock -count=20
go test ./apps/api/internal/store/postgres -run Migration -count=1
go test ./apps/api/internal/store/postgres -count=1
go test ./apps/api/internal/store/sqlite -run 'Test(SlashCommand|SlashCommandInvocation)' -count=1
go test ./apps/api/internal/httpapi -run TestHTTPSlashCommandRequiresChannelWriteAuthorityBeforeCallback -count=1
go test -race ./apps/api/internal/store/sqlite -run 'Test(SlashCommand|SlashCommandInvocation)' -count=1
go test -race ./apps/api/internal/httpapi -run TestHTTPSlashCommandRequiresChannelWriteAuthorityBeforeCallback -count=1
git diff --check origin/main...HEAD

The full Go suite and PostgreSQL package used a disposable loopback
PostgreSQL 17 test instance via CLICKCLACK_POSTGRES_TEST_DSN. PostgreSQL
integration tests were live, not skipped.

Current-Head Real Behavior Proof

Behavior addressed: an authenticated guest caller submits attacker-controlled
slash-command form data to the real HTTP slash-hook endpoint. The fourth
combined guest write must be denied before callback delivery and invocation
persistence.

Real environment:

checkout: security/slash-authorization-followup;
HEAD: f350c66;
real clickclack serve process;
migrated, file-backed SQLite store;
real HTTP POST /api/hooks/slash/{channel_id};
independent callback receiver;
direct SQLite row counts and receiver counts after every request.

Local redacted evidence log:

/home/rev/projects/clickclack-pr-logs/pr-146-f350c667/real-behavior.log

Exact proof steps after this patch:

PATH=/home/rev/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.26.5.linux-amd64/bin:$PATH \
GOTOOLCHAIN=local \
GOPROXY=file:///home/rev/go/pkg/mod/cache/download \
GOSUMDB=off \
go build -tags clickclack_e2e_unsafe_callbacks \
  -o <temporary-proof-directory>/clickclack ./apps/api/cmd/clickclack

<temporary-proof-directory>/clickclack serve \
  --addr 127.0.0.1:18080 \
  --data <temporary-proof-directory>/data \
  --dev-bootstrap=true

curl -sS -X POST \
  http://127.0.0.1:18080/api/hooks/slash/<guest-channel-id> \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -H 'X-ClickClack-User: <guest-user-id>' \
  --data-urlencode command=/proof \
  --data-urlencode text='attempt N'

sqlite3 <temporary-proof-directory>/data/clickclack.db \
  'SELECT COUNT(*) FROM slash_command_invocations WHERE command_id = ...;'

The disposable fixture was seeded directly into SQLite to create a guest
membership and reserved guest channel, then the command was registered
through the real HTTP API as the owner. The request path itself used the
production HTTP router and persistence/callback code.

Observed output:

PR=146
HEAD=f350c667c3cad7d74f369e2dbe764f437ac152eb
health={"status":"ok"}
latest_migration=0042_slash_command_guest_budget_channel_index.sql
attempt=1 http=200 response={"response_type":"ephemeral"} persisted_invocations=1 callback_count=1
attempt=2 http=200 response={"response_type":"ephemeral"} persisted_invocations=2 callback_count=2
attempt=3 http=200 response={"response_type":"ephemeral"} persisted_invocations=3 callback_count=3
attempt=4 http=429 response={"error":"waiting room post limit reached"} persisted_invocations=3 callback_count=3
proof_status=PASS

Observed result: the first three registered guest attempts persisted and
reached the independent callback receiver. The fourth request was rejected
with HTTP 429; its invocation count and callback count remained unchanged.

Query-Plan Evidence

With 1,000 channels and 100,000 invocation rows, PostgreSQL changed from a
hash join plus sequential invocation scan under the old
(workspace_id, user_id, created_at) index to a nested-loop index-only scan
under (workspace_id, user_id, channel_id, created_at). Rows read before the
LIMIT fell from 2,001 to 3, and measured execution time fell from 0.299 ms to
0.085 ms in the disposable PostgreSQL 17 plan.

SQLite likewise changed from a scan of the invocation table to a covering-index
search. No message index was added.

What Was Not Tested

The live boundary proof used the explicitly named
clickclack_e2e_unsafe_callbacks build tag so a loopback receiver could be
used. It does not prove the production public-address callback dialer;
callback SSRF/redirect/proxy policy remains covered by its dedicated tests.
The live boundary proof used loopback-only X-ClickClack-User dev auth rather
than a production bearer/session credential. The actor, channel, persistence,
budget, and callback paths were real.
The live HTTP proof used SQLite. PostgreSQL behavior was verified through the
live PostgreSQL store package and deterministic concurrency test, not a full
deployed PostgreSQL HTTP server.
No multi-node or production deployment test was performed.

Unrelated working-tree changes were not present before this work, none were
overwritten or included, and the final repository worktree is clean.

clawsweeper · 2026-08-03T15:38:56Z

Codex review: needs maintainer review before merge. Reviewed August 4, 2026, 12:01 AM ET / 04:01 UTC.

ClawSweeper review

What this changes

The branch adds transactional slash-command authorization and scope checks, counts persisted guest command invocations against the guest write limit, and adds SQLite/PostgreSQL migrations and regressions.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open for maintainer review: the PR appears to close a real authorization gap with strong live proof, but it deliberately makes existing guest slash-command workflows share the three-write rolling quota and may return HTTP 429 after upgrade.

Priority: P1
Reviewed head: f350c667c3cad7d74f369e2dbe764f437ac152eb
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	Strong live proof and focused cross-store coverage support a good patch, pending maintainer acceptance of the deliberate quota compatibility change.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (live_output): The PR body supplies current-head live HTTP output showing three persisted and delivered guest invocations, followed by a fourth HTTP 429 with neither another record nor callback.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): The PR body supplies current-head live HTTP output showing three persisted and delivered guest invocations, followed by a fourth HTTP 429 with neither another record nor callback.
Evidence reviewed	5 items	Current authorization gap: Current main checks guest authority before registered invocation persistence, but its quota query counts only messages; a stored invocation does not consume a guest write slot. Persistence-boundary repair: The branch re-reads active command and channel workspace state, verifies scope and write authority inside one transaction, then persists only after those checks succeed. Repository SQL policy followed: The change updates SQL query/schema sources alongside generated sqlc bindings, consistent with the repository policy.
Findings	None	None.
Security	None	None.

How this fits together

The slash-hook endpoint resolves a registered command, persists an invocation, then sends its callback. SQLite and PostgreSQL store logic decides whether the caller, channel, command, and guest quota are valid before either persistence or callback delivery.

flowchart LR
  A[Guest slash request] --> B[Slash-hook endpoint]
  B --> C[Command lookup]
  C --> D[Transactional authority check]
  D --> E[Shared guest write quota]
  E --> F[Invocation record]
  F --> G[Callback delivery]

Decision needed

Question	Recommendation
Should successful registered slash-command invocations consume the existing guest three-write rolling budget after upgrade?	Accept the shared quota: Adopt the documented combined budget so each persisted guest command invocation consumes one of the three guest write slots.

Why: The branch makes a deliberate user-visible quota change that existing deployments will discover at runtime, and the implementation cannot establish whether that compatibility tradeoff is intended.

Before merge

Resolve merge risk (P1) - Existing guests who previously invoked registered commands beyond their message quota will begin receiving HTTP 429 once their combined rolling three-write budget is exhausted.
Complete next step (P2) - A maintainer must settle the quota compatibility contract; no discrete mechanical defect was found in the proposed implementation.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch surface	21 files; +1,420/-55 lines	The repair spans HTTP routing, both storage engines, forward migrations, generated sqlc bindings, documentation, and regression coverage.
Production vs tests	production +240/-54; tests +1,178/-1	Most added code is targeted regression coverage, while production changes are concentrated in authorization, quota queries, and migrations.

Merge-risk options

Maintainer options:

Accept the combined quota (recommended)
Approve the upgrade behavior and merge the documented shared guest budget with its authorization regressions.
Preserve separate command capacity
Revise the branch to retain transactional authorization while omitting command invocations from the guest message quota.

Technical review

Best possible solution:

Explicitly accept the combined guest-write quota as the upgrade contract, then land the transactional authorization hardening with its cross-store regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes: current main’s source counts only guest messages before it persists a registered invocation, while the PR provides current-head live HTTP output showing the repaired three-successes/fourth-429 boundary.

Is this the best way to solve the issue?

Unclear: transactional revalidation is the narrow maintainable security fix, but maintainers must decide whether sharing the existing guest quota is the intended upgrade policy.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 601224ddee75.

Labels

Label justifications:

P1: Unauthorized guest slash-command persistence or callback delivery is an urgent channel authorization defect.
merge-risk: 🚨 compatibility: Existing guest integrations can newly hit HTTP 429 because command invocations join the rolling write budget.
merge-risk: 🚨 security-boundary: The PR changes authorization decisions and callback gating at the slash-command persistence boundary.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body supplies current-head live HTTP output showing three persisted and delivered guest invocations, followed by a fourth HTTP 429 with neither another record nor callback.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies current-head live HTTP output showing three persisted and delivered guest invocations, followed by a fourth HTTP 429 with neither another record nor callback.

Evidence

What I checked:

Current authorization gap: Current main checks guest authority before registered invocation persistence, but its quota query counts only messages; a stored invocation does not consume a guest write slot. (apps/api/internal/httpapi/features.go:1577, 601224ddee75)
Persistence-boundary repair: The branch re-reads active command and channel workspace state, verifies scope and write authority inside one transaction, then persists only after those checks succeed. (apps/api/internal/store/postgres/slash_commands.go:202, f350c667c3ca)
Repository SQL policy followed: The change updates SQL query/schema sources alongside generated sqlc bindings, consistent with the repository policy. (AGENTS.md:1, f350c667c3ca)
Feature history: The original slash-command persistence path dates to a571a1d; steipete’s merged pull request 145 is the most recent related current-main security hardening. (apps/api/internal/store/sqlite/slash_commands.go:163, a571a1de695f)
Current-main status: The checked-out main is 601224d and does not contain the PR head; the latest tagged release is v0.2.1, so this change is neither implemented on main nor released. (apps/api/internal/httpapi/features.go:1579, 601224ddee75)

Likely related people:

Shakker: Current-main blame attributes the original lookup and invocation persistence implementation to the source-introducing commit. (role: introduced original slash-command persistence; confidence: high; commits: a571a1de695f; files: apps/api/internal/store/sqlite/slash_commands.go, apps/api/internal/store/postgres/slash_commands.go)
steipete: Authored merged pull request 145, the immediately preceding current-main integration and slash authorization hardening. (role: recent related security-hardening author; confidence: high; commits: f62c1709f867; files: apps/api/internal/httpapi/features.go, docs/features/integrations.md)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

Get explicit maintainer confirmation that the shared guest quota is the intended upgrade contract.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

reviewed 2026-08-03T15:37:21.161Z sha 2971562 :: needs real behavior proof before merge. :: none
reviewed 2026-08-03T16:39:41.416Z sha 2971562 :: needs maintainer review before merge. :: none

jason-allen-oneal · 2026-08-03T16:35:45Z

@clawsweeper re-review

clawsweeper · 2026-08-03T16:35:48Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

State: Complete
Detail: The durable review result and its route handoff completed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/30833367676
Updated: 2026-08-03T16:42:31.937Z

jason-allen-oneal · 2026-08-04T02:42:48Z

@clawsweeper re-review

clawsweeper · 2026-08-04T02:42:50Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

State: Complete
Detail: The durable review result and its route handoff completed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/30876458321
Updated: 2026-08-04T04:02:36.702Z

steipete · 2026-08-05T12:58:47Z

Landed as 8c157367eb7e3f9cc9fd2dc53605b55c0c2e71fe after maintainer verification.

Local proof at exact reviewed head e91217ad1ca571b07a520216841bee3d015830b0:

pnpm generate:sqlc reproduced generated store bindings with no additional diff.
pnpm check passed the full Go, web, SDK, desktop, FakeCo, typecheck, lint, and formatting gate.
go test -race ./apps/api/internal/store/sqlite -run 'Test(SlashCommand|SlashCommandInvocation)' -count=1 passed.
A real tagged clickclack binary served a migrated file-backed SQLite instance and delivered registered slash callbacks to an independent HTTP receiver. Attempts 1–3 returned HTTP 200 with persisted/callback counts 1, 2, and 3. Attempt 4 returned HTTP 429 with both counts remaining at 3.

Exact-head GitHub proof:

CI run https://github.com/openclaw/clickclack/actions/runs/31007509020 passed TypeScript, PostgreSQL-backed Go tests, deadcode, coverage, Playwright E2E, and Docker build.
Desktop run https://github.com/openclaw/clickclack/actions/runs/31007509189 passed macOS, Windows, and Linux builds.
Codex autoreview was clean before and after the migration consolidation; no accepted/actionable findings remained.

Caveat: the live HTTP proof used the explicitly test-only unsafe-callback build tag to reach a loopback receiver. Production callback network policy remained covered by the full test suite.

jason-allen-oneal added 2 commits August 3, 2026 21:58

fix(security): enforce slash invocation write authority

faad74f

fix(security): close slash authorization races and scope gaps

f350c66

jason-allen-oneal force-pushed the security/slash-authorization-followup branch from 2971562 to f350c66 Compare August 4, 2026 02:38

refactor: collapse unpublished guest budget migrations

e91217a

steipete merged commit 8c15736 into openclaw:main Aug 5, 2026
11 checks passed

Navigation Menu

Uh oh!

fix(security): enforce slash invocation write authority - #146

fix(security): enforce slash invocation write authority#146
steipete merged 3 commits into
openclaw:mainfrom
jason-allen-oneal:security/slash-authorization-followup

jason-allen-oneal commented Aug 3, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Aug 3, 2026 •

edited

Loading

Security

Review metrics

Merge-risk options

Technical review

Labels

Evidence

Rank-up moves

Rating scale

Workflow

History

Uh oh!

jason-allen-oneal commented Aug 3, 2026

Uh oh!

clawsweeper Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

jason-allen-oneal commented Aug 4, 2026

Uh oh!

clawsweeper Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

steipete commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jason-allen-oneal commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

What Changed

Verification

Current-Head Real Behavior Proof

Query-Plan Evidence

What Was Not Tested

Uh oh!

clawsweeper Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ClawSweeper review

What this changes

Merge readiness

Review scores

Verification

How this fits together

Decision needed

Before merge

Security

Review metrics

Merge-risk options

Technical review

Labels

Evidence

Rank-up moves

Rating scale

Workflow

History

Uh oh!

jason-allen-oneal commented Aug 3, 2026

Uh oh!

clawsweeper Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-allen-oneal commented Aug 4, 2026

Uh oh!

clawsweeper Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

steipete commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jason-allen-oneal commented Aug 3, 2026 •

edited

Loading

clawsweeper Bot commented Aug 3, 2026 •

edited

Loading

clawsweeper Bot commented Aug 3, 2026 •

edited

Loading

clawsweeper Bot commented Aug 4, 2026 •

edited

Loading