CenterForDigitalHumanities / CenterForDigitalHumanities/rerum_server_nodejs

Switch obj.hasOwnProperty() to Object.hasOwn() where the receiver is untrusted

Open
#290 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.