ReviewOS

stacks/bun-query-builder

toArray() does not apply hidden: true, so password/token attributes serialize in full

#1182
Closed glennmichael123 opened this 8 days ago ·
8 days ago

hidden: true is honoured by toJSON() and ignored by toArray(), which sits beside it and spreads _attributes directly.

Reproduction

A model with two secrets marked hidden:

attributes: {
  id:        { validation: { rule: {} } },
  email:     { validation: { rule: {} } },
  password:  { validation: { rule: {} }, hidden: true },
  api_token: { validation: { rule: {} }, hidden: true },
}
toJSON()        : {"id":1,"email":"a@b.c"}
toArray()       : {"id":1,"email":"a@b.c","password":"bcrypt$hash","api_token":"sk_live_abc"}
getAttributes() : {"id":1,"email":"a@b.c","password":"bcrypt$hash","api_token":"sk_live_abc"}
JSON.stringify(model) : {"id":1,"email":"a@b.c"}

toJSON() and JSON.stringify are safe. toArray() is not.

Why this is likely to bite

toArray() recurses into relations with toArray() (orm.ts:1725-1729), so an eager-loaded with('user') carries the parent's secrets too — the same propagation shape as #1093, which fixed hidden attributes leaking through with() payloads.

It is also a convention violation. This API mirrors Laravel's — toArray, toJSON, hidden are its names — and there toArray() applies $hidden (toJSON is implemented in terms of toArray). Someone carrying that expectation over will reach for toArray() in a response serializer and get the password hash. The two methods sitting next to each other with opposite behaviour is the trap.

Root cause

orm.ts:1720:

toArray(): Record<string, unknown> {
  const values: Record<string, unknown> = { ...this._attributes }
  // ... relations, also via toArray()

versus orm.ts:1738, which builds the hidden set and filters.

Suggested fix

Have toArray() apply the same hidden set, and let toJSON() delegate to it — matching Laravel, and removing the chance of the two drifting again. The filter in toJSON is already written; it just needs to move down.

getAttributes() is the open question. It also returns everything, but unlike toArray() it reads as an explicit "give me the raw attributes" accessor, and redacting it would break legitimate internal use such as isDirty/diffing. I would leave it and document it as unredacted, but it should be a decision rather than an oversight.

Scope note

Not a SQL-level issue and unrelated to #1181 — the data is correctly fetched, and only the serialization boundary is wrong. Filing separately for that reason.

Happy to send the patch.

Sign in to comment on this issue.