HarperFast / HarperFast/harper

Ops-API validation accepts unknown parameters everywhere: the shared validator sets allowUnknown:true, overriding schemas that look closed

Open
#2,137 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

Every operation validated through the shared wrapper **silently accepts unknown parameters**. A misspelled, misremembered, or removed parameter is ignored rather than rejected, and the request returns 200.

## Root cause

`validation/validationWrapper.ts:93-99`:

```ts
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);
}
```

`allowUnknown: true` overrides Joi's default (which *does* reject unknown keys) for every caller. Eleven validators route through it, including several on destructive or data-selecting paths:

`analyticsValidator` · `searchValidator` · `deleteValidator` · `bulkDeleteValidator` · `insertValidator` · `transactionLogValidator` · `readLogValidator` · `fileLoadValidator` · `installValidator` · `statusValidator` · `configValidator`

## Concrete instance found by QA

`get_analytics` accepts a `bucket_ms` parameter and silently drops it. `bucket_ms` **does not exist anywhere in harper or harper-pro** — `grep` returns 0 hits in both repos. `getOp` (`resources/analytics/read.ts`) forwards only the schema's named fields to `get()`, so the caller gets un-bucketed data and a 200, with nothing indicating the parameter was ignored.

## The schema comment makes this worse

`validation/analyticsValidator.ts` builds `Joi.object({...}).strict()` with this comment:

```ts
// `.strict()` disables Joi's default type coercion so a numeric string like
// '1779834663816' is rejected for a `Joi.number()` field instead of being
// silently converted. Strictness propagates to child schemas.
```

That is accurate about coercion, but a reader reasonably concludes the schema is strict about *shape* too. It isn't — the wrapper's `allowUnknown: true` overrides it. A schema that looks closed and is open is worse than one that looks open.

## Why this is more than cosmetic

For a read like `get_analytics` the cost is a confusing result. The concern is the **data-selecting and destructive** validators on that list: if a condition/filter key can be misspelled and silently ignored rather than rejected, an operation can match a different row set than the caller intended. `deleteValidator` and `bulkDeleteValidator` are the ones worth assessing first.

I have **not** measured whether a typo'd condition key on delete/search actually widens the match — that depends on how each op consumes its conditions downstream, and it should be checked per-op rather than assumed. Flagging it as the reason this deserves more than a "nice to fix".

## Suggested direction

Default to rejecting unknown keys, and let individual schemas opt in where a pass-through is genuinely wanted:

```ts
schema.validate(object, { allowUnknown: false, abortEarly: false, ... })
```

That will surface callers currently relying on the permissiveness — which is the point, but it means the change wants a survey of the ops API's real traffic before flipping, and possibly a deprecation window that warns on unknown keys before rejecting them. Worth deciding whether the default flips globally or per-validator.

## Verification

Source-confirmed on `main` (`eb702ee52`): the `allowUnknown: true` line, the 0-hit grep for `bucket_ms`, and the validator list above. The `get_analytics` 200-with-dropped-param behaviour is from the original exploratory-QA measurement, not re-run here.

Found by exploratory QA (narrow `bucket_ms` finding); the general cause was identified while adjudicating it during a queue triage pass.

Contributor guide

Open the contributing guide

Research direction

Read validation/validationWrapper.ts:93-99 and validation/analyticsValidator.ts, then trace the listed validators and resources/analytics/read.ts. Run the stated grep for bucket_ms and assess how deleteValidator and bulkDeleteValidator consume condition keys before deciding whether the behavior should change globally or per validator. Done means the affected callers and intended unknown-key policy are verified.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
api, backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.