fix(marketplace): validate inside the components array on publish

[lilyshen0722]

Closes TASK-066 (filed from @sprint-review 58443).

The gap

POST /api/marketplace/publish length-checks installableId, name, description and readme, and enum-checks kind — then takes components from the request body and persists it behind a single components.length > 50 test. :250 persists on create, :175 on update, and :422 copies source.components wholesale on fork, so anything stored propagates without re-validation.

Nested validation was not overlooked as a category. It was applied to widgetLocation, which has an enum (Installable.ts:396), and stopped one field short of widgetUrl, which does not.

What this changes

components must be an array. A string has .length, so the old gate passed any string of 50 characters or fewer straight through.

widgetUrl takes a scheme allowlist (http:/https:). Nothing renders a widget today, so stored rows are inert — but inert only until a renderer ships, at which point every value already in the catalog goes live at once. Validation has to land before the feature rather than with it, and it is cheapest right now: GET /api/marketplace/browse measured total: 0 (with a 404 positive control confirming the route reaches Mongo), so there is nothing to backfill. Allowlist rather than denylist because enumerating what to block is the losing side of that argument.

The four Schema.Types.Mixed fields get a 16KB serialized cap — widgetConfigSchema, schemaFields, skillExamples, metadata. Mixed accepts any JSON of any size, four per component at 50 components per manifest. That is a larger surface than the URL and independent of it; the allowlist alone would not have touched it.

Component name and type are also checked, and errors name the offending index.

Who can reach it

Wider than "an authenticated human user". middleware/auth.ts:38 takes any cm_-prefixed token and resolves User.findOne({ apiToken: token }). Runtime cm_agent_* tokens miss that lookup and 401 — because they are stored hashed on a different field, not because of prefix routing. But the bot user token minted by issueUserTokenForInstallation (routes/registry/tokens.ts:147) is generateApiToken() output, stored plaintext in exactly that field on the agent's own User row, and every moltbot seat is issued one (provision.ts:200). Those authenticate as full users.

Separately: marketplace-api.ts never imports middleware/apiTokenScopes.ts, so no route here reads the scopes auth populates. Not addressed in this PR — a scope gate is a policy decision, and this PR is the input validation.

Tests

17 cases, backend/__tests__/unit/routes/marketplace-api.componentValidation.test.js. 20/20 green across all three marketplace suites on Node 22.

The suite leads with a control that publishes a well-formed component and reaches Installable.create. Every other case asserts a rejection, and an input rejected for the wrong reason — a broken mock, a missing required field in the shared body, a handler that 500s on everything — is indistinguishable from one rejected by the validator. The control pins the other end.

Not addressed

• Whether any Installable row anywhere holds a widgetUrl (browse only covers source: 'marketplace' + published; a Mongo read still settles the rest).
• installableId.length > 64 on a non-string body value is undefined > 64 → passes → .toLowerCase() throws → 500. Real, adjacent, and a different finding; left for its own change rather than widened into this one.
• The missing scope gate, above.

🤖 Generated with Claude Code

[lilyshen0722]

Gated at cde5dfe7, not the 56e81da7 cited in the pod — the head moved during review, so everything below is measured at cde5dfe7.

Verified. 20/20 green across all three marketplace suites on Node 22 (componentValidation, dual-write, marketplace.official) — the count checks out; my first run said 19/2 because marketplace.official.test.js doesn't match a marketplace-api pattern.

The tests discriminate rather than merely pass. Mutating the scheme check to if (false) reddens exactly the three scheme cases and nothing else:

✕ rejects widgetUrl scheme javascript:alert(1)
✕ rejects widgetUrl scheme data:text/html,<script>alert(1)</script>
✕ rejects widgetUrl scheme file:///etc/passwd

The components.length finding is real and nicely put — 'x'.repeat(50) has a .length of 50, so the old gate admitted any string. Array.isArray closes it.

Placement is right too, and I checked this rather than assuming: validateComponents sits at :242, before the branch splits, so it covers both the update path (:285 existing.components = components) and the create path (:360). One call, both writers.

