Skip to content

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

Description

@dawsontoth

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:

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:254add_certificate:

is_authority: Joi.boolean().required(),

read for truthiness at :292 and :325:

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: 4security/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 placeObject.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 Disk-space safety thresholds: reclamation / write-stop / hard-shutdown #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, #2199.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:securitySecurity, TLS/certs, authentication, authorizationbugSomething isn't working

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions