CenterForDigitalHumanities / CenterForDigitalHumanities/rerum_server_nodejs
Switch obj.hasOwnProperty() to Object.hasOwn() where the receiver is untrusted
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 6
- Avg merge
- 1h 25m
- Merged PRs (30d)
- 3
Description
## Summary
Eight call sites invoke `hasOwnProperty` as a **method on the object being tested**. RERUM stores arbitrary JSON, so an object under test can carry its own `hasOwnProperty` property, which shadows `Object.prototype.hasOwnProperty` and makes the call fail.
## Why `Object.hasOwn` is immune
It is about which object is the **receiver** of the method lookup, not the method name.
- `obj.hasOwnProperty(k)` resolves `hasOwnProperty` **on `obj`** — own property first, then up the prototype chain. An own key named `hasOwnProperty` shadows the built-in.
- `Object.hasOwn(obj, k)` resolves `hasOwn` **on the `Object` constructor**. `obj` is an ordinary argument and is never consulted to find the function.
```
--- obj.hasOwnProperty("__deleted") (receiver is the record) ---
plain record -> true
record w/ hasOwnProperty -> THROWS: TypeError: obj.hasOwnProperty is not a function
record w/ hasOwn -> true
record w/ fn override -> false <-- silently wrong
--- Object.hasOwn(obj, "__deleted") (receiver is Object) ---
plain record -> true
record w/ hasOwnProperty -> true
record w/ hasOwn -> true
record w/ fn override -> true
```
The failure mode depends on the value. A string throws a `TypeError`. A **function** returning `false` would not throw at all — a deleted record would be reported as live. That second case is unreachable here, because RERUM stores JSON and JSON cannot encode a function, so the realistic worst case is a 500.
## Call sites
| # | Site | Receiver | Provenance | Verdict |
|---|------|----------|-----------|---------|
| 1 | `utils.js:46` | `received_options` | request body | switch — highest priority |
| 2 | `utils.js:72` | `received_options` | request body | switch — highest priority |
| 3 | `utils.js:105` (`isDeleted`) | `obj` | stored record | switch |
| 4 | `utils.js:113` (`isReleased`) | `obj` | stored record | switch |
| 5 | `utils.js:114` (`isReleased`) | `obj.__rerum` | RERUM-minted, fixed shape | consistency only |
| 6 | `controllers/patchUpdate.js:59` | `originalObject` | stored record | switch |
| 7 | `controllers/patchUnset.js:65` | `originalObject` | stored record | switch |
| 8 | `controllers/patchSet.js:63` | `originalObject` | stored record | switch |
### Sites 1 and 2 are a different class from the rest
Sites 3–8 require a poisoned record to already exist in the database. Sites 1 and 2 take their receiver straight from the request:
```
--- utils.js:46/72 configureRerumOptions ---
OK normal create body
BREAKS body w/ __rerum.hasOwnProperty -> TypeError: received_options.hasOwnProperty is not a function
```
`create()` does `provided = structuredClone(req.body)`, and `configureRerumOptions()` clones `provided.__rerum` into `received_options`. `structuredClone` preserves an own `hasOwnProperty` key, so `POST /v1/api/create` with a body of `{"__rerum":{"hasOwnProperty":"x"}}` throws. The call is at `controllers/crud.js:41`, **before** the `try` at line 54, so it escapes as a **500** rather than a `400`. `bulkCreate` has the same shape through `controllers/bulk.js:73`.
Both require a valid token, so this is an authenticated self-inflicted 500 rather than an anonymous availability problem. It is still a 500 on well-formed JSON, which is the wrong answer to give.
`putUpdate.js:115` is **not** affected — it passes `extUpdate=true`, and that branch sets `received_options = {}` before either check runs.
### Site 5 is the only genuine non-issue
`configureRerumOptions()` builds `rerumOptions` from scratch and never spreads caller keys into it, so a stored `__rerum` always has the fixed RERUM shape. `obj.__rerum.hasOwnProperty(...)` only breaks if `isReleased` is handed a hand-made object, and all twelve callers of `isDeleted`/`isReleased` pass a record from `db.findOne`. Worth switching with the others so the next reader does not have to re-derive that it is safe.
## Current impact: none
Measured against production (`annotationStore.alpha`):
```
'hasOwnProperty' present on stored records: 0
'__rerum.hasOwnProperty' present on stored records: 0
'__rerum.history.hasOwnProperty' present on stored records: 0
'__rerum.releases.hasOwnProperty' present on stored records: 0
```
Also checked and absent at the top level: `constructor`, `__proto__`, `toString`, `valueOf`, `isPrototypeOf`, `propertyIsEnumerable`.
Nothing stored today triggers any of this. Sites 1 and 2 do not depend on stored data, so those are reachable now by any client holding a token.
## Suggested fix
Replace each site with `Object.hasOwn(receiver, key)`:
```javascript
// utils.js
const isDeleted = function(obj){
return Object.hasOwn(obj, "__deleted")
}
const isReleased = function(obj){
let bool =
(Object.hasOwn(obj, "__rerum") &&
Object.hasOwn(obj.__rerum, "isReleased") &&
obj.__rerum.isReleased !== "")
return bool
}
```
Eight lines across `utils.js`, `controllers/patchUpdate.js`, `controllers/patchUnset.js`, and `controllers/patchSet.js`. No behavior change for valid data. `Object.hasOwn` is available from Node 16.9, well under the repo's `>=24.14.0` engine floor.
The older equivalent is `Object.prototype.hasOwnProperty.call(obj, k)`, immune for the same reason.
Contributor guide
Assessment
This issue has not been assessed yet.