Skip to content

fix(security): require moderator review before hiding reported skills - #3681

Merged
Patrick-Erichsen merged 1 commit into
codex/security-01-org-ownershipfrom
codex/security-02-report-review
Sep 11, 2026
Merged

Patrick-Erichsen merged 1 commit into
codex/security-01-org-ownershipfrom
codex/security-02-report-review

Conversation

@Patrick-Erichsen

@Patrick-Erichsen Patrick-Erichsen commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Four ordinary accounts could automatically hide any visible skill by reporting it once each. Report submission now updates moderator intake and counts only; an authorized moderator must explicitly decide to hide a skill. Official and ordinary skills follow the same rule.

Addresses GHSA-5jj4-m8c9-gwcq. The advisory is published with the deployed revision and regression evidence.

Verification

  • Before: a regression against cbfee7343ddc867316dd9b3de6fa8856730f9f41 observed moderationStatus: hidden after the fourth distinct report.
  • After: authenticated runtime tests cover official and ordinary skills, four retained reports, duplicate-report rejection, denied reporter moderation privileges, and successful moderator hide.
  • Real local Convex backend at http://127.0.0.1:3330: four distinct synthetic authenticated users reported an official skill through the public skills:report mutation. Result: reportCount: 4, moderationStatus: active, deleted: false.
  • bun run ci:static passed.
  • bun run ci:unit passed: 6,621 tests; 497 passing files, 1 skipped.
  • bun run ci:types-build passed.
  • Local Convex deployment and typecheck passed.
  • .agents/skills/autoreview/scripts/autoreview --mode local --no-web-search: clean, no actionable findings.

Best-fix verdict: best. Report counts remain useful moderation evidence, while visibility decisions stay with moderators. Raising the threshold or exempting only official skills would retain the coordinated takedown mechanism. Existing hidden rows and historical audit labels remain intact.

Combined release validation: ci:static, ci:unit (6,656 passing tests), and ci:types-build passed on f3cd9104981d1875cc2945418172837297afcb5d. The protected Test frontend passed four HTTP checks, two browser checks, and an archive download check.

Deployment verified

Merged and deployed in 8c2de6c506bb4efabe3f0c2ffb8370b9e23d4650. Test deployment and Production deployment succeeded for that exact SHA. Live reads/downloads/image rendering and browser smoke tests pass; forged ingress is rejected. The active Production catalog rollout was restored after backend deployment. The superseded private security-fork PR is closed.

@Patrick-Erichsen
Patrick-Erichsen requested a review from a team as a code owner September 11, 2026 15:43
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

🦞👀
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.

@Patrick-Erichsen
Patrick-Erichsen added this pull request to stack #3685 September 11, 2026 15:43
@vercel

vercel Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
clawhub Ready Ready Preview Sep 11, 2026 3:46pm UTC

Request Review

@Patrick-Erichsen Patrick-Erichsen changed the title codex/security 02 report review fix(security): require moderator review before hiding reported skills Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 11, 2026, 11:47 AM ET / 15:47 UTC.

ClawSweeper review

What this changes

The PR makes skill reports update moderation intake without automatically hiding skills, adds authorization regression coverage, and documents the revised moderation rule.

Merge readiness

Ready for maintainer review

No blocking findings. Current main still contains the report-driven takedown mechanism, so this security fix remains necessary; it is also collaborator-authored and protected from automatic closure.

Priority: P0
Reviewed head: 84f78d33eead16325067251d11d8c6fc5c97913c

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused security repair with relevant local backend evidence, authorization coverage, and no blocking findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The captured local Convex result exercises public skills:report with four distinct authenticated reporters and shows the skill remaining active and undeleted at count four; inspected regression tests additionally cover reporter denial and moderator success.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured local Convex result exercises public skills:report with four distinct authenticated reporters and shows the skill remaining active and undeleted at count four; inspected regression tests additionally cover reporter denial and moderator success.
Evidence reviewed 9 items Pinned change and repository identity: The origin remote identifies openclaw/clawhub. The verified base-to-head delta contains four files, with 83 additions and 52 deletions; the checkout remains clean.
Current main still needs the fix: Current main's public report mutation sets softDeletedAt and moderationStatus=hidden when the report count exceeds three, then hides embeddings. The proposed removal addresses that exact path.
Report intake and final effects: The changed mutation retains authentication, deduplication, active-report limits, report persistence, counts, and audit events. The event helper only appends records; skill triggers maintain derived search and publisher data without granting reporters moderation authority.
Findings None None.
Security None None.

