Validate severity flag values instead of silently changing what runs - #166
Open
gesposito wants to merge 1 commit into
Open
Validate severity flag values instead of silently changing what runs#166gesposito wants to merge 1 commit into
gesposito wants to merge 1 commit into
Conversation
Contributor
|
@gesposito is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
gesposito
force-pushed
the
fix/validate-severity-flags
branch
from
August 24, 2026 19:19
ff19e9c to
7623506
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The six options that take a severity —
--severity,--min-severity,--only-severity— are now declared with.addOption(new Option(...).choices(SEVERITIES)), so Commander rejects an unrecognized value instead of passing it through to be cast toSeverityand used as a filter key.export --only-severityalready validated by hand, and that now-duplicated check is removed.Why
Only one of the six severity options validated its input. The other five cast the raw string straight to
Severity, and because the value ends up as a lookup key or an equality operand, a typo doesn't fail — it quietly changes which findings the command operates on, then reports success and exits 0.The failure is not even consistent, because each call site compares differently:
triage --severityfinding.severity !== severityTriage complete., exits 0, triages zero findingsrevalidate --min-severityORDER[f] > ORDER[bad]n > undefinedis false, so the filter is disabled and everything is revalidatedenrich --min-severityORDER[f] <= ORDER[bad]n <= undefinedis false, so nothing qualifies and nothing is enrichedexport --min-severityORDER[f] > ORDER[bad]metrics --min-severityORDER[bad] ?? 2MEDIUMexport --only-severityexport.ts:354So the same typo under-includes on two commands, over-includes on two others, and silently picks a tier on a fifth. What we hit in practice:
deepsec triage --severity CRTICALechoed the typo back three times, triaged nothing, printed a greenTriage complete.and exited 0. Nothing in the run record showed it either —toTriage.length === 0returns beforecreateRunMeta, so no run is written at all. Separately,deepsec export --min-severity NONSENSE --only-true-positiveexported all 474 findings rather than erroring.--only-severityproves the intent: unrecognized severities were always meant to be an error, and the check simply wasn't applied to its five siblings.Notes for reviewer
.choices()is Commander's own mechanism for this, so there is no hand-written parser to keep in step with the ladder. It also renders the allowed values into--help, which lets four descriptions drop the parenthetical list they were maintaining by hand — one of which had already drifted:revalidate --min-severitydocumented five of the six severities, omittingLOWeven though it has always been accepted. Using.choices()means that list can no longer go stale..choices()requires.addOption(new Option(...))rather than.option(...), which is a new shape in this file. It seemed worth it for six options that were all getting the same treatment; happy to switch to a sharedparseArgfunction if you'd rather keep.option()throughout.The error is Commander's standard one, naming the offending flag:
error: option '--min-severity <sev>' argument 'NONSENSE' is invalid. Allowed choices are CRITICAL, HIGH, MEDIUM, HIGH_BUG, BUG, LOW.SEVERITIESis declared incli.tsrather than in@deepsec/core, to keep this a validation-only change. Hoisting a canonical ladder into core is #48's job, and it can't ride along here: the six localSEVERITY_ORDERmaps have drifted (packages/processor/src/enrich.ts:156andpackages/deepsec/src/commands/export.ts:9rankHIGH_BUGaboveMEDIUM;packages/deepsec/src/sandbox/partitioner.ts:8omitsLOWentirely), so unifying them changes whatexport --min-severity MEDIUMreturns. That is user-visible and belongs in its own PR.Two small things came along because they are the same lines:
export.ts's hand-rolled check is now unreachable, since Commander rejects the value beforeexportCommandruns; andtriage --helpno longer prints its default twice (Severity to triage (default: MEDIUM) (default: "MEDIUM")), because the description no longer restates what Commander already renders.The test is an e2e one rather than a unit test, since with
.choices()there is no unit left to test — the regression worth guarding is the wiring. It exercises all six flags through the built bundle; reverting any single option to a bare.option()fails it with that flag named.Not addressed here, and arguably the real ergonomic gap:
triagetakes only an exact tier, so covering the ladder means six sequential runs. Adding--min-severitytotriageis a feature, not this bug, so it is left out.Verification
pnpm testpasses — 2411 passed, 1 skipped, 63 filespnpm lintpassespnpm knippassespnpm -r buildpassespnpm test:bundlepasses — 30 testspnpm typecheck:deepsecpasses