fix(forms): ignore client clawhubUserId on appeal submit

[SebTardif]

What Problem This Solves

POST https://appeals.openclaw.ai/clawhub/submit copies every FormData key except session, then stores {...context, ...collected.payload}. A hidden clawhubUserId field replaces the ClawHub ID that was fetched for the signed-in GitHub account.

Accept then runs clawhub.unbanUser with target: "clawhubUserId" from that stored payload. The review UI shows the injected ID, but the action still unbans whoever the applicant wrote.

This change persists only configured form fields, overlays server-fetched context last, and resolves the unban target from that context.

Why This Change Was Made

An authenticated applicant could unban a different ClawHub user by submitting an extra form field. The OAuth-bound context must win.

User Impact

Ban-appeal submit still works for the signed-in account. Extra FormData keys are ignored. Reviewers still see the ClawHub ID from server context.

Evidence

Live bun against the patched collect/merge helpers with attacker FormData:

$ bun -e "import { collectPayload, buildSubmissionPayload } from './src/forms/payload.ts'; ..."
{"collected":{"reason":"please unban"},"stored":{"reason":"please unban","clawhubUserId":"oauth-clawhub-user","clawhubHandle":"real"}}

The client sent clawhubUserId=attacker. It is absent from collected and the stored payload keeps oauth-clawhub-user.

Same-repo: the appeal form landed in #13. #35 stops accept/deny when form actions fail; it does not stop payload overwrite.

Real behavior proof

• Behavior or issue addressed: Appeal submit no longer lets client FormData replace the OAuth-bound ClawHub user id used as the unban target.
• Real environment tested: Windows 11, bun 1.4.1, worktree C:\tmp\wt-he-f008 at e356aee.
• Exact steps or command run after this patch: Built a Request with FormData reason=please unban and clawhubUserId=attacker, then called collectPayload and buildSubmissionPayload with context oauth-clawhub-user.
• Evidence after fix: terminal JSON above. Collected keys are only reason. Stored clawhubUserId is oauth-clawhub-user.
• Observed result after fix: Attacker id is dropped. Unban resolution reads the context id.
• What was not tested: A live POST to appeals.openclaw.ai with a real GitHub OAuth session.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 10, 2026, 11:33 PM ET / September 11, 2026, 03:33 UTC (Revision 2).

ClawSweeper review

What this changes

The PR filters appeal submissions to configured input fields, gives server-fetched account context precedence, and requires a stored ClawHub identity for unban requests.

Merge readiness

⛔ Blocked before merge - 6 items remain

The fix remains necessary on current main, and the earlier test-location finding is resolved. No introduced correctness defect was found, but the security-sensitive change still needs production-path proof and an explicit rollout decision for pending appeals.

Priority: P0
Reviewed head: 375f747f70217b76c134b44819acbc1ff9f691de
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused intake repair and resolved test-location finding are useful, but authority proof and pending-appeal rollout safety remain unsettled.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: Authority-chain proof required: the captured Windows Bun trace exercises payload helpers with supplied context, and the unban test mocks fetch. Provide redacted after-fix evidence through authenticated submission, persistence, reviewer acceptance, and real service I/O showing the allowed account succeeds while substituted and invalidated bindings cannot unban another account. Include pending pre-fix appeals; terminal traces or diagnostic recordings are suitable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: Authority-chain proof required: the captured Windows Bun trace exercises payload helpers with supplied context, and the unban test mocks fetch. Provide redacted after-fix evidence through authenticated submission, persistence, reviewer acceptance, and real service I/O showing the allowed account succeeds while substituted and invalidated bindings cannot unban another account. Include pending pre-fix appeals; terminal traces or diagnostic recordings are suitable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	10 items	Repository policy: Read the complete root AGENTS.md. Config-driven forms and shared Forms types informed the review; no nested AGENTS.md or maintainer-notes directory was found. Current main still permits payload overwrite: The main implementation collects every non-session field and spreads collected payload after server context. A submitted clawhubUserId therefore replaces the account identity before persistence. Current branch and release check: GitHub still reports the supplied main SHA. The releases endpoint returned no releases; no shipped fix was established.
Findings	None	None.
Security	Needs attention	Establish trust for pending appeal identities: Acceptance reads the same stored payload field for old and new submissions. Previously injected IDs remain usable unless pending rows are revalidated or invalidated; this is unresolved existing exposure rather than an introduced regression.

How this fits together

Hermit's forms service authenticates applicants, retrieves moderation context, stores appeals, and sends them to Discord reviewers. Accepting a ClawHub appeal uses Hermit's service credential to request an account unban.

flowchart TD
 A[Authenticated applicant and form fields] --> B[Filter configured inputs]
 C[ClawHub account context] --> D[Merge server context last]
 B --> D
 D --> E[Stored appeal]
 E --> F[Authorized Discord reviewer]
 F --> G[ClawHub unban service]

Loading

Decision needed

Question	Recommendation
How should pending ClawHub appeals be handled before their stored account IDs are treated as trusted after deployment?	Revalidate pending account bindings: Verify pending appeals against their stored authenticated applicant and reject mismatches before sending an unban request.

