From b084bba86fe9d394d6a5ea287161864e7695a609 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 11:35:21 +0200 Subject: fix(cli): guard publish's IO, validate --dtstamp, and list all commands publish's own mkdir_p/write_file (unlike every other IO path on this branch) were unguarded: an unwritable --out parent raised a bare Unix.Unix_error(EACCES,...) and an --out naming an existing file raised ENOTDIR, both as uncaught exceptions with a stack trace rather than the project's one-line "colitur: ..." form. The same defect class commit 6bd741b already fixed once for template reads -- --out is user input too. Fixed by wrapping the whole publish_report call (not each write_file site) in one handler for Unix.Unix_error and Sys_error, mirroring why that earlier fix guarded the whole read and not only the open. Added a cram case using a read-only directory inside the test's own cram sandbox, not /tmp, so a failed cleanup cannot leave an unwritable directory behind in a shared location. --dtstamp was the only user string reaching output unescaped and unvalidated: "--dtstamp hello" silently emitted an invalid "DTSTAMP:hello", and a value carrying its own CRLF injected extra lines into every VEVENT. Fixed by rejecting anything not matching RFC 5545's UTC form (8 digits, 'T', 6 digits, 'Z') before either emit or publish does anything else, one line to stderr, exit 2. usage() was byte-unchanged from before the branch and listed only the six pre-existing commands, omitting all four commands this branch added (emit, table, render, publish). Added them; the three cram pins of the exact usage string are updated to match. --- bin/main.ml | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- test/cli.t | 43 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/bin/main.ml b/bin/main.ml index 6f3cbd5..6f64582 100644 --- a/bin/main.ml +++ b/bin/main.ml @@ -389,7 +389,38 @@ let readings_report ~overlays y = resolved_year_report ~line:readings_line ~over file; XML: one document per year, the schema's own root is a single year; ICS: one VCALENDAR per year, valid to concatenate for a subscriber that reads multiple files). *) +(* [--dtstamp] is the only user string that reaches [emit]/[publish] output + unescaped and unvalidated (it becomes an ICS DTSTAMP: property value + directly, Colitur_render.Emit_ics.year's own [dtstamp] parameter) -- + every OTHER interpolated value in this project's output is either + escaped (Escape.apply) or engine-computed, never raw user input placed + straight into a line-oriented format. RFC 5545 section 3.3.5 defines + DATE-TIME's UTC form as exactly 8 digits, "T", 6 digits, "Z" + (e.g. "20270101T000000Z"); rejecting anything else is what stops + "--dtstamp hello" from silently emitting a malformed "DTSTAMP:hello" AND + what stops a value carrying its own CRLF (e.g. "X\r\nBEGIN:VEVENT\r\n...") + from being injected verbatim into every VEVENT -- a value shaped exactly + like the real form cannot contain either character. *) +let dtstamp_well_formed s = + let is_digit c = c >= '0' && c <= '9' in + String.length s = 16 + && String.for_all is_digit (String.sub s 0 8) + && s.[8] = 'T' + && String.for_all is_digit (String.sub s 9 6) + && s.[15] = 'Z' + +let check_dtstamp = function + | None -> () + | Some s when dtstamp_well_formed s -> () + | Some s -> + Printf.eprintf + "colitur: --dtstamp %S is not RFC 5545 UTC form (want 8 digits, 'T', 6 digits, 'Z', e.g. \ + 20270101T000000Z)\n" + s; + exit 2 + let emit_report ~format ~overlays ~dtstamp ~from_y ~to_y = + check_dtstamp dtstamp; if from_y > to_y then begin Printf.eprintf "colitur: --from %d is after --to %d\n" from_y to_y; exit 2 @@ -695,6 +726,7 @@ let index_html ~from_y ~to_y = Buffer.contents b let publish_report ~from_y ~to_y ~out ~overlays ~dtstamp ~prune = + check_dtstamp dtstamp; if from_y > to_y then begin Printf.eprintf "colitur: --from %d is after --to %d\n" from_y to_y; exit 2 @@ -978,7 +1010,9 @@ let print_help () = let usage () = prerr_endline "colitur: usage: colitur easter | colitur temporal | colitur day | colitur \ - readings | colitur check FILE | colitur new-overlay (try: colitur --help)"; + readings | colitur emit --format FMT --from Y --to Y | colitur table --year Y --template \ + FILE | colitur render --template FILE --year Y | colitur publish --from Y --to Y --out DIR | \ + colitur check FILE | colitur new-overlay (try: colitur --help)"; exit 2 let with_year ys f = @@ -1383,5 +1417,27 @@ let () = exit 2 | Some from_ys, Some to_ys -> with_year from_ys (fun from_y -> - with_year to_ys (fun to_y -> publish_report ~from_y ~to_y ~out ~overlays ~dtstamp ~prune)))) + with_year to_ys (fun to_y -> + (* [publish_report] writes many files across a whole + year range ([mkdir_p]/[write_file], both above) -- + an unwritable [--out] parent (EACCES) or an [--out] + that names an existing plain file (ENOTDIR) raises + from deep inside that loop, same defect class as + the template read this project already guards + (commit 6bd741b): "--out" is user input too, and + the WHOLE call is guarded here rather than each + [write_file] site individually, for the same + reason that fix guarded the whole read and not + only the open. [Unix.mkdir] raises + [Unix.Unix_error] directly; [open_out_bin] + (stdlib, not the Unix module) wraps the same + underlying errno in [Sys_error] instead -- both + are real on this path, so both are caught. *) + try publish_report ~from_y ~to_y ~out ~overlays ~dtstamp ~prune with + | Unix.Unix_error (e, fn, arg) -> + Printf.eprintf "colitur: %s: %s: %s\n" fn arg (Unix.error_message e); + exit 2 + | Sys_error e -> + Printf.eprintf "colitur: %s\n" e; + exit 2)))) | _ -> usage ()) diff --git a/test/cli.t b/test/cli.t index a4fa696..80c8078 100644 --- a/test/cli.t +++ b/test/cli.t @@ -17,7 +17,7 @@ A year outside the supported domain is rejected (exit 2): No/garbage arguments give a usage error (exit 2): $ colitur - colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur check FILE | colitur new-overlay (try: colitur --help) + colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur emit --format FMT --from Y --to Y | colitur table --year Y --template FILE | colitur render --template FILE --year Y | colitur publish --from Y --to Y --out DIR | colitur check FILE | colitur new-overlay (try: colitur --help) [2] The EF temporal cycle for a year, one line per day: @@ -327,14 +327,14 @@ A flag needing a value, given none: $ colitur day 2026 --overlay colitur: --overlay needs a file path - colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur check FILE | colitur new-overlay (try: colitur --help) + colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur emit --format FMT --from Y --to Y | colitur table --year Y --template FILE | colitur render --template FILE --year Y | colitur publish --from Y --to Y --out DIR | colitur check FILE | colitur new-overlay (try: colitur --help) [2] An unknown option is rejected rather than treated as a positional word: $ colitur day 2026 --diocese colitur: unknown option --diocese - colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur check FILE | colitur new-overlay (try: colitur --help) + colitur: usage: colitur easter | colitur temporal | colitur day | colitur readings | colitur emit --format FMT --from Y --to Y | colitur table --year Y --template FILE | colitur render --template FILE --year Y | colitur publish --from Y --to Y --out DIR | colitur check FILE | colitur new-overlay (try: colitur --help) [2] The shipped example overlay is runnable documentation, and it must actually @@ -464,6 +464,28 @@ An unknown format is a usage error on stderr, exit 2: colitur: unknown format "yaml" (want csv, json, sexp, xml or ics) [2] +--dtstamp is the only user string that reaches ICS output unescaped and +unvalidated -- it must be exactly RFC 5545's UTC DATE-TIME form (8 digits, +"T", 6 digits, "Z") or refused outright, rather than either silently +emitting a malformed DTSTAMP or, worse, letting an embedded CRLF inject +extra lines into every VEVENT: + + $ colitur emit --format ics --from 2027 --to 2027 --dtstamp hello + colitur: --dtstamp "hello" is not RFC 5545 UTC form (want 8 digits, 'T', 6 digits, 'Z', e.g. 20270101T000000Z) + [2] + + $ colitur emit --format ics --from 2027 --to 2027 --dtstamp "$(printf 'X\r\nBEGIN:VEVENT\r\nUID:evil')" + colitur: --dtstamp "X\r\nBEGIN:VEVENT\r\nUID:evil" is not RFC 5545 UTC form (want 8 digits, 'T', 6 digits, 'Z', e.g. 20270101T000000Z) + [2] + +A well-formed value is threaded through unchanged. (The events themselves +end in CRLF per RFC 5545 -- match the substring, not a `$`-anchored full +line, or a shell that does not mangle the trailing "\r" is doing the +grep-anchor's job for it by accident.) + + $ colitur emit --format ics --from 2027 --to 2027 --dtstamp 20270101T000000Z | grep -c 'DTSTAMP:20270101T000000Z' + 365 + emit refuses a reversed range rather than emitting nothing: $ colitur emit --format csv --from 2028 --to 2027 @@ -993,6 +1015,21 @@ publish never deletes a file it does not own: colitur: publish requires --out DIR [2] +publish writes many files across a whole year range (mkdir_p/write_file), +same as the template read guarded in commit 6bd741b -- --out is user input +too, and an unwritable parent used to surface as an uncaught +Unix.Unix_error instead of the project's one-line form. The read-only +directory below lives in this test's own cram sandbox, not /tmp: a failed +`rm -rf` of an unwritable directory would otherwise leave it behind in a +shared location, so it is restored to writable before the test ends either +way: + + $ mkdir ro-parent && chmod 555 ro-parent + $ colitur publish --from 2027 --to 2027 --out ro-parent/sub + colitur: mkdir: ro-parent/sub: Permission denied + [2] + $ chmod 755 ro-parent + publish's own flags have no effect on the other commands, and other commands' flags have no effect on publish -- refused rather than silently ignored, the same discipline as everywhere else: -- cgit v1.3