Skip to content

The playground's delete accepts any component name, so it can release a compiled component to every Bot #108

Description

@Hotragn

DELETE /api/sandboxed/:name hands the raw path parameter to the store, which deletes from
components — the table shared with the compiled catalogue — with nothing checking which kind of
component the name belonged to.

The three mutating routes on this surface disagree about what a name may be, and that is the bug
rather than the delete itself:

Route Guard
save refuses a name that is not a slug (SandboxedNameRefusedError, sandboxed.ts:165)
publish refuses a name with no draft behind it (requireRow, sandboxed.ts:241)
remove none

Where it is

sandboxed-routes.ts:109-115:

routes.delete("/:name", requireUser, async (context) => {
  const forbidden = requireAdmin(context);
  if (forbidden) return forbidden;

  await store.remove(context.req.param("name"), actorEmail(context));
  return context.json({ ok: true });
});

and sandboxed.ts:279-285:

async remove(name: string, by: string): Promise<void> {
  await database
    .delete(sandboxedComponents)
    .where(eq(sandboxedComponents.name, name));
  await database.delete(components).where(eq(components.name, name));

For a compiled component's name the first delete matches nothing and the second one succeeds.
components.name is the primary key and kind (db/schema/components.ts:33) is what separates a
sandboxed row from a chart, card or decision the build ships. Nothing reads it here.

What goes with it

Both child tables cascade on components.name:

  • componentExclusionsdb/schema/components.ts:66-68, onDelete: "cascade"
  • componentFunctionsdb/schema/components.ts:95-97, onDelete: "cascade"

The two fail in opposite directions, and only one of them matters much.

The withholdings fail open. The schema says so itself, at db/schema/components.ts:57-61:

One Bot held back from one component. A published component is available to every Bot; a row here
is the exception.

So an exclusion row is the entire mechanism for "not this Bot". Losing it does not hide the
component, it releases it. Then syncCatalogue (components/store.ts:118-147) treats the component
as missing on the next announcement from a browser and reinserts it:

// Published on arrival, so a fresh install can draw from the moment it starts.
publishedDescription: entry.description,
published: true,

A component an administrator had deliberately kept from one Bot comes back published and available
to every Bot.

The function grants fail closed, which the schema also states (:83-86): absence is the refusal,
so the component can call nothing. That is a capability silently lost rather than one gained — worth
fixing, not worth alarm.

The trail describes the wrong thing. The audit row is component.unpublished with
payload.kind: "sandboxed" (sandboxed.ts:287-292), which is the one thing the deleted component
was not, and "unpublished" rather than "governance deleted, cascading".

How far this goes, honestly

requireAdmin gates the route, so this is not reachable by an ordinary signed-in user and it is not
an escalation. It is a footgun: a name typed or scripted wrong, or a UI that lists more than it
should, and per-Bot governance is gone with a { ok: true } and a row naming the wrong kind. The
recovery is also not obvious, because the component reappears looking healthy — published, drawable,
and no longer withheld from anybody.

I would not oversell it beyond that.

What I think the fix is

Refuse a name this surface does not own, and answer 404 the way publish does.

Gating on the governance row's kind rather than on the presence of a sandboxed_components row,
because ownership is the actual question and the two answers differ in one case worth keeping: the
two deletes above are not in a transaction, so a failure between them leaves a governance row with no
source. That orphan is the "catalogue disagrees with the build" state the existing comment names, it
is this surface's to clear, and requiring the source row would have made it undeletable through the
only endpoint that could.

A PR is up alongside this issue rather than after it, so nobody picks it up twice.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions