|
| 1 | +# Security Review Reference |
| 2 | + |
| 3 | +Load this before Step 10. The inline checklist in `SKILL.md` names the |
| 4 | +vulnerability classes; this file gives you the **mental model** for finding |
| 5 | +them and Medusa-specific patterns that are easy to miss. |
| 6 | + |
| 7 | +## The core heuristic: trace tainted input to a dangerous sink |
| 8 | + |
| 9 | +Most real vulnerabilities are the same shape: **user-controlled input reaches |
| 10 | +a sensitive operation without validation in between.** Don't scan for scary |
| 11 | +function names — trace data flow. |
| 12 | + |
| 13 | +1. **Identify the sources** the diff introduces or touches. Anything an |
| 14 | + external caller can influence is tainted: request bodies/params/query, |
| 15 | + headers, uploaded file names and contents, webhook payloads, entity fields |
| 16 | + that were set from any of the above, and values read back from the DB that |
| 17 | + originally came from a user. |
| 18 | +2. **Identify the sinks** in the changed code: filesystem paths, **object |
| 19 | + storage keys / bucket paths**, URLs used in server-side fetches, SQL / query |
| 20 | + filters, shell commands, `eval`/`Function`, dynamic `require`/`import`, |
| 21 | + redirect targets, template rendering, response bodies, and logs. |
| 22 | +3. **Check what happens between source and sink.** Is the input validated, |
| 23 | + allow-listed, normalized, length-capped, or escaped? If a tainted value |
| 24 | + flows into a sink untouched, that is your finding — regardless of how |
| 25 | + "normal" the surrounding code looks. |
| 26 | + |
| 27 | +If you cannot tell from the diff whether a value is tainted, **read the |
| 28 | +callers and the type definition** to see where it originates. A filename or key |
| 29 | +that looks internal is often set directly from an upload request. |
| 30 | + |
| 31 | +## Red flag: a diff that WIDENS what user input reaches a sink |
| 32 | + |
| 33 | +The most commonly missed security bug is not new dangerous code — it is a |
| 34 | +change that **removes an implicit protection**. Watch for: |
| 35 | + |
| 36 | +- Code that previously used only a *sanitized fragment* of an input and now |
| 37 | + uses **more** of it. Example: parsing a filename and previously using only |
| 38 | + the base name + extension, then changing it to also prepend the parsed |
| 39 | + **directory** — that directory is attacker-controlled and can contain `..` |
| 40 | + or leading `/`. |
| 41 | +- Removing a `replace(/[^a-z0-9]/…)`, a `path.basename()`, an allow-list |
| 42 | + check, a length cap, or an `encodeURIComponent`. |
| 43 | +- Loosening a validator/regex, widening an accepted type, or making a |
| 44 | + previously-fixed value configurable from the request. |
| 45 | +- Concatenating a new user-controlled segment into an existing path, key, URL, |
| 46 | + or identifier string. |
| 47 | + |
| 48 | +When you see the diff route more of an input into a path/key/URL/query, stop |
| 49 | +and ask: *"What is the worst string an attacker can put here, and where does it |
| 50 | +end up?"* Then confirm the code rejects or neutralizes that string. |
| 51 | + |
| 52 | +## Path / key traversal — filesystem AND object storage |
| 53 | + |
| 54 | +Path traversal is **not limited to `fs.*` calls.** Any construction of a path, |
| 55 | +key, or identifier from user input is in scope: |
| 56 | + |
| 57 | +- **Filesystem:** `fs.readFile`, `fs.writeFile`, `createReadStream`, |
| 58 | + `path.join(base, userInput)`, static file serving. |
| 59 | +- **Object storage keys** (S3, GCS, R2, etc.): the `Key` / object path passed |
| 60 | + to `PutObject`, `GetObject`, `Upload`, presigned-URL helpers. Cloud SDKs |
| 61 | + treat the key as an opaque string and will happily store `a/../../b`, so |
| 62 | + `..` and leading `/` segments let a caller **escape a configured prefix, |
| 63 | + cross a tenant/namespace boundary, or overwrite another object.** |
| 64 | +- **Cache keys, URL paths, redirect targets, archive entry names** (zip-slip). |
| 65 | + |
| 66 | +What to require in the fix: |
| 67 | +- Reject or strip `..` segments and leading `/` (and their URL-encoded forms |
| 68 | + `%2e%2e`, `%2f`) **before** the value is used to build the path/key. |
| 69 | +- Prefer deriving the safe part explicitly (e.g. `path.basename`, an |
| 70 | + allow-list of characters) over trying to blacklist bad sequences. |
| 71 | +- Confirm a configured prefix / base directory actually **contains** the final |
| 72 | + resolved path — prefixing a string does not prevent `..` from climbing out |
| 73 | + of it. |
| 74 | + |
| 75 | +Concrete pattern that MUST be flagged (this is the shape of a real miss): |
| 76 | + |
| 77 | +```ts |
| 78 | +// filename comes from the upload request (tainted) |
| 79 | +const parsed = path.parse(file.filename) |
| 80 | +const dir = parsed.dir ? `${parsed.dir}/` : "" |
| 81 | +// prefix is meant to isolate this tenant's files |
| 82 | +const key = `${config.prefix}${dir}${parsed.name}-${ulid()}${parsed.ext}` |
| 83 | +// filename "../../other-tenant/logo.png" -> key climbs out of `prefix` |
| 84 | +``` |
| 85 | + |
| 86 | +Finding: *path traversal in the storage key — a crafted `filename` containing |
| 87 | +`..` escapes `config.prefix` and can write outside the intended |
| 88 | +tenant/namespace. Fix: strip `..`/leading-`/` segments (or reject them) before |
| 89 | +building the key.* |
| 90 | + |
| 91 | +## Database injection — beyond raw SQL string concatenation |
| 92 | + |
| 93 | +Raw SQL built from user input (`` `... WHERE id = '${req.params.id}'` ``) is the |
| 94 | +obvious case, but Medusa uses MikroORM and the remote-query layer, which have |
| 95 | +their own injection surfaces. Flag any of these when the value is tainted: |
| 96 | + |
| 97 | +- **Raw MikroORM / Knex queries:** `em.execute(...)`, `knex.raw(...)`, |
| 98 | + `manager.getConnection().execute(...)`, or `.raw(...)` fragments built by |
| 99 | + string interpolation instead of bound parameters (`?` / named bindings). |
| 100 | +- **Query-filter / operator injection:** passing `req.body`, `req.query`, or |
| 101 | + `req.filterableFields` **straight into** a service `.list*()`, repository, or |
| 102 | + `query.graph({ filters })` call without a validator. An attacker can inject |
| 103 | + operators (`$ne`, `$like`, `$or`, `$gt`, …) or filter on unintended columns |
| 104 | + to read rows they should not see, bypass a scope filter, or turn an equality |
| 105 | + check into a broad match. Medusa routes are expected to validate/whitelist |
| 106 | + the request schema (Zod / `validateAndTransformBody`) and pass only known |
| 107 | + fields into the filter object. |
| 108 | +- **Dynamic column / order / table names** from user input (identifiers cannot |
| 109 | + be parameterized) — must be checked against an allow-list. |
| 110 | +- **`$where` / JS-expression style filters** or anything that lets the caller |
| 111 | + supply an expression string. |
| 112 | + |
| 113 | +Fix expectation: use bound parameters for values, an allow-list for |
| 114 | +identifiers, and validate the request into a known shape before it reaches any |
| 115 | +filter or query builder. Never interpolate a tainted value into a query string. |
| 116 | + |
| 117 | +## Output encoding — unescaped JSON, HTML, and prototype pollution |
| 118 | + |
| 119 | +This is the most commonly missed class. A value can be safe in the DB yet |
| 120 | +dangerous when it is **serialized back out** without proper encoding: |
| 121 | + |
| 122 | +- **Unescaped JSON embedded in HTML / a `<script>` context (XSS).** The classic |
| 123 | + bug is interpolating `JSON.stringify(data)` into an HTML string or inline |
| 124 | + script — e.g. `` `<script>window.__STATE__=${JSON.stringify(data)}</script>` `` |
| 125 | + or building an HTML email/template from user data. `JSON.stringify` does |
| 126 | + **not** escape HTML, so a value containing `</script>` (or `<!--`, `]]>`, |
| 127 | + U+2028/U+2029) breaks out of the script/tag and injects arbitrary markup. |
| 128 | + Fix: escape the dangerous characters (`<`, `>`, `&`, U+2028, U+2029) in the |
| 129 | + serialized string before embedding it, or pass the data via a `data-*` |
| 130 | + attribute / DOM API rather than string-concatenating it into HTML. |
| 131 | +- **User input reflected into any HTML/markup response** (server-rendered pages, |
| 132 | + emails, invoices, SVGs, redirects with a reflected param) without escaping → |
| 133 | + XSS/HTML injection. |
| 134 | +- **Building JSON by string concatenation** instead of `JSON.stringify` — a |
| 135 | + quote or brace in the value corrupts or injects into the document. Always |
| 136 | + serialize with `JSON.stringify`, never hand-assemble JSON. |
| 137 | +- **`JSON.parse` on untrusted input without a guard** — wrap in try/catch (a |
| 138 | + malformed body should be a 400, not an unhandled throw), and when the parsed |
| 139 | + object is **merged/assigned** into another object (`Object.assign`, spread, |
| 140 | + deep-merge, `lodash.merge`), treat `__proto__` / `constructor` / `prototype` |
| 141 | + keys as **prototype-pollution** vectors — reject or use a null-prototype |
| 142 | + target. |
| 143 | +- **`Content-Type` mismatch:** returning user-controlled text as `text/html` |
| 144 | + (or omitting the header so a browser sniffs it as HTML) when it should be |
| 145 | + `application/json` / `text/plain`. |
| 146 | + |
| 147 | +## Authorization on data-scoped routes |
| 148 | + |
| 149 | +For any new/changed route or workflow that reads or mutates data belonging to a |
| 150 | +user, store, or customer: |
| 151 | + |
| 152 | +- Is there an ownership/scope check, or does it trust an ID from the request? |
| 153 | +- Is the auth/permission middleware actually applied to the new route? |
| 154 | +- Can a lower-privileged actor reach an admin-only operation? |
| 155 | + |
| 156 | +## Other classes (see SKILL.md Step 10 for the full list) |
| 157 | + |
| 158 | +- **Code execution:** `eval`/`Function`, dynamic `require`/`import` with user |
| 159 | + paths, shell construction from user input. |
| 160 | +- **SSRF:** user-controlled URLs in server-side `fetch`/HTTP calls without an |
| 161 | + allow-list. |
| 162 | +- **DoS:** missing size/length/pagination limits on user input. |
| 163 | +- **Data exposure:** secrets/PII/internal IDs in responses or logs, stack |
| 164 | + traces leaked to clients, hardcoded credentials. |
| 165 | +- **Supply chain:** typosquat packages, `pre/postinstall` scripts, |
| 166 | + lockfile/manifest mismatch. |
| 167 | + |
| 168 | +## Severity and phrasing |
| 169 | + |
| 170 | +- Every confirmed OR plausible security issue is **blocking** — it belongs in |
| 171 | + `blocking_points` with `"requires-more"` in `labels_to_add`, never merely |
| 172 | + mentioned in `summary`. |
| 173 | +- If you are not certain the input is reachable/tainted, still flag it as a |
| 174 | + question (*"Can `file.filename` contain `..`? If so this escapes the |
| 175 | + prefix…"*) and keep it blocking — the author must confirm or disprove. |
| 176 | +- Use `close-malicious` only for deliberate attacks (backdoors, exfiltration), |
| 177 | + never for good-faith bugs like the traversal above — those are |
| 178 | + `needs-changes`. |
| 179 | +- Phrase each finding as: |
| 180 | + *"`<file>:<location>`: `<vuln class>` — `<one-sentence attack scenario>`. Fix: `<concrete fix>`."* |
0 commit comments