Repository navigation
print.mjs: reject option values it cannot use - #140
rudycelekli wants to merge 1 commit into
Conversation
--out and --format read the next argv entry without checking that it exists, so a flag left last silently became undefined and fell through the ?? chain: a bare --format printed the spec's format, or poster, and exited 0. The unknown-format check also ran only after the spec module had been imported, so a misspelled format surfaced behind an unrelated error. Validate both flags, and the --format value, immediately after the parse loop. The later check stays, because it still guards a format coming from the spec itself.
rahul05ranjan
left a comment
There was a problem hiding this comment.
Verified this end to end on a clean checkout.
The premise is real: at main, scripts/print.mjs swallows a bare --format (argv[++index] gives undefined, line 225 falls through), the spec loads, and line 234's options.format ?? spec.format ?? "poster" silently picks a fallback — and the misspelled-format check at line 235 only runs after both the spec import and the built-page check, so --format sepia dies with no built page before it ever mentions the bad flag. I reproduced both on main with a minimal spec: the bare flag reaches the page-build error, sepia reaches the same error.
At this branch the behavior is what the changelog promises:
print.mjs /tmp/sheet-probe.mjs --format→--format needs a valueat line 221, before the spec import.--format sepia→unknown format "sepia"at line 232, also before the spec import.node --test scripts/print-options.test.mjspasses (1/1). Nice touch that the test drives a nonexistent spec file — it actually proves the errors fire before the spec read rather than after.
The --out guard via the shared valueOf is the right shape; no other flag in that loop takes a value.
One nit: the unknown-format message suggests use one of ${FORMATS.join(", ")} or all, but all is handled before the loop at line 231 rather than being a member of FORMATS — correct as written, just worth knowing the list in the message never contains all itself, so the tail or all is doing the work.
From a security standpoint this is a fail-fast correctness fix rather than a boundary issue — print.mjs runs locally with trusted argv — but validating before the spec import also means a crafted spec module is no longer imported when the invocation itself is malformed, which is a reasonable side benefit.
What this changes
--outand--formatnow require their value, and an unknown--formatis rejected right after the argument loop, before the spec module is imported.Why
Both flags read
argv[++index]without checking that it exists. A flag left last becameundefinedand fell through the??chain, so the existing rejection in the same file —unknown format "..."; use one of poster, drawing, detailed or all— was bypassed by the one input that most looks like a mistake.On
main:No shipped spec sets
format, so a bare--formatalways prints theposterdefault and exits 0: the sheet is not the one asked for and nothing says so.Separately, the unknown-format check ran after
await import(specFile), so a misspelled format on a figure whose light page is not built was reported as the missing page, or as a module error, rather than as the typo it was.The later check is kept, because it still guards a
formatcoming from the spec itself.This is the same fix already recorded under Unreleased for
capture.mjs, and matchesdrive.mjs,lines.mjs,turn-check.mjsandturn-bench.mjs.Tests
scripts/print-options.test.mjsIt fails on
mainby name (# pass 0 # fail 1) and passes here (# pass 1 # fail 0).node --test scripts/print-sheet.test.mjs scripts/print-options.test.mjsis# tests 12 # pass 12 # fail 0.Valid input still passes validation and is unaffected:
--format poster|drawing|detailed|all,--out <name>, and no--formatat all.I did not run the full
scripts/*.test.mjssuite (it needs Chrome and the temp react/typescript install). The change touches onlymain()'s argument parsing inprint.mjs, which no other module imports.Checks
git status --shortshows only the changes I meant. (No example or kit change;git status --shortshows onlyscripts/print.mjs,scripts/print-options.test.mjsandCHANGELOG.md.)kit/change, and no committed page changes.node scripts/lines.mjs. Not applicable: no figure change.UnreleasedinCHANGELOG.md.Screenshots
No visual change and no committed page changes, so there is nothing to screenshot.