HarperFast / HarperFast/harper
Ops-API validation accepts unknown parameters everywhere: the shared validator sets allowUnknown:true, overriding schemas that look closed
- 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
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