Repository navigation
🔒️ Check the ids and versions in bulk and query requests - #742
shreyanshj10 wants to merge 1 commit into
Conversation
alecgibson
left a comment
alecgibson
left a comment
There was a problem hiding this comment.
This is pretty much what I had in mind for #736, and pulling the checks into isInvalidName() / isInvalidVersion() makes _checkRequest() much easier to read. I see two real gaps, both inline below. b: null still gets through. Single-doc f/s versions are still unchecked, and I'd like to fold those in here. Beyond that, a couple of comment nits.
Nits (personal preference)
lib/agent.jsL378–380 (and L389–390, L405–406): Do we need these comments? The functions read clearly enough on their own. The bulk and query ones also describe the bug rather than the code: after this change, the ids aren't "handed to the database adapter as they arrive", and they aren't passed "straight through" either. I'd put that why in the commit body, which is empty at the moment, so it's discoverable viagit blameand can't drift here.test/agent.jsL70–71: (Small one) "the only way" isn't quite true, sinceObject.defineProperty()does it too. The surprising part is that an object literal would set the prototype instead of creating a key. Maybe say just that?
Generated by Claude Code via /alec-review. Patterns derived from Alec's historical review comments — review with that in mind.
| // Bulk requests carry either an array of ids, or a map of id to version, and | ||
| // both are handed to the database adapter as they arrive. | ||
| function checkBulkData(data) { | ||
| if (typeof data !== 'object') return 'Invalid bulk subscribe data'; |
There was a problem hiding this comment.
typeof null === 'object', so b: null still passes this check, and for…in over null iterates nothing. With bf, we end up calling getOpsBulk('dogs', null) and replying with a success. With bs, the client gets a raw Cannot convert undefined or null to object from Object.keys() in Backend#subscribeBulk(). Could we add null as a b value to the test table too? At the moment it's only covered as an id or a version.
| if (typeof data !== 'object') return 'Invalid bulk subscribe data'; | |
| if (!data || typeof data !== 'object') return 'Invalid bulk subscribe data'; |
| if (request.a === ACTIONS.op || request.a === ACTIONS.presence) { | ||
| if (request.v != null && (typeof request.v !== 'number' || request.v < 0)) return 'Invalid version'; | ||
| if (request.v != null && isInvalidVersion(request.v)) return 'Invalid version'; | ||
| } |
There was a problem hiding this comment.
Should we drop the op/p guard and check v on every doc request? Right now {a: 'f', c: 'dogs', d: 'fido', v: {$gt: -1}} (and the s equivalent) passes exactly that to getOps() as from. That's the same shape we now reject in a bulk version map. I know the description calls this out of scope, but rejecting a non-numeric bulk version is the same behaviour change, just through a different action. Real clients only ever send doc.version here, which is a number or null, and nf versions are already validated on the client, so I don't think any legitimate client would notice. (Worth adding f/s cases to the tests too.)
| if (request.a === ACTIONS.op || request.a === ACTIONS.presence) { | |
| if (request.v != null && (typeof request.v !== 'number' || request.v < 0)) return 'Invalid version'; | |
| if (request.v != null && isInvalidVersion(request.v)) return 'Invalid version'; | |
| } | |
| if (request.v != null && isInvalidVersion(request.v)) return 'Invalid version'; |
|
coverage: 97.819% (+0.03%) from 97.785% — shreyanshj10:fix/issue-736-bulk-id-types into share:main |
This branch has not been deployed
3 participants
GHSA-p56x-m2v9-77x3 type-checked
canddon the snapshot fetch actions, butthe ids and versions carried by bulk requests (
b) and by a query'sreconnect payload (
r) are still handed to the database adapter as theyarrive, so a client can send an object, a number or
__proto__where a documentid is expected, or a non-numeric version.
I pulled the string and version checks already in
_checkRequest()out intoisInvalidName()andisInvalidVersion()so the single-doc, bulk and querypaths share one validator, then applied them to:
barray,bversion map,[id, version]pairs in a query'srpayload.A malformed entry now comes back as
Invalid idorInvalid versionbeforefetchBulk(),getOpsBulk()or the query runs, and anrpayload that isn'tan array of arrays comes back as
Invalid query results.I deliberately left the single-doc
f/sversion alone: it is onlytype-checked for
opandptoday, and widening that is a behaviour changebeyond this issue.
Tests
test/agent.jsgains arequest validationblock that sends each malformedmessage over a real socket and asserts both the error and that
getSnapshotBulk(),getOpsBulk()andquery()were never called.npm testand
npm run lintpass.Fixes #736