One gap: /fork bypasses the validator entirely

:532 still copies components: source.components || [] with no validation, and the source lookup at :513 is:

const source = await Installable.findOne({
  installableId: sourceInstallableId.toLowerCase(),
  status: 'active',
});

No source: filter. So /fork reads any active Installable — including builtin / user / template rows, which are precisely the populations your browse measurement stated it could not see — and writes the copy out as source: 'marketplace' with published: true.

That path is concretely populated today: scripts/seed-native-agents.ts:282-285 writes source: 'builtin', status: 'active' rows carrying components.

Not exploitable right now — seeded content is first-party, so there is nothing hostile to launder. The problem is that it undermines this PR's own argument. The comment at the top says validation is cheapest now "when the published catalog is empty and there is nothing to backfill." That holds only if every path into the catalog validates. /fork is a path into the catalog that doesn't, sourcing from a population this PR never covered.

Suggested: run validateComponents(source.components) before building forkDoc and reject on failure. Failing closed is the right behaviour — refusing to fork a malformed legacy row is strictly better than promoting it to published: true.

No test covers this: the new suite resolves getRouteHandler('/publish', 'post') and only ever exercises publish. A fork case would also pin the fail-closed decision.

Verdict

Approving the publish-path fix — it does what it claims, the tests discriminate, and it covers both publish writers. The fork gap is the same finding one route over, so I'd rather see it in this PR than filed as a follow-up that outlives the context; your call whether it blocks.

Not verified: I did not run the full backend suite (three marketplace suites only); did not test the 16KB cap at its exact boundary; did not check whether InstallableInstallation.ts is a fourth writer of component data; and did not exercise /fork at runtime — the bypass is read from the source and the absence of any fork call to validateComponents, not from a live request.

[lilyshen0722]

Re-gated at bf2af843. The fork fix is correct and complete. Approving.

Coverage is now total across the three component writers, which is the property I actually wanted:

:242  validateComponents(components)         → guards :285 (update) and :360 (create)
:529  validateComponents(source.components)  → guards :550 (fork)

Those are every write to .components in the file. Nothing reaches Mongo ungated.

24/24 green across all three marketplace suites on Node 22.

The new tests discriminate. Neutering only the fork check (const sourceComponentsError = null;) reddens exactly two, and both are fork-specific:

✕ refuses to launder an unvalidated widgetUrl out of a builtin source
✕ refuses an oversized Mixed field in the source

Failing closed with a 400 is the right call, and prefixing Source manifest cannot be forked: keeps it attributable — a caller can tell "your request is malformed" from "the thing you asked to fork is."

The regression this could have caused, measured

A fail-closed check against pre-existing rows can newly break forking of legitimate first-party content, so I checked the population rather than assuming it was fine. FIRST_PARTY_APPS resolves through config/native-agents/apps.ts to five definitions, and every seeded component clears the bounds with room:

pod-summarizer   name 14   desc  68
pod-welcomer     name 12   desc  72
recorder         name  8   desc   9
scout            name  5   desc  73
task-clerk       name 10   desc  59
                 (limits: name 100, desc 500)

All carry type: 'agent' (in the enum), no widgetUrl, and no Mixed fields — runtime and persona aren't in MIXED_COMPONENT_FIELDS. So the entire forkable builtin population passes, and no existing fork breaks.

Your reasoning in the comment is right and worth keeping verbatim — the status: 'active'-only lookup is exactly why publish-time validation doesn't cover fork, and remote belongs in that list.

Not verified: I did not run the full backend suite (three marketplace suites only); did not exercise /fork against a live Mongo, so the laundering path is closed in source and in unit tests rather than end-to-end; did not test the 16KB cap at its exact boundary; and my builtin-population check covers FIRST_PARTY_APPS only — a user or template row created by some other path could still fail the new check, which would be correct behaviour but would surface as a fork that used to work and now 400s.

Note the PR reads BLOCKED right now — checks re-running after the push, not a failure.