How this fits together

ClawHub accepts skill reports from signed-in users and exposes them to moderators. Skill visibility controls whether catalog entries remain available, while explicit moderation actions update visibility, search embeddings, and audit records.

flowchart TD
  A[Signed-in reporter] --> B[Validate and deduplicate report]
  B --> C[Store report and update count]
  C --> D[Moderator intake]
  D --> E{Authorized moderation decision}
  E --> F[Hide skill and update search]
  C --> G[Retain existing visibility]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Diff size +83/-52 across 4 files The patch is confined to report handling, its regression test, and moderation intent.
Production versus test LOC Production +4/-44; tests +71/-0; specs +8/-8 The repair reduces production code while covering both ordinary and official skills.

Technical review

Best possible solution:

Keep reporting as auditable moderation intake, reserve report-driven visibility changes for explicit authorized decisions, and preserve historical hides for individual review.

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

Yes: four distinct authenticated users reporting one visible skill reach the automatic-hide branch on current main; this review established the path from source without executing it.

Is this the best way to solve the issue?

Yes—best fix. Removing report-count authority at the public mutation matches existing HTTP moderation intake; raising the threshold or exempting official skills would preserve the coordinated takedown mechanism.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P0: Current main allows four ordinary accounts to bypass moderator authority and suppress another publisher's skill.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured local Convex result exercises public skills:report with four distinct authenticated reporters and shows the skill remaining active and undeleted at count four; inspected regression tests additionally cover reporter denial and moderator success.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.

Label justifications:

  • P0: Current main allows four ordinary accounts to bypass moderator authority and suppress another publisher's skill.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured local Convex result exercises public skills:report with four distinct authenticated reporters and shows the skill remaining active and undeleted at count four; inspected regression tests additionally cover reporter denial and moderator success.

Evidence

What I checked:

  • Pinned change and repository identity: The origin remote identifies openclaw/clawhub. The verified base-to-head delta contains four files, with 83 additions and 52 deletions; the checkout remains clean. (84f78d33eead)
  • Current main still needs the fix: Current main's public report mutation sets softDeletedAt and moderationStatus=hidden when the report count exceeds three, then hides embeddings. The proposed removal addresses that exact path. (convex/skills.ts, cbfee7343ddc)
  • Report intake and final effects: The changed mutation retains authentication, deduplication, active-report limits, report persistence, counts, and audit events. The event helper only appends records; skill triggers maintain derived search and publisher data without granting reporters moderation authority. (convex/skills.ts:4193, 84f78d33eead)
  • Authorization remains at the visibility write: setSoftDeleted obtains the current user and asserts moderator privileges before any visibility write. Formal report triage likewise reloads the actor and checks moderator privileges before applying its final action. Reports do not create a stored permission or deferred hide operation. (convex/skills.ts:10932, 84f78d33eead)
  • Focused regression coverage: The two parameterized cases cover ordinary and official skills, four retained reports, unchanged visibility, duplicate rejection, denial of a reporter's explicit hide attempt, and successful moderator hiding. These tests were inspected, not executed during this read-only review. (convex/skillReports.runtime.test.ts:10, 84f78d33eead)
  • Captured real backend result: The supplied complete PR body reports four distinct synthetic authenticated users invoking public skills:report on a real local Convex backend, observing reportCount=4, moderationStatus=active, and deleted=false afterward. This directly exercises the changed owner and forbidden report-driven visibility effect. Combined-release checks at a separate SHA are supplemental, not proof of the pinned head's identity.

Likely related people:

  • Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • pgm-norbert: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

@Patrick-Erichsen
Patrick-Erichsen merged commit f7476e4 into main Sep 11, 2026
30 of 31 checks passed
@Patrick-Erichsen
Patrick-Erichsen deleted the codex/security-02-report-review branch September 11, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant