Skip to content

fix(api): guard moderated skill files and tags#2287

Open
vyctorbrzezowski wants to merge 5 commits into
openclaw:mainfrom
vyctorbrzezowski:brzezowski/skill-file-tag-guards
Open

fix(api): guard moderated skill files and tags#2287
vyctorbrzezowski wants to merge 5 commits into
openclaw:mainfrom
vyctorbrzezowski:brzezowski/skill-file-tag-guards

Conversation

@vyctorbrzezowski
Copy link
Copy Markdown
Contributor

@vyctorbrzezowski vyctorbrzezowski commented May 16, 2026

Summary

Makes the public skill APIs consistently refuse files, versions, and tags that should not be public. Hidden, deleted, cross-skill, or stale data now returns a blocked/not found response instead of leaking old files.

What changed

  • Public skill file readers now share the same moderation block semantics as ZIP downloads.
  • Skill tags must point at a non-deleted version that belongs to the same skill.
  • Digest-backed public lists preserve legacy markerless latest-version rows, but fail closed when an explicit marker points to another skill.
  • getBySlug suppresses latestVersion when the latest row is soft-deleted or belongs to another skill.
  • resolveVersionByHash suppresses stale, cross-skill, or soft-deleted latestVersion rows instead of serializing them.
  • The digest/backfill validation model stays intact; expensive validation is still kept out of hot public list paths.

Public behavior

Invalid latest-version state becomes null/absent in public detail and resolver responses. It is never returned as a serialized stale version object.

Moderated file and package compatibility readers fail closed. Cross-skill tags and soft-deleted versions are rejected instead of being treated as public artifacts.

Legacy digest rows without the new owner marker keep their latest version for compatibility; rows with an explicit cross-skill owner marker are suppressed.

Behavior proof

Live Convex runtime proof for the endpoint paths ClawSweeper requested on rebased head 3ff605dd:

$ bunx convex run --push --typecheck=disable --codegen disable proof2287:run
✔ Convex functions ready!
{
  "fixture": {
    "activeSlug": "proof2287-vjc9cp6b-active",
    "moderatedSlug": "proof2287-vjc9cp6b-moderated"
  },
  "results": [
    {
      "body": "Blocked: this skill has been flagged as malicious by ClawScan and cannot be downloaded.",
      "label": "raw-file moderated skill",
      "status": 403
    },
    {
      "body": "Blocked: this skill has been flagged as malicious by ClawScan and cannot be downloaded.",
      "label": "package compatibility file for moderated skill",
      "status": 403
    },
    {
      "body": "Version not found",
      "label": "raw-file cross-skill tag",
      "status": 404
    },
    {
      "body": "Version not available",
      "label": "raw-file soft-deleted tag",
      "status": 410
    },
    {
      "body": "Version not found",
      "label": "package compatibility cross-skill tag",
      "status": 404
    },
    {
      "body": "Version not found",
      "label": "package compatibility soft-deleted tag",
      "status": 404
    }
  ]
}

After that proof, I removed the temporary convex/proof2287.ts helper and pushed the deployment again without it:

$ bunx convex run --push --typecheck=disable --codegen disable publishers:listMine
✔ Convex functions ready!
[]

Live Convex runtime proof for the compatibility-sensitive digest path from the earlier head:

$ bunx convex run --push --typecheck=disable --codegen disable proof2287:run
- Preparing Convex functions...

✔ Convex functions ready!
{
  "fixture": {
    "legacySlug": "proof2287-...-legacy",
    "staleSlug": "proof2287-...-stale"
  },
  "relevant": [
    {
      "latestVersion": "1.0.0",
      "slug": "proof2287-...-legacy"
    },
    {
      "latestVersion": null,
      "slug": "proof2287-...-stale"
    }
  ]
}

This creates one legacy markerless digest row and one explicit cross-skill/stale digest row. The legacy row still serializes latestVersion; the explicit stale row returns null, which addresses the compatibility concern without reopening the leak.

Focused regression suite on rebased head 3ff605dd:

$ bun run test convex/skills.public.test.ts convex/httpApiV1.handlers.test.ts convex/downloads.test.ts convex/skills.deleteTags.test.ts convex/skills.publicListCursor.test.ts convex/lib/skillSearchDigest.test.ts
Test Files  6 passed (6)
Tests       269 passed (269)

Validation

$ bun run ci:unit
Test Files  204 passed (204)
Tests       2003 passed (2003)
Statements  85.98%
Branches    75.17%
Functions   86.37%
Lines        89.48%
$ bun run ci:types-build
tsc, package typechecks, clawhub-mod typecheck, Vite build, and Nitro build passed.

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 16, 2026

