Skip to content

Store: real product images, availability, and a storefront worth browsing - #88

Merged
CaYatur merged 2 commits into
mainfrom
feat/store
Jul 28, 2026
Merged

Store: real product images, availability, and a storefront worth browsing#88
CaYatur merged 2 commits into
mainfrom
feat/store

Conversation

@CaYatur

@CaYatur CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Closes #76. Closes #77. Closes #78. Closes #80. Closes #81. Closes #82.

The store worked, and looked like it had been built to prove it worked. A
product image was a URL in a text box. The storefront was a flat grid c4 with
a 60px icon inside a card three times its size, a gift emoji stuck on the end of
crate names, and nothing to click into.

Images (#76, #77)

Icon by pasted URL or upload, in both admin UIs, with a live thumbnail of
exactly what buyers will see — including the broken-image state. Same control
for every crate reward icon, and for up to eight extra pictures per product,
shown as a thumbnail strip in the new detail view.

Panel uploads go to a new store-scoped route rather than the site's existing
/api/site/upload, which is gated on settings for the website's own server.
Somebody trusted to edit products should not need the keys to the public site to
give one a picture.

One validator, because these are attacker-controlled

Any web user with store can set an image source, and it renders for every
visitor to the public site. isSafeImageSrc allows exactly two shapes —
http(s)://… and /uploads/<plain filename> — and refuses everything else:

Refused Why
javascript: script execution the moment anything makes it a link
data: can carry an SVG, and an inline SVG can carry a <script>
//evil.example/x.png protocol-relative: reads like a path, is not one
/uploads/../../secrets escapes the uploads directory
/uploads/.env, /uploads/sub/dir.png not a plain filename

Case-insensitively — a scheme check that the shift key defeats is not a check.

Enforced in upsertProduct for the product icon, every reward icon and every
gallery image, so both admin UIs and the integration API pass through the same
gate. A refused source is dropped rather than failing the whole save, so one bad
icon does not cost the operator the rest of their edit.

Availability (#81)

hidden, stock, perPlayerLimit, sort.

  • Hidden products are dropped at the payload boundary, not styled out.
    Filtering in the UI would still ship an unlaunched product's name, price and
    reward list to every visitor, and leave its id buyable. Asserted both ways.
  • Stock decrements inside the same synchronous block as the balance
    deduction, so two requests arriving together cannot both take the last one.
  • Per-player limit counts from the purchase history, and the asking
    player's own count travels in the payload — so the storefront can say "limit
    reached" up front instead of only discovering it when the purchase fails. An
    anonymous visitor is not told anybody's counts.

The storefront (#78, #80, #82)

  • Cards sized around the picture, in an auto-fill grid rather than a fixed
    four columns. A fixed count stretches the card on a wide screen and leaves the
    image inside it looking tiny — the actual complaint.
  • A fixed 16:10 image box, so a 16×16 Minecraft icon and a 1024px render occupy
    the same space instead of one card being three times the height of another.
  • Crates and items in separate sections, order configurable: crates first,
    items first, or one mixed grid. Empty sections are not emitted — a heading
    with nothing under it reads as a bug.
  • An inline SVG crate badge replacing 🎁. An emoji renders as a different
    picture on every platform, cannot be coloured, and carries no accessible name.
  • A detail view: full description, large image, gallery, crate contents with
    odds, stock and limit.
  • A toolbar: search, type filter, five sort orders. Search reaches into a
    crate's reward names, which is how people actually look for one.

The whole storefront is one shared module both pages paste in — the same
arrangement as crateUi.ts, for the same reason: two hand-kept copies of a
store is how the website ended up ignoring the crate animation setting for two
releases.

One judgement call worth stating

A product with no explicit order sorts after every product that has one.
Unset-as-zero is the obvious reading and the wrong one: an operator who numbers
three products 1, 2, 3 and leaves twenty alone means "these three first", and
zero would bury them under all twenty. The desktop field stays blank rather than
defaulting to 0, so saving a product does not silently pin it.

Verification

All ten smoke gates green (MSMS_SMOKE, _EVENTS, _METRICS, _AUDIT,
_ALERTS, _ANALYSIS, _BRIDGE, _WEB, _MODUPDATE, _JAVA).

New coverage:

  • the image allowlist against five legitimate and ten hostile inputs, plus the
    gallery cap
  • upsertProduct dropping a javascript: icon, a data: gallery image and a
    javascript: reward icon
  • a hidden product absent from the payload and unbuyable by id
  • two in stock refusing a third sale, with out-of-stock as the reason and the
    remaining count published
  • a per-player limit that is per player — one buyer hitting theirs does not
    block anyone else — and the owned count reaching the payload only for the
    asking player
  • section ordering both ways, mixed as one section, empty sections dropped, a
    nonsense layout coerced
  • price/featured sorting and search-by-crate-contents
  • the two new routes gated on store (403 for a console-only user), a
    non-image upload refused with 415, and the returned path passing the same
    validator that guards stored ones
  • both served pages rendering the storefront: sections in order, sold-out
    flagged with its buy button disabled, an inline SVG instead of the emoji, the
    detail view carrying the odds, and a hostile icon failing to escape its
    attribute in both the card and the detail view

…sing

Closes #76, #77, #78, #80, #81 and #82.

The store worked, and looked like it had been built to prove it worked. Every
product was a URL in a text box and a 60px icon inside a card three times its
size, in a flat four-column grid with a gift emoji stuck on the end of the
crate names, and no way to click into anything.

**Images (#76, #77).** A product icon can now be pasted as a URL or uploaded
from disk, in the desktop app and the web panel, with a live thumbnail of
exactly what buyers will see, including the broken-image state. Same control
for each crate reward, and for up to eight extra pictures per product shown in
the new detail view.

Uploading from the panel goes to a new store-scoped route rather than the
site's existing one, which is gated on `settings` for the website's own server:
somebody trusted to edit products should not need the keys to the public site
to give one a picture.

All of it goes through one validator. Image sources are attacker-controlled -
any web user with `store` can set one, and it renders for every visitor - so
isSafeImageSrc allows exactly http(s) URLs and /uploads/<plain filename>, and
refuses javascript: (script execution), data: (an inline SVG can carry a
script), protocol-relative //host/x.png (reads like a path, is not one) and any
traversal in the uploads name. Case-insensitively, because a scheme check
defeated by the shift key is not a check. It is enforced in upsertProduct for
the icon, every reward icon and every gallery image, so both admin UIs and the
API pass through the same gate.

**Availability (#81).** hidden, stock, perPlayerLimit and sort. Hidden products
are dropped at the payload boundary, not styled out - shipping one would leak
an unlaunched product's name, price and reward list, and leave its id buyable.
Stock decrements inside the same synchronous block as the balance, so two
requests arriving together cannot both take the last one. The per-player limit
counts from the purchase history, and the asking player's own count travels in
the payload so a storefront can say "limit reached" instead of only discovering
it when the purchase fails.

**The storefront itself (#78, #80, #82).** Cards sized around the picture, an
auto-filling grid instead of a fixed four columns, crates and items in separate
sections whose order is configurable, an inline SVG crate badge replacing the
emoji, a detail view with the full description, gallery and crate contents, and
a toolbar with search, type filter and five sort orders. Search reaches into a
crate's reward names, which is how people actually look for one.

The whole storefront is one shared module the panel and website both paste in,
the same arrangement as crateUi.ts, and for the same reason: two hand-kept
copies of a store is how the website ended up ignoring the crate animation
setting for two releases.

One judgement call worth stating: a product with no explicit order sorts AFTER
every product that has one. Unset-as-zero is the obvious reading and the wrong
one - an operator who numbers three products 1, 2, 3 and leaves twenty alone
means "these three first", and zero would bury them under all twenty.

Verified with all ten smoke gates green. New coverage: the image allowlist
against ten hostile inputs, a hidden product absent from the payload and
unbuyable by id, two in stock refusing a third sale, a per-player limit that is
per player, section ordering, empty sections not emitted as bare headings,
search by crate contents, and both served pages rendering the storefront with a
sold-out flag, an SVG badge instead of the emoji, and a hostile icon failing to
escape its attribute.
Copilot AI review requested due to automatic review settings July 28, 2026 02:45
@CaYatur CaYatur added enhancement New feature or request area:economy Economy / balance / store area:site Public website / storefront labels Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Three findings, and one of them was the verification itself.

**The per-player limit stopped working on exactly the stores that need it.**
ownedCount counted matching rows in st.txns, which is trimmed to the newest
500. Once a store is busy enough for old rows to fall off, a player's record
of buying the once-per-account item goes with them and they can buy it again -
silently, with no error anywhere. Purchase counts now live in their own
productId -> mcName -> count map, incremented in the same atomic block as the
balance and the stock, and untouched by trimming. Existing files are seeded
from whatever history survives; an old store may under-count once at migration,
which beats a limit that keeps drifting forever. Deleting a product drops its
counters, since no new product will reuse the id and the ledger still records
that the purchases happened.

**A gallery thumbnail could break out of its attribute.** The detail view built
onclick="sfShotPick('<url>')" by interpolating the image source, escaped for
the src next to it but not for the handler - a URL carrying a double quote
would close the attribute and the rest would be parsed as markup. It passes the
index now; nothing user-controlled goes near that attribute.

**The smoke suite could report a pass having run nothing.** Chasing a
disappearing WEB gate turned out to be four stray electron processes from an
earlier parallel run: a second instance loses the single-instance lock and
called app.quit(), which exits 0. Every gate went green while executing none of
them, which is worse than a failing test because it looks like a working one.
When any MSMS_SMOKE* variable is set, losing the lock now prints a failure and
exits 1. Verified by holding the lock and watching a second instance exit 1.

Also: the search box lost focus when its last character was deleted, because
re-focus was keyed on the text being non-empty rather than on the user typing;
and a purchase refused for stock or a per-player limit showed a raw
"Error: out-of-stock" on both storefronts, which now say what happened and
reload so a count taken by somebody else updates.

While tightening the page-script harness, its stub fetch returned {} for
everything, so both pages' own bootstrap threw asynchronously and the suite
printed unhandled rejections on every run - the noise that hides a real one.
The stub now answers the shapes those paths destructure, and the run is clean.

All twelve smoke gates pass.
@CaYatur

CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Self review

Three findings, all fixed in 9c2b045. One of them is in the verification itself,
which is the one I'd want caught.

1. The per-player limit stopped working on exactly the stores that need it

ownedCount counted matching rows in st.txns:

return st.txns.filter((t) => t.productId === productId && t.mcName === mcName).length

and forty lines away:

if (st.txns.length > 500) st.txns.length = 500

So once a store is busy enough for old rows to fall off the end, a player's
record of having bought the once-per-account item goes with it and they can buy
it again. Silently — no error, no log line, nothing to notice. The limit fails
open, and it fails open precisely on the busy store that needed a limit in the
first place.

Purchase counts now live in their own productId → mcName → count map,
incremented in the same atomic block as the balance and the stock, and untouched
by trimming. Existing files are seeded from whatever history survives; an old
store may under-count once at migration, which beats a limit that keeps drifting
forever. Deleting a product drops its counters — no new product reuses the id,
and the ledger and txn history still record that the purchases happened. This is
a cache of how many, not the record of what happened.

2. A gallery thumbnail could break out of its attribute

'<img src="'+sAttr(sfImg(sImg))+'" onclick="sfShotPick(\''+String(sImg).replace(/'/g,"\\'")+'\')"/>'

sAttr guards the src. The onclick payload got only single quotes escaped —
a URL carrying a double quote closes the attribute and everything after it
is parsed as markup. isSafeImageSrc permits arbitrary https:// URLs, and a
" in one is perfectly legal.

It passes the index now. Nothing user-controlled goes near that attribute,
which is a better fix than escaping it correctly, because the next person to
touch it cannot get the escaping wrong.

3. The smoke suite could report a pass having run nothing

Worth writing up because it nearly fooled me.

MSMS_SMOKE_WEB started exiting 0 with completely empty stdout. I rebuilt,
re-ran, redirected to a file — nothing. The cause was four stray electron
processes left by an earlier parallel gate run: a second instance loses
requestSingleInstanceLock() and calls app.quit(), which exits 0.

Every gate would have gone green while executing none of them. That is worse
than a failing test, because it looks like a working one — and my whole
verification doctrine here is "the exit code is the signal".

When any MSMS_SMOKE* variable is set, losing the lock now prints a failure and
exits 1. Verified by holding the lock with a background instance and watching a
second one:

second instance exit = 1
SMOKE: FAIL - another instance holds the single-instance lock; nothing ran

Smaller things from the same pass

  • Search box lost focus when you deleted its last character — re-focus was
    keyed on the text being non-empty rather than on the user typing.
  • A purchase refused for stock or a limit showed Error: out-of-stock on
    both storefronts. They now say what happened, and reload, so a count somebody
    else took updates rather than sitting stale.
  • The page-script harness printed unhandled rejections on every run. Its
    stub fetch returned {} for everything, so both pages' own bootstrap threw
    asynchronously. That is exactly the noise a real rejection hides behind. The
    stub answers the shapes those paths destructure, plus
    documentElement/scrollTo, and the run is clean.

Reviewed and deliberately left alone

  • sfRender rebuilds the whole box per keystroke. A catalogue is tens of
    products, not thousands; incremental DOM updates here would be more code and
    more ways to be wrong.
  • Stock is not restored if delivery fails. Delivery queues for an offline
    player rather than failing, and a refund path needs a policy decision (and an
    audit trail) that belongs with Audit: balance administration is not recorded in the audit trail #68, not here.
  • toPublic ships stock to everyone. Deliberate — "3 left" is the point.
  • sanitizeImages drops bad entries instead of rejecting the save. One
    mistyped URL should not cost an operator the rest of their edit; the field
    warns in both UIs before saving.

Verification after the fixes

All twelve smoke gates pass: MSMS_SMOKE, _EVENTS, _METRICS, _AUDIT,
_ALERTS, _ANALYSIS, _BRIDGE, _WEB, _MODUPDATE, _JAVA, _WORLDS,
_WIZARD — and now that is a claim the harness can actually make.

@CaYatur
CaYatur merged commit e0a012e into main Jul 28, 2026
1 check passed
@CaYatur
CaYatur deleted the feat/store branch July 28, 2026 02:56
CaYatur added a commit that referenced this pull request Jul 28, 2026
…t that did not test (#89)

Three loose ends from #87/#88, found reading back over what was merged.

**common.clear existed in neither locale.** ImageField uses it for the clear
button's tooltip, so that button's title read the literal string
"common.clear". TypeScript could not catch it: tr is typed as `typeof en` and
the key was missing from both, so the shapes still matched.

**#80 said the SVG badge replaces the gift emoji "in the storefront and in both
admin product lists", and the web panel's Manage list still had the emoji.** The
storefront and the desktop list were done; that one was not. It uses the same
CRATE_ICON_SVG now.

**The assertion written to catch the first of those did not catch it.** It
scanned document.body.innerText for anything shaped like a translation key,
which sounds right and misses every key used in a title, placeholder or
aria-label - `common.clear` among them, since innerText contains no attributes.
Worse, the clear button only renders once an image field has a value, so an
untouched editor never draws it at all.

Fixed by scanning attributes too and by filling the icon field first, through
the native value setter so React notices. Then proved: with the key removed the
gate now exits 1 with "untranslated keys rendered in the store view:
common.clear", and with it restored it passes. A test that cannot fail is worse
than no test, because it reports coverage that is not there - the same shape as
the single-instance-lock false pass fixed in the previous PR.

The tab sweep already mounts every view, so nothing was crashing; what was
missing was any assertion about what the Store view actually renders. It now
also checks the crate animation picker and image fields are present, so #75 and
#76 have renderer-level coverage rather than only shared-logic coverage.

All twelve smoke gates pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:economy Economy / balance / store area:site Public website / storefront enhancement New feature or request

Projects

None yet

2 participants