openvibers/openvibe.inventory Commits

Inventory: a Workshop item's creator never publishes or retires it (staff review only); limits before the Media upload; resubmit a rejected badge (#12)

OP OpenVibers committed 10 Oct 2026 · 39b05efc35 · 3 files, +96 −6

An independent security review of Inventory found a real bypass of the Workshop's staff review (ADR-054 §6), plus a limit that came too late.

- **HIGH (fixed):** `PATCH /api/v1/definitions/:id` let a Workshop item's creator change its status directly. `publish: true` on a rejected draft published it, and `status: retired` took an in-review item out of the queue. Now a creator:
  - changes only the name, description and image, and only while the item is a draft;
  - resubmits for review with `publish: true` on a draft (within the 5-in-review cap), which never publishes;
  - can't change a reviewed or in-review item (409 `inventory.review_locked`).
- **MEDIUM (fixed):** the submit page uploaded the image to OpenVibe.Media before checking the 5-in-review and 20-a-day limits. A person over a limit now stores nothing (`checkWorkshopQuota` runs first).
- **New:** "Send for review again" on /workshop/mine for a rejected draft, with an optional rename.

`test/workshop.test.js` covers the locked statuses, resubmission, the page button (only the creator's own badge) and the upload-before-limit case. Suite: 19/19 files pass.

Changes

ddiff --git a/server/inventory/store.js b/server/inventory/store.js
iindex cffe74f..0986c96 100644
---- a/server/inventory/store.js
++++ b/server/inventory/store.js
@@@ -164,10 +164,7 @@ function createInventory({ s, outbox, log = console }) {
              if (!b.art || typeof b.art !== 'object' || !/^med_[0-9A-HJKMNP-TV-Z]{26}$/.test(String(b.art.media_id || '')) || Object.keys(b.art).length !== 1) fail(422, 'inventory.bad_art', 'a Workshop item is one reviewed image: art is { media_id }');
              if (!(Number.isInteger(b.supply_cap) && b.supply_cap >= 1 && b.supply_cap <= WORKSHOP.maxCap)) fail(422, 'inventory.bad_supply', `a Workshop item has a cap from 1 to ${WORKSHOP.maxCap}`);
              if (b.grantors !== undefined || b.aliases !== undefined || b.publish) fail(422, 'request.bad_body', 'a Workshop item is submitted for review, with no grantors or aliases');
--            const waiting = Number(await db.value("SELECT count(*) FROM inventory_definitions WHERE issuer = $1 AND status = 'in_review'", [requester]));
--            if (waiting >= WORKSHOP.inReview) fail(429, 'inventory.review_queue_full', `at most ${WORKSHOP.inReview} items in review at once`);
--            const today = Number(await db.value('SELECT count(*) FROM inventory_definitions WHERE issuer = $1 AND left(created_at, 10) = $2::text', [requester, s.iso().slice(0, 10)]));
--            if (today >= WORKSHOP.perDay) fail(429, 'inventory.submissions_today', `at most ${WORKSHOP.perDay} submissions a day`);
++            await checkWorkshopQuota(requester);
              b = { ...b, rarity: 'common' };
          }
          checkDefinitionBody(b);
@@@ -195,9 +192,10 @@ function createInventory({ s, outbox, log = console }) {
      async function updateDefinition(requester, id, b) {
          if (!b || typeof b !== 'object') fail(400, 'request.bad_body', 'send the fields to change');
          return await s.tx(async () => {
--            const cur = await db.maybe('SELECT * FROM inventory_definitions WHERE id = $1 FOR UPDATE', [String(id || '')]);
++            const cur = await db.maybe('SELECT d.*, k.workshop FROM inventory_definitions d JOIN inventory_kinds k ON k.id = d.kind WHERE d.id = $1 FOR UPDATE OF d', [String(id || '')]);
              if (!cur) fail(404, 'inventory.definition_not_found', 'no such definition');
              if (cur.issuer !== requester) fail(403, 'inventory.not_issuer', 'only its issuer edits a definition');
++            if (cur.workshop && PERSON_RE.test(requester)) return await updateWorkshopItem(requester, cur, b);
              checkDefinitionBody(b, { partial: true });
              const locked = cur.status === 'published' || cur.status === 'retired';
              for (const k of ['rarity', 'attributes', 'supply_cap', 'kind']) if (locked && b[k] !== undefined) fail(409, 'inventory.published', `${k} cannot change once the item is published (its owners rely on it)`);
@@@ -223,6 +221,48 @@ function createInventory({ s, outbox, log = console }) {
          });
      }
  
++    /**
++     * The Workshop's submission limits for a person (at most WORKSHOP.inReview waiting, WORKSHOP.perDay a day). Throws
++     * the same 429 the submission would; the submit page asks before it uploads the image, so a person over a limit
++     * stores nothing in OpenVibe.Media.
++     */
++    async function checkWorkshopQuota(requester) {
++        const waiting = Number(await db.value("SELECT count(*) FROM inventory_definitions WHERE issuer = $1 AND status = 'in_review'", [requester]));
++        if (waiting >= WORKSHOP.inReview) fail(429, 'inventory.review_queue_full', `at most ${WORKSHOP.inReview} items in review at once`);
++        const today = Number(await db.value('SELECT count(*) FROM inventory_definitions WHERE issuer = $1 AND left(created_at, 10) = $2::text', [requester, s.iso().slice(0, 10)]));
++        if (today >= WORKSHOP.perDay) fail(429, 'inventory.submissions_today', `at most ${WORKSHOP.perDay} submissions a day`);
++    }
++
++    /**
++     * A Workshop item edited by its creator (ADR-054 §6). Staff review is the only way to publish and takedown the only
++     * way to retire, so the creator never changes the status directly. They edit a draft (a rejected item), and
++     * `publish: true` on a draft resubmits it for review, within the in-review cap. In review or published, the item
++     * stays as reviewed: its name, words and image do not change.
++     */
++    async function updateWorkshopItem(requester, cur, b) {
++        const allowed = ['name', 'description', 'art', 'publish'];
++        const extra = Object.keys(b).filter((k) => !allowed.includes(k));
++        if (extra.length) fail(422, 'request.bad_body', `a Workshop item's creator changes only its name, description and image (not ${extra.join(', ')})`);
++        if (cur.status !== 'draft') fail(409, 'inventory.review_locked', `the item is ${cur.status}: staff publish and retire Workshop items, and a reviewed item does not change`);
++        checkDefinitionBody(b, { partial: true });
++        if (b.art !== undefined && (!b.art || typeof b.art !== 'object' || !/^med_[0-9A-HJKMNP-TV-Z]{26}$/.test(String(b.art.media_id || '')) || Object.keys(b.art).length !== 1)) {
++            fail(422, 'inventory.bad_art', 'a Workshop item is one reviewed image: art is { media_id }');
++        }
++        let status = 'draft';
++        if (b.publish) {
++            const waiting = Number(await db.value("SELECT count(*) FROM inventory_definitions WHERE issuer = $1 AND status = 'in_review'", [requester]));
++            if (waiting >= WORKSHOP.inReview) fail(429, 'inventory.review_queue_full', `at most ${WORKSHOP.inReview} items in review at once`);
++            status = 'in_review';
++        }
++        const now = s.iso();
++        await db.exec(`UPDATE inventory_definitions SET name = $2, description = $3, art = $4, status = $5, updated_at = $6,
++            review_note = CASE WHEN $5 = 'in_review' THEN NULL ELSE review_note END WHERE id = $1`,
++        [cur.id, b.name !== undefined ? b.name.trim() : cur.name, b.description !== undefined ? b.description : cur.description,
++            JSON.stringify(b.art !== undefined ? b.art : cur.art), status, now]);
++        await ledger(status === 'in_review' ? 'resubmitted' : 'edited', { definition: cur.id, actor: requester });
++        return await getDefinition(cur.id);
++    }
++
      // ── Grants ────────────────────────────────────────────
      /**
       * One instance of a published definition the requester issued, to a person. → { instance, created }.
@@@ -428,7 +468,7 @@ function createInventory({ s, outbox, log = console }) {
  
      return {
          listKinds, getKind, upsertKind, listDefinitions, getDefinition, byAlias, createDefinition, updateDefinition,
--        grant, consume, inventory, equipped, equip, ledger, review, workshopItems, takedown,
++        grant, consume, inventory, equipped, equip, ledger, review, workshopItems, takedown, checkWorkshopQuota,
      };
  }
  
ddiff --git a/server/workshop/pages.js b/server/workshop/pages.js
iindex 890c415..e421a99 100644
---- a/server/workshop/pages.js
++++ b/server/workshop/pages.js
@@@ -113,6 +113,8 @@ ${notice(req)}
          if (pic.error) return back(res, '/workshop/new', 'error', pic.error);
          const cap = Number.parseInt(f.supply_cap, 10);
          try {
++            // The limits first: a person over them stores nothing in OpenVibe.Media.
++            await inv.checkWorkshopQuota(`user:${me}`);
              const mediaId = await workshopMedia.upload({ buffer: form.file.buffer, type: pic.type, name: f.name, ownerSubject: me });
              await inv.createDefinition(`user:${me}`, {
                  kind: KIND, name: String(f.name || '').trim(), description: String(f.description || '').trim() || undefined,
@@@ -142,6 +144,10 @@ ${mine.length ? html`<ul class="ws-mine">${mine.map((d) => html`<li class="ws-it
    <div class="ws-item-body">
      <h2>${d.name} <span class="ws-state ws-${d.status}">${STATUS_LABEL[d.status] || d.status}</span></h2>
      ${d.status === 'draft' && d.review_note ? html`<p class="notice bad">Staff's reason: ${d.review_note}</p>` : ''}
++    ${d.status === 'draft' ? html`<form method="post" action="/workshop/${d.id}/resubmit" class="ws-give">
++      <label>Name <input type="text" name="name" required maxlength="64" value="${d.name}"></label>
++      <button type="submit" class="inv-btn primary">Send for review again</button>
++    </form>` : ''}
      ${d.status === 'in_review' ? html`<p class="muted">Waiting for staff. Nobody else can see it yet.</p>` : ''}
      ${d.status === 'published' ? html`<p class="muted">${d.supply.issued} of ${d.supply.cap} given · <a href="/items/${d.id}">its page</a></p>
      ${d.supply.issued < d.supply.cap ? html`<form method="post" action="/workshop/${d.id}/give" class="ws-give">
@@@ -170,6 +176,21 @@ ${mine.length ? html`<ul class="ws-mine">${mine.map((d) => html`<li class="ws-it
          }
      });
  
++    // A rejected badge goes back to staff: its creator may rename it first (the image stays the one they uploaded).
++    r.post('/workshop/:id/resubmit', express.urlencoded({ extended: false, limit: '2kb' }), async (req, res) => {
++        const me = signedIn(req);
++        if (!me) return login(res, '/workshop/mine');
++        if (!sameOrigin(req, config.baseUrl)) return res.status(403).type('text/plain').send('That form must come from this site.');
++        const name = String((req.body || {}).name || '').trim();
++        try {
++            await inv.updateDefinition(`user:${me}`, String(req.params.id || ''), { ...(name ? { name } : {}), publish: true });
++            return back(res, '/workshop/mine', 'done', 'Sent for review again.');
++        } catch (err) {
++            if (err instanceof InventoryError) return back(res, '/workshop/mine', 'error', err.detail || err.code);
++            throw err;
++        }
++    });
++
      // ── Staff review ─────────────────────────────────────────
      r.get('/workshop/review', async (req, res) => {
          if (!signedIn(req)) return login(res, '/workshop/review');
ddiff --git a/test/workshop.test.js b/test/workshop.test.js
iindex ad031d6..8baf6ab 100644
---- a/test/workshop.test.js
++++ b/test/workshop.test.js
@@@ -104,6 +104,31 @@ function multipart(fields, file) {
              assert.match((await t.get('/workshop/mine', { as: alice })).text, /Staff(&#39;|&#x27;|')s reason: Too close to a channel logo\./);
          });
  
++        await check('the creator never publishes or retires: a rejected draft is edited and resubmitted for review, and a reviewed item is locked', async () => {
++            const patch = (json) => t.get(`/api/v1/definitions/${first.id}`, { as: alice, method: 'PATCH', json, headers: origin });
++            let r = await patch({ name: 'OG Viewer II', publish: true });
++            assert.strictEqual(r.status, 200, r.text);
++            assert.strictEqual(r.json().definition.status, 'in_review', 'publish on a draft resubmits it, it never publishes');
++            assert.strictEqual((await inv.getDefinition(first.id)).name, 'OG Viewer II');
++            r = await patch({ publish: true });
++            assert.deepStrictEqual([r.status, r.json().code], [409, 'inventory.review_locked'], 'in review: locked');
++            r = await patch({ status: 'retired' });
++            assert.deepStrictEqual([r.status, r.json().code], [422, 'request.bad_body'], 'the creator never retires (takedown is staff)');
++            r = await patch({ rarity: 'legendary' });
++            assert.strictEqual(r.status, 422, 'nor sets a rarity');
++            assert.strictEqual((await inv.getDefinition(first.id)).status, 'in_review');
++            await form(staff, `/workshop/${first.id}/review`, { decision: 'reject', reason: 'Still too close.' });
++            assert.strictEqual((await inv.getDefinition(first.id)).status, 'draft');
++            // The page's button resubmits a draft (renamed), and only its creator's.
++            assert.match((await t.get('/workshop/mine', { as: alice })).text, /Send for review again/);
++            assert.match((await form(carol, `/workshop/${first.id}/resubmit`, { name: 'Mine now' })).headers.get('location') || '', /error=/);
++            r = await form(alice, `/workshop/${first.id}/resubmit`, { name: 'OG Viewer III' });
++            assert.match(r.headers.get('location') || '', /done=/);
++            const back = await inv.getDefinition(first.id);
++            assert.deepStrictEqual([back.status, back.name], ['in_review', 'OG Viewer III']);
++            await form(staff, `/workshop/${first.id}/review`, { decision: 'reject', reason: 'Still too close.' });
++        });
++
          let badge;
          await check('a published badge is public, given by @name within its cap, and worn with its image', async () => {
              await submit(alice, { name: 'Night Owl', supply_cap: '2', rights: 'yes' }, webp(256, 256));
@@@ -133,6 +158,10 @@ function multipart(fields, file) {
              for (let i = 0; i < 5; i++) await inv.createDefinition(`user:${dave.subject}`, { kind: 'network.badge', name: `B${i}`, art: { media_id: 'med_01JZ0000000000000000000099' }, rarity: 'common', attributes: {}, supply_cap: 10 });
              const sixth = await inv.createDefinition(`user:${dave.subject}`, { kind: 'network.badge', name: 'B6', art: { media_id: 'med_01JZ0000000000000000000099' }, rarity: 'common', attributes: {}, supply_cap: 10 }).catch((e) => e);
              assert.strictEqual(sixth.code, 'inventory.review_queue_full');
++            const stored = uploads.length;
++            const over = await submit(dave, { name: 'B7', supply_cap: '5', rights: 'yes' }, png(64, 64));
++            assert.match(over.headers.get('location') || '', /error=/);
++            assert.strictEqual(uploads.length, stored, 'over the limit, the image is never uploaded to Media');
              const notStaff = await t.get(`/api/v1/definitions/${badge.id}/review`, { as: alice, method: 'POST', json: { decision: 'publish' }, headers: origin });
              assert.deepStrictEqual([notStaff.status, notStaff.json().code], [403, 'inventory.staff_only']);
              const earned = await inv.grant(`user:${alice.subject}`, { definition_id: badge.id, subject: dave.subject, idempotency_key: 'ws:earned-0001', origin: 'earned' }).catch((e) => e);