HarperFast / HarperFast/harper

validateBySchema discards joi's coerced value, so every non-strict Joi.boolean() treats the string "false" as true

Open
#2,200 0 comments 0 reactions 0 assignees View on GitHub
area:security bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.