@vyctorbrzezowski is attempting to deploy a commit to the Amantus Machina Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 16, 2026

Codex review: needs changes before merge.

Latest ClawSweeper review: 2026-05-23 05:43 UTC / May 23, 2026, 1:43 AM ET.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • 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.

Summary
The PR adds shared public guards so moderated, deleted, cross-skill, or stale skill versions are not exposed through public skill file, package, tag, latest-version, and digest-backed API paths.

Reproducibility: yes. Source inspection on current main shows public artifact readers and the newer export route can dereference stored version ids without the PR's same-skill guard; the PR body also includes live Convex proof for the older affected endpoint paths.

PR rating
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Summary: Strong runtime proof and a useful core patch are held back by a current-main export-path security gap that must be repaired before merge.

Rank-up moves:

  • Rebase onto current main and extend the same latest-version guard to /api/v1/skills/export.
  • Add focused export regression coverage for cross-skill and soft-deleted latest-version ids.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

Real behavior proof
Sufficient (terminal): The PR body includes live Convex terminal output showing moderated file/package, cross-skill tag, soft-deleted tag, and digest compatibility behavior after the fix.

Risk before merge

  • Resolving the current merge conflict without carrying the same same-skill and soft-delete guard into /api/v1/skills/export can leave stale or cross-skill artifact payloads readable through bulk export.
  • The intended fail-closed behavior changes public API outcomes for invalid or moderated artifacts to 403, 404, 410, or null, which may affect existing consumers that previously received stale data.
  • The PR is currently CONFLICTING against main, so the security-sensitive export path must be handled during rebase rather than assumed covered by current CI.

Maintainer options:

  1. Guard export before merge (recommended)
    Rebase the branch and reject stale, cross-skill, or soft-deleted export latest versions before the handler queues storage reads.
  2. Accept fail-closed API rollout
    After the export path is covered, maintainers can accept that invalid public artifact/version states now return blocked/not-found/gone/null responses.
  3. Pause for staged compatibility
    If API consumers need transition time, pause this PR until maintainers choose a compatibility note or staged rollout for the stricter public responses.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Rebase the PR onto current main and extend the skill export path to reject stale, cross-skill, or soft-deleted latest versions before loading files. Reuse the PR's public skill version guard and digest owner marker pattern where possible, and add focused export regression coverage.

Next step before merge
A narrow repair is available: rebase the PR and apply the same version-owner and soft-delete guard to the current main export route before merge.

Security
Needs attention: The diff narrows public artifact exposure, but current-main drift added a bulk export route that still needs the same stale/cross-skill version guard before merge.

Review findings

  • [P1] Guard export versions before reading files — convex/httpApiV1/skillsV1.ts:1724-1728
Review details

Best possible solution:

Rebase onto current main, apply the same public-version guard semantics to the export route, preserve the legacy markerless digest compatibility behavior, and land after maintainers accept the fail-closed API outcomes.

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

Yes. Source inspection on current main shows public artifact readers and the newer export route can dereference stored version ids without the PR's same-skill guard; the PR body also includes live Convex proof for the older affected endpoint paths.

Is this the best way to solve the issue?

No, not yet. The shared helper and digest marker approach is a maintainable fix, but it is incomplete against current main until the export route is guarded and covered by regression tests.

Label justifications:

  • P2: This is focused public API and artifact-access hardening with real but bounded blast radius.
  • merge-risk: 🚨 compatibility: The PR intentionally changes public artifact and version responses for moderated, stale, deleted, or cross-skill states.
  • merge-risk: 🚨 security-boundary: The patch changes public artifact exposure controls, and current main has a bulk export path that needs the same guard before merge.
  • rating: 🦐 gold shrimp: Current PR rating is 🦐 gold shrimp because proof is 🦞 diamond lobster, patch quality is 🦐 gold shrimp, and Strong runtime proof and a useful core patch are held back by a current-main export-path security gap that must be repaired before merge.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes live Convex terminal output showing moderated file/package, cross-skill tag, soft-deleted tag, and digest compatibility behavior after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live Convex terminal output showing moderated file/package, cross-skill tag, soft-deleted tag, and digest compatibility behavior after the fix.

