ReviewOS

also looking at this

stacks/bun-query-builder

fix(security): toArray() now applies hidden, and toJSON() delegates to it

#1184
Merged glennmichael123 wants to merge fix/toarray-hidden into main
2 files +126 -24
9 days ago

Closes #1182.

hidden: true was honoured by toJSON() and ignored by toArray(), which sits directly beneath it:

before
  toJSON()  {"id":1,"email":"a@b.c"}
  toArray() {"id":1,"email":"a@b.c","password":"bcrypt$hash","api_token":"sk_live_abc"}

after
  toJSON()  {"id":1,"email":"a@b.c"}
  toArray() {"id":1,"email":"a@b.c"}

Why it was likely to bite

Two serializers side by side with opposite behaviour. This API takes its names from Eloquent, where toArray() applies $hidden and toJSON() is defined in terms of it — so the expectation someone arrives with is the wrong one, and the way they find out is a password hash in a response body.

toArray() also recurses into relations with toArray(), so an eager-loaded relation carried the related model's secrets into the parent payload. Same propagation shape as #1093, which fixed hidden attributes leaking through with().

The fix

toArray() applies the hidden filter, and toJSON() delegates to it rather than keeping a second copy. One implementation, so they cannot drift apart again — they already had.

getAttributes() still returns everything, and is now documented as the raw accessor: dirty-tracking and diffing need the whole row. That one is deliberate, which is the difference between it and this bug.

Verification

  • 6 new tests, mutation-checked — emptying the hidden set fails 4 of them
  • Covers both serializers, JSON.stringify(model), a substring assertion that the secret appears nowhere in the payload, the relation-propagation path, and that getAttributes() is still raw
  • tsc and pickier clean (one pre-existing brace-style warning at orm.ts:515, outside this change)

A note on the suite

My first full-suite run showed 40 failures, all mine: the new test file called setConfig at module top level, and config is process-wide, so it leaked a sqlite :memory: dialect into every file that ran after it. It turned out not to be needed at all — make() builds an instance without touching a database — so it is gone.

Worth recording because the same hazard is documented in config.ts and bit me anyway.

After removing it: 2462 pass. Both full runs on this branch show a handful of CLI tests timing out at ~5000ms; clean main shows five of the same class on the same command, and each passes in isolation, so they are load flakes rather than regressions.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.