Why: Existing rows have no trust-version marker, and choosing revalidation or invalidation affects legitimate pending appeals.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: the captured Windows Bun trace exercises payload helpers with supplied context, and the unban test mocks fetch. Provide redacted after-fix evidence through authenticated submission, persistence, reviewer acceptance, and real service I/O showing the allowed account succeeds while substituted and invalidated bindings cannot unban another account. Include pending pre-fix appeals; terminal traces or diagnostic recordings are suitable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve security concern: Establish trust for pending appeal identities - Acceptance reads the same stored payload field for old and new submissions. Previously injected IDs remain usable unless pending rows are revalidated or invalidated; this is unresolved existing exposure rather than an introduced regression.
• Resolve merge risk (P1) - Pending pre-fix appeals may contain substituted IDs; this patch does not sanitize or invalidate those rows. That is residual exposure, not a newly introduced vulnerability.
• Resolve merge risk (P1) - The newly protected stored identity can outlive its submission-time account lookup. Evidence does not establish rejection before unban I/O when that binding becomes invalid or reassigned.
• Complete next step (P2) - Approve pending-appeal handling and add the required authorization proof. Redact credentials, IP addresses, private endpoints, and personal data from traces or diagnostic recordings. Update the PR body to trigger re-review; if it does not, ask a maintainer to comment @clawsweeper re-review.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [high] Establish trust for pending appeal identities — src/forms/actions.ts:90

Agent review details

Security

Needs attention: The patch improves intake security without a detected new vulnerability, but persisted identity trust remains a rollout and proof concern.

Review metrics

Metric	Value	Why it matters
Production and test delta	Production +36/-19, net +17; tests +149	Production growth extracts payload filtering and tightens identity resolution; five focused tests cover collection, merge precedence, and target selection.

Merge-risk options

Maintainer options:

1. Establish trust before acceptance (recommended)
   Approve pending-appeal handling and provide evidence that substituted or invalidated account bindings cannot reach the unban side effect.
2. Pause acceptance during rollout
   Keep affected pending appeals from being accepted until their identities are revalidated or the appeals are invalidated.

Technical review

Best possible solution:

Preserve server-owned identity at intake and establish an approved acceptance-time or rollout safeguard for untrusted pending appeals, with final-effect authorization evidence.

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

Yes, from source: submit an extra clawhubUserId with a valid eligible account's form, then have a reviewer accept it; main stores and dispatches the substituted ID. This review did not execute that path.

Is this the best way to solve the issue?

Yes for new submissions: filtering configured inputs and overlaying server context is a narrow repair. Complete remediation still requires an approved treatment of pending rows and proof through the final unban boundary.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against fd6920504d12.

Labels

Label justifications:

• P0: Current main allows an authenticated applicant to substitute the account unbanned when a reviewer accepts the appeal.
• merge-risk: 🚨 security-boundary: The changed trust boundary needs final-effect proof and rollout handling before stored appeal identities can safely be relied upon.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: the captured Windows Bun trace exercises payload helpers with supplied context, and the unban test mocks fetch. Provide redacted after-fix evidence through authenticated submission, persistence, reviewer acceptance, and real service I/O showing the allowed account succeeds while substituted and invalidated bindings cannot unban another account. Include pending pre-fix appeals; terminal traces or diagnostic recordings are suitable. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

• [high] Establish trust for pending appeal identities — src/forms/actions.ts:90
  Acceptance reads the same stored payload field for old and new submissions. Previously injected IDs remain usable unless pending rows are revalidated or invalidated; this is unresolved existing exposure rather than an introduced regression.
  Confidence: 0.98

What I checked:

• Repository policy: Read the complete root AGENTS.md. Config-driven forms and shared Forms types informed the review; no nested AGENTS.md or maintainer-notes directory was found. (AGENTS.md:1, 375f747f7021)
• Current main still permits payload overwrite: The main implementation collects every non-session field and spreads collected payload after server context. A submitted clawhubUserId therefore replaces the account identity before persistence. (src/forms/server.tsx:392, fd6920504d12)
• Current branch and release check: GitHub still reports the supplied main SHA. The releases endpoint returned no releases; no shipped fix was established. (fd6920504d12)
• Authenticated submission boundary: The patched handler validates the session and provider, fetches moderation context using the authenticated user, validates eligibility, then merges server context last before storing the appeal. (src/forms/server.tsx:368, 375f747f7021)
• Persistent authority and dependency signal: Submission payloads are serialized into the database. The changed unban function reads clawhubUserId from that payload and sends it to ClawHub's ban-appeal-unban endpoint with a service credential; this establishes the relevant ClawHub protocol dependency. It does not refresh the applicant-to-account binding. (src/forms/actions.ts:90, 375f747f7021)
• Final service authorization boundary: The verified ClawHub endpoint authenticates the service credential and forwards userId, reason, and reviewerDiscordId. Its mutation checks the target's ban state and audit record, then removes the ban; it does not receive or revalidate the applicant's GitHub account binding. (convex/users.ts:1896, cbfee7343ddc)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• hannesrudolph: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

• Add final-effect evidence for the allowed applicant, a substituted account, and an invalidated or reassigned binding before unban I/O.
• Document the approved handling of pending pre-fix appeals and demonstrate its upgrade behavior.

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 (1 earlier review cycle)

• reviewed 2026-09-11T03:23:53.651Z sha e356aee :: needs real behavior proof before merge. :: [P2] Move the Bun regression test outside the Worker source tree