HarperFast / HarperFast/harper
validateBySchema discards joi's coerced value, so every non-strict Joi.boolean() treats the string "false" as true
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
`validateBySchema` validates a value and then throws away the validated result, returning only the error. Because joi's default `convert: true` *coerces* rather than rejects, every schema field whose runtime type joi would have normalized is left un-normalized on the caller's object — while validation reports success.
`core/validation/validationWrapper.ts:93-99`:
```js
export function validateBySchema(object, schema) {
let result = schema.validate(object, { allowUnknown: true, abortEarly: false, errors: { wrap: { label: "'" } } });
if (result.error) {
return new Error(result.error.message);
}
// result.value — the coerced object — is discarded
}
```
`Joi.boolean()` is the sharpest case: it *accepts* the strings `'true'`, `'false'`, `'TRUE'`, `'FALSE'` by coercing them, so they pass validation and the handler then reads the raw string. `'false'` is truthy in JS, so **any handler that branches on truthiness treats `'false'` as `true`**. Verified against the repo's joi 17.13.4:
```
'false' → validation error: none | joi's coerced value: false | object retains "false" | truthy: true
'TRUE' → validation error: none | joi's coerced value: true | object retains "TRUE" | truthy: true
1, 0, null, {}, [], 'yes', '' → rejected
```
## Confirmed instance with a security-relevant outcome
`harper-pro/security/certificate.ts:254` — `add_certificate`:
```ts
is_authority: Joi.boolean().required(),
```
read for truthiness at `:292` and `:325`:
```ts
if (!is_authority && !private_key && !matchingKeyFound)
throw new ClientError('A suitable private key was not found for this certificate');
...
if (!is_authority || (is_authority && existingPrivateKeyName) || (is_authority && private_key)) {
```
`add_certificate` with `is_authority: "false"` therefore takes the **CA** branches even though the caller explicitly said it is not a CA: the "non-CA certs must have a private key" guard at `:292` is skipped, and `:325`'s `private_key_name` attachment behaves as for a CA. A caller declining a certificate authority gets one. (super_user-gated, and adjacent to the already-ticketed CORE-3069 sub-CA work.)
The instance that prompted this was the same shape in `add_ssh_key`: `generate: "false"` minted an SSH keypair the caller declined — fixed in HarperFast/harper-pro#594 by adding `.strict()` and comparing `=== true`, which is a per-site workaround for a wrapper-level problem.
## Scope
Non-strict `Joi.boolean()`, excluding the ones already carrying `.strict()`:
- **core: 15** (6 already strict — e.g. `dataLayer/schemaDescribe.ts`'s `exact_count`, `skip_record_count`, `include_computed`, which shows the idiom is already the established fix)
- **harper-pro: 4** — `security/certificate.ts:254` (`is_authority`, above), `replication/setNode.ts:19,20,26` (`verify_tls`, `replicates`, `isLeader`)
Each needs checking individually: a field that is only ever passed through, stored, or strictly compared is fine; one that is branched on for truthiness is a live bug. The same reasoning applies to any other type joi coerces (`Joi.number()` accepting `'42'`, `Joi.date()` accepting a string), though boolean is the only one where the coercion flips a *decision* rather than just leaving a differently-typed value.
## Fix options
1. **Return the coerced object from `validateBySchema` and have callers use it.** Correct root fix, but it changes the wrapper's contract at every call site, and any caller that mutates the original object afterwards (`add_ssh_key` sets `req.key` and relies on that same object reaching `replicateOperation`) needs care.
2. **Mutate in place** — `Object.assign(object, result.value)` before returning — so existing callers get normalized values with no signature change. Smaller blast radius, but it makes the wrapper mutating, which is a surprise of its own.
3. **Default the wrapper to `convert: false`**, so a wrong-typed value is rejected rather than silently coerced-then-discarded. Strictest and arguably most correct given the wrapper never uses the conversion, but it will reject inputs some clients send today (form-encoded and query-string callers in particular) — needs an audit before it could ship.
4. **Leave the wrapper, add `.strict()` per field.** What #594 did. Works, but it is opt-in, so the next non-strict boolean reintroduces the bug.
(1) or (2) plus a lint rule against bare `Joi.boolean()` would stop this recurring by construction.
## Provenance
Found during a `deep-review` of HarperFast/harper-pro#594 (server-side SSH keygen), where three of four review agents independently hit the `generate: "false"` case; the wrapper-level generalization and the `is_authority` instance were confirmed afterwards by reading the code and testing against the installed joi. Cross-ref: HarperFast/harper-pro#594, HarperFast/harper#2199.
Contributor guide
Research direction
Start with core/validation/validationWrapper.ts:93-99 and trace how its result is used at callers, especially harper-pro/security/certificate.ts and the listed replication fields. Audit the non-strict Joi.boolean() fields and compare their validation output with the values handlers read. Done means choosing and implementing a consistent wrapper or field-level fix without breaking callers, including the is_authority and generate cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- nodejs, typescript
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100