Full review comments:

  • [P1] Guard export versions before reading files — convex/httpApiV1/skillsV1.ts:1724-1728
    Current main now has /api/v1/skills/export, which this branch must reconcile during rebase. That handler loads each digest latestVersionId and queues storage reads without checking version.skillId === digest.skillId or softDeletedAt, so stale or cross-skill digest state can still export the artifact payloads this PR is trying to suppress.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

  • [high] Export path can still read stale version files — convex/httpApiV1/skillsV1.ts:1724
    exportSkillsV1Handler loads digest.latestVersionId and then plans blob reads without checking version ownership or soft-delete state, so conflict resolution must apply the PR's guard before this security boundary is complete.
    Confidence: 0.86

Acceptance criteria:

  • bun run test convex/skills.exportList.test.ts convex/httpApiV1.handlers.test.ts convex/skills.public.test.ts convex/httpApiV1.shared.test.ts
  • bun run ci:unit
  • bun run ci:types-build

What I checked:

  • Live PR state: Live GitHub state shows head 3ff605d, labels including proof sufficient plus compatibility/security-boundary merge risks, mergeable state CONFLICTING/DIRTY, and the PR body's live Convex terminal proof for moderated file/package, cross-skill tag, soft-deleted tag, and digest compatibility paths. (3ff605dd3b9c)
  • Current-main raw-file gap: On current main, the public raw-file handler resolves latest/tagged versions by id and checks only missing or soft-deleted state before reading storage; it does not verify that the loaded version belongs to the requested skill. (convex/httpApiV1/skillsV1.ts:991, 0907fae0d991)
  • Current-main export gap: The newer export handler loads each digest.latestVersionId with getVersionByIdInternal and then queues file storage ids without checking version.skillId against the exported skill or rejecting soft-deleted versions. (convex/httpApiV1/skillsV1.ts:1724, 0907fae0d991)
  • Current-main export filter: listByDateRange currently filters export rows with Boolean(latestVersionId) and isPublicSkillDoc, but it does not validate the referenced latest-version row before exportSkillsV1Handler reads files. (convex/skills.ts:10467, 0907fae0d991)
  • PR guard helper: The PR adds getPublicSkillFileAccessBlock, isSkillVersionForSkill, and isPublicSkillVersionAvailableForSkill to centralize moderation blocking and same-skill/non-deleted version checks. (convex/lib/skillFileAccess.ts:15, 3ff605dd3b9c)
  • PR endpoint coverage: On the PR head, the raw-file route now applies moderation blocking and rejects versions whose skillId does not match the requested skill before storage access. (convex/httpApiV1/skillsV1.ts:988, 3ff605dd3b9c)

Likely related people:

  • Patrick Erichsen: Current main's raw-file/shared-helper baseline and the latest export logging/page-size changes are attributed to this author in blame and recent history. (role: recent area contributor; confidence: high; commits: b753b1f7ab0e, afd73264bdb3, 0907fae0d991; files: convex/httpApiV1/skillsV1.ts, convex/downloads.ts, convex/httpApiV1/shared.ts)
  • wuchengan734-a11y: The bulk skill export handler and listByDateRange export query are attributed to the Mirror Site export commit in blame. (role: introduced export route; confidence: high; commits: e22bb7d42708; files: convex/httpApiV1/skillsV1.ts, convex/skills.ts)
  • vyctorbrzezowski: Beyond authoring this PR, this contributor recently landed adjacent public skill API pagination work touching the same list/digest surface. (role: recent adjacent contributor; confidence: medium; commits: 07bf41e1091e; files: convex/skills.ts, convex/skills.publicListCursor.test.ts, CHANGELOG.md)
  • Peter Steinberger: History shows the HTTP API v1 split and moderation-batch consolidation that shaped the current API helper boundary. (role: historical refactor author; confidence: medium; commits: f94e20d4c35f; files: convex/httpApiV1/skillsV1.ts, convex/httpApiV1/shared.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 0907fae0d991.

@vyctorbrzezowski vyctorbrzezowski marked this pull request as ready for review May 16, 2026 13:59
@vyctorbrzezowski vyctorbrzezowski requested review from a team and Patrick-Erichsen as code owners May 16, 2026 13:59
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. labels May 16, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. labels May 18, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the brzezowski/skill-file-tag-guards branch from 6989c43 to 5ffc166 Compare May 18, 2026 15:01
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@vyctorbrzezowski vyctorbrzezowski force-pushed the brzezowski/skill-file-tag-guards branch from 5ffc166 to dcd84ef Compare May 18, 2026 20:13
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels May 18, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 20, 2026

ClawSweeper PR egg

🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@vyctorbrzezowski vyctorbrzezowski force-pushed the brzezowski/skill-file-tag-guards branch from e5ac58f to 3ff605d Compare May 21, 2026 15:27
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels May 21, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant