|
1 | | -# CCFE — technical debt backlog |
2 | | - |
3 | | -Actionable work packages distilled from the M8 close-out audit (`M8-AUDIT.md`). |
4 | | -Each is self-contained and issue-ready: scope, steps, acceptance criteria, |
5 | | -effort and dependencies. Ordered by recommended priority. |
6 | | - |
7 | | -Effort key: **S** ≈ <½ day, **M** ≈ 1–2 days, **L** ≈ several days. |
8 | | -All work must keep the suite green and the four CI checks passing |
9 | | -(`perl -c`, `prove -lr t/`, `perlcritic src/lib`, perltidy on `src/lib`). |
10 | | - |
11 | | -| ID | Title | Dimension | Priority | Effort | Depends on | Status | |
12 | | -|----|-------|-----------|----------|--------|-----------|--------| |
13 | | -| TD-1 | RESTRICTED-mode hardening | security | **high** | L (4 sub-tasks) | — | ✅ done | |
14 | | -| TD-2 | Close the pty test-coverage gaps | coverage | high | M | — | ✅ done | |
15 | | -| TD-3 | Break up the oversized `ccfe.pl` subs | quality | med | L | TD-2 | ✅ done | |
16 | | -| TD-4 | Docs & packaging polish (man pages, POD) | docs | med | M | — | ✅ done | |
17 | | -| TD-5 | Logging I/O polish | performance | low | S | — | ✅ done | |
18 | | - |
19 | | -**TD-1 done** (`restricted = yes` strengthened to a real boundary, per the |
20 | | -agreed approach): TD-1d eval-free colour parsing (`attr_value`), TD-1c shell-free |
21 | | -argv exec for system:/exec:, TD-1a config-lock against user-writable files, |
22 | | -TD-1b user-writable object-dir refusal. Proven by t/25–t/27; README "Restricted |
23 | | -mode" updated. **TD-2 done**: pty coverage for the browser (search/save), the |
24 | | -issue-#1 empty-list repro, F7 shell-escape, the `-s`/`-v`/`-h` CLI; fixed three |
25 | | -latent warnings (two in `list_shortcuts`) along the way. Test count 313 → 351. |
26 | | - |
27 | | -**TD-3 in progress** — `do_form` broken up from **1392 → 590 lines**. First the |
28 | | -field-creation loop, the init-command phase and the `KEY_RESIZE` rebuild were |
29 | | -lifted (`build_form_fields`, `run_form_init`, `resize_form`), and the |
30 | | -confirm/log/wait_key opts handling was shared with `do_menu` |
31 | | -(`apply_action_opts`). Then the four big event-loop arms became helpers: |
32 | | -`run_form_submit` (Enter dispatch: run/form/system/exec), `form_value_list` |
33 | | -(F2 chooser; returns a break flag for the ES_EXIT case), `form_tab_cycle` |
34 | | -(TAB/Shift-TAB single-val cycling) and `form_save_fields` (Save). The event |
35 | | -loop is now a thin dispatcher of mostly one-line arms. Each extraction was |
36 | | -guarded by the form behaviour net, which was first strengthened with three new |
37 | | -pty tests (t/29 fields/nav/separator/boolean, t/30 F2-list/F5-preview, |
38 | | -t/31 TAB-cycle/Save) — test count 351 → 366. The remaining `do_form` bulk is the |
39 | | -per-call helper closures (kept as closures to preserve the M7 "won't stay |
40 | | -shared" fix) and setup/teardown. |
41 | | - |
42 | | -`load_config` broken up **592 → 496 lines**: the ~20-arm colour/attribute |
43 | | -cascade across `field_attr{}`/`active_field_attr{}`/`menu_global{}`/ |
44 | | -`browser_global{}` is now four name→cfg-key maps + an `apply_attr_section()` |
45 | | -helper (overlaps the done TD-1d `attr_value`). Verified semantically verbatim |
46 | | -(t/06, t/15, plus a direct cfg-value check that each section applies its |
47 | | -configured `COLOR_PAIR` to the right key while a bogus key still errors). |
48 | | - |
49 | | -`run_browse` broken up **543 → 363 lines**: its four nested *named* subs |
50 | | -(`get_search_buff`/`search_next`/`search_all`/`load_pad`) were hoisted to file |
51 | | -scope — they only ever touched the package-global pad state run_browse |
52 | | -`local`-ises, so the move is behaviour-identical (a named sub already compiles |
53 | | -at package scope) and de-nests the misleading "sub inside sub" shape. Then the |
54 | | -Save arm became `browser_save()` (returns a break flag for the two ES_EXIT |
55 | | -cases). Verified by t/21 (search drives all three search helpers) and t/22 |
56 | | -(save: picker/script/file write). |
57 | | - |
58 | | -The three mid-sized subs were also broken up: `do_menu` **308 → 230** via |
59 | | -`run_menu_action()` (the menu analogue of `run_form_submit`: parse the item |
60 | | -action, apply its opts, dispatch menu/form/system/exec/run); `do_list` |
61 | | -**344 → 295** via `list_setup()` (per-type footer keys / top message + the |
62 | | -`display` line-wrap, also fixing a latent uninit-value warning); `load_form` |
63 | | -**224 → 120** via `resolve_field_defaults()` (the per-field const/command/ |
64 | | -boolean default resolution + attribute filling). Verified by t/28/t/12, |
65 | | -t/23/t/30, and t/14/t/19/t/20/t/29 respectively; full suite green (366). |
66 | | - |
67 | | -Finally `run_browse`'s open3 + `IO::Select` capture phase became |
68 | | -`capture_output()` (spawn the child, multiplex stdout/stderr into the pad and |
69 | | -the temp file, count lines per stream, buffer a trailing partial line to EOF, |
70 | | -reap the child; `$cpid` stays the package global so the SIGINT handler can still |
71 | | -kill the child mid-run). A new pty test (t/32) was added *first* to pin its |
72 | | -observable results — both streams captured, per-stream line counts, the `ES=` |
73 | | -exit status, the no-final-newline flush — taking the suite 366 → 372. |
74 | | -`run_browse` ends at **272 lines** (543 originally). |
75 | | - |
76 | | -Every sub the audit flagged is now broken up: do_form 1392→590, load_config |
77 | | -592→496, run_browse 543→272, do_list 344→295, do_menu 331→230, load_form |
78 | | -224→120. |
79 | | - |
80 | | -**TD-3 done.** Finally `ccfe.pl` was brought under the CI gate. It was reformatted |
81 | | -with `perltidy` (proven whitespace-only: with all whitespace stripped the token |
82 | | -stream is byte-identical to the previous file) and added to the perltidy |
83 | | -tidy/tidy-check targets. For `perlcritic`, rather than weaken `src/lib`'s |
84 | | -strict severity-3 `.perlcriticrc`, a dedicated `.perlcriticrc-ccfe` lints the |
85 | | -legacy script at severity 5 with the documented legacy idiom classes exempted |
86 | | -(bareword file/dir handles, 2-arg open, stringy eval, global loop iterators, |
87 | | -in-place list mutation, conditional `my`, the `_log_write` singleton nested sub) |
88 | | -— enough to catch new high-severity issues without churning the legacy idioms. |
89 | | -`make check` (test + critic + tidy-check) is green; CI runs both profiles. |
90 | | - |
91 | | ---- |
92 | | - |
93 | | -## TD-1 — RESTRICTED-mode hardening *(security, high, L)* |
94 | | - |
95 | | -**Why.** The audit's headline finding: RESTRICTED mode is a guardrail, not a |
96 | | -security boundary. A motivated local user can defeat it. It is now *documented* |
97 | | -as such (README §"Restricted mode"), but to make it a real kiosk boundary it |
98 | | -needs the four changes below. They are independent and independently shippable; |
99 | | -do the cheapest-value-first or all together behind a new "locked" deployment |
100 | | -mode. **Each needs a design decision on backward compatibility** (existing |
101 | | -restricted setups must keep working, or opt in). |
102 | | - |
103 | | -### TD-1a — System config that user config cannot weaken |
104 | | -`load_config` applies every file on `@cnf_path` in order (system `$ETCDIR` → |
105 | | -`~/.config/ccfe` → legacy) with last-wins. A user re-sets `restricted = no`. |
106 | | -- **Do:** once `restricted = yes` (or a new `restricted_lock = yes`) is set by a |
107 | | - system-owned config, later/user config files must not be able to weaken it — |
108 | | - ignore attempts to set `restricted = no`, narrow (not widen) `restricted_allow`, |
109 | | - or change `shell`/`user_shell`/`path` from a less-trusted file. Track which |
110 | | - file each setting came from (or apply a one-way ratchet). |
111 | | -- **Files:** `src/ccfe.pl` `load_config`; `src/lib/CCFE/Config.pm` already returns |
112 | | - sections in file order, so the caller can attribute provenance. |
113 | | -- **Accept:** a test where a user config setting `restricted = no` after a system |
114 | | - `restricted = yes` does NOT re-enable escapes. |
115 | | - |
116 | | -### TD-1b — Refuse user-writable config/object dirs in restricted deployments |
117 | | -`@mf_path`/`@cnf_path` include `~/.local/share/ccfe` / `~/.config/ccfe` and honour |
118 | | -`CCFE_*_DIR` / `-l`. A user authors any `run:` action (never allowlisted) → arbitrary |
119 | | -execution. |
120 | | -- **Do:** in a locked/restricted deployment, skip object/config dirs that are |
121 | | - writable by the invoking user (or not root/system-owned), and ignore the |
122 | | - `CCFE_*_DIR`/`-l` overrides (or require them to resolve to non-user-writable |
123 | | - paths). Make the policy explicit and logged. |
124 | | -- **Files:** `src/ccfe.pl` path setup (`@mf_path`/`@cnf_path` build), `load_menu`/ |
125 | | - `load_form` dir scan. |
126 | | -- **Accept:** with the lock on, a `run:` menu placed in `~/.local/share/ccfe/...` |
127 | | - is not loaded/run. |
128 | | - |
129 | | -### TD-1c — Shell-free (argv) execution so the allowlist constrains |
130 | | -Actions run via `/bin/sh -c`, but `CCFE::Restrict::denies_verb` only checks the |
131 | | -first token's basename, so `system: df; sh`, `df $(sh)`, backticks, and `%{FIELD}` |
132 | | -values with metacharacters all bypass it. |
133 | | -- **Do:** for `system:`/`exec:` under RESTRICTED, parse the action into an argv |
134 | | - list and exec without a shell (or reject any argument containing shell |
135 | | - metacharacters). Keep `run:` as the explicit "this is a shell command" verb, |
136 | | - documented as unconstrained. Note `prepare_action` substitutes `%{ID}` *before* |
137 | | - the verb check — re-order or move to argv so substituted values can't inject. |
138 | | -- **Files:** `src/ccfe.pl` `call_system`/`exec_command`/`prepare_action` and the |
139 | | - menu/form dispatch; `src/lib/CCFE/Restrict.pm`. |
140 | | -- **Accept:** tests for `system: df; sh` (denied), a field value `; sh` (denied), |
141 | | - while `system: df -h` still runs. |
142 | | - |
143 | | -### TD-1d — eval-free colour/attribute config parsing |
144 | | -`load_config` does `eval "$ctx->{cfg}{X} = $attrv"` for ~20 colour/attr settings; |
145 | | -a config value can run arbitrary Perl in-process. |
146 | | -- **Do:** validate `$attrv` against the expected grammar |
147 | | - (`A_*`, `COLOR_PAIR(n)`, `color_pair('fg','bg')`, integer, `|`-combinations) |
148 | | - and build the value with a small parser instead of `eval`. Table-drive the |
149 | | - ~20 near-identical arms while here (also helps TD-3). |
150 | | -- **Files:** `src/ccfe.pl` `load_config` colour/attr branches (`field_attr`, |
151 | | - `active_field_attr`, `menu_global`, `browser_global`). |
152 | | -- **Accept:** a config value `0; system("...")` does NOT execute; all existing |
153 | | - theme files (`ccfe.conf.smit*`) still parse identically (assert via `t/06`). |
154 | | - |
155 | | ---- |
156 | | - |
157 | | -## TD-2 — Close the pty test-coverage gaps *(coverage, high, M)* |
158 | | - |
159 | | -**Why.** The pure modules are 92–100% covered, but `ccfe.pl`'s interactive |
160 | | -interior has real gaps, several on security- or crash-relevant paths. The pty |
161 | | -harness (`src/t/lib/CCFE/Test/Pty.pm`) makes all of these testable now. |
162 | | - |
163 | | -**Do — add pty/CLI tests for:** |
164 | | -1. **Output-browser search** — run a command with known output, send `/word`, |
165 | | - then `n`; assert the match highlights and the pad scrolls. Covers |
166 | | - `get_search_buff`/`search_next`/`search_all`/`ask_string`. |
167 | | -2. **Save-to-file / save-to-script** — in the browser, save simple + detailed + |
168 | | - runnable-script; assert files exist with the right mode (the script path is |
169 | | - `chmod 0755`), the RESTRICTED denial omits the script option, and the |
170 | | - open-failure branch shows a `disp_msg`. |
171 | | -3. **`exec_command` command-not-found** — the real issue-#1 repro: a form field |
172 | | - whose `list_cmd` fails silently (F2). Replaces the source-pattern guard in |
173 | | - `t/02-issue1-regression.t` with an end-to-end run. |
174 | | -4. **F7 shell-escape** — send F7, run a trivial command, `exit` back; assert |
175 | | - return to the TUI (the allowed path; denial is already unit-tested). |
176 | | -5. **`Theme.pm` init paths** (70% → higher) — a pty assertion that a colour |
177 | | - config actually allocates pairs, or a small `Curses` stub. |
178 | | -6. **CLI** — extend `t/07` with `ccfe -s`, `--help`, `--version` (exit code + |
179 | | - a string). |
180 | | - |
181 | | -**Accept:** each named sub gains runtime coverage; the issue-#1 test no longer |
182 | | -relies on source-grep. **Do this before TD-3** so the refactor is test-guarded. |
183 | | - |
184 | | ---- |
185 | | - |
186 | | -## TD-3 — Break up the oversized `ccfe.pl` subs *(quality, med, L)* |
187 | | - |
188 | | -**Why.** Six subs carry the structural debt and drive the perltidy/perlcritic |
189 | | -divergence: `do_form` (1392 lines, cyclomatic 285), `load_config` (582), |
190 | | -`run_browse` (543), `do_list` (350), `do_menu` (331), `load_form` (224). |
191 | | - |
192 | | -**Do (incrementally, test-guarded by TD-2):** |
193 | | -- Start with `do_form`: extract the field-creation loop, the event loop, and the |
194 | | - action-dispatch into helpers (several already have pure cores in |
195 | | - `CCFE::Layout`/`CCFE::Action` to lean on). Then `run_browse` and `load_config` |
196 | | - (the latter's colour cascade is table-driveable — overlaps TD-1d). |
197 | | -- After the big subs are tractable, run a perltidy pass over `ccfe.pl` and **add |
198 | | - it to the CI perltidy/perlcritic gate** (currently exempt). Decide a perlcritic |
199 | | - severity for `ccfe.pl` that holds the line without fighting legacy idiom |
200 | | - (bareword filehandles, the documented eval-string sites — keep those policies |
201 | | - exempted as in `.perlcriticrc`). |
202 | | - |
203 | | -**Accept:** no sub over an agreed complexity threshold; `ccfe.pl` perltidy-clean |
204 | | -and in the CI gate; suite green throughout. |
205 | | - |
206 | | ---- |
207 | | - |
208 | | -## TD-4 — Docs & packaging polish *(docs, med, M)* |
209 | | - |
210 | | -**Why.** README prose is current, but the reference docs lag and the package |
211 | | -ships man pages users can't find. |
212 | | - |
213 | | -**Do:** |
214 | | -- **Man-page rewrite** — `src/man/*` still say version 1.58 and document the |
215 | | - pre-v2 `/usr/lib/ccfe` layout; they omit every M6 flag (`-k`, `-D/--dump`, |
216 | | - `-P/--plugins`) and feature (mouse, colour, resize). Rewrite to the 2.2 state. |
217 | | -- **Man pages onto the MANPATH** — the deb stages them under |
218 | | - `/usr/lib/ccfe/man/...`, off the man search path, so `man ccfe` fails from a |
219 | | - package install despite the README. Symlink/install into the man hierarchy |
220 | | - (`debian/ccfe.links` or `dh_installman`). |
221 | | -- **Module POD + contributor doc** — the `CCFE::*` modules have good header |
222 | | - comments but zero POD (`perldoc CCFE::FormFile` is empty). Add a one-paragraph |
223 | | - `=head1`/`=head1 FUNCTIONS` per module and a short `lib/CCFE` architecture |
224 | | - overview for contributors. |
225 | | -- **Tidy** — two licence files (`LICENCE` 2021 + `LICENSE` 2026) and a stale |
226 | | - `doc/ChangeLog` in the staged tree; consolidate. |
227 | | - |
228 | | -**Accept:** `man ccfe` works from a package install and documents current flags; |
229 | | -`perldoc CCFE::X` yields content; one contributor architecture doc exists. |
230 | | - |
231 | | -**Done.** `.TH` version 1.58 → 2.2 on all five pages; `ccfe.1` documents the M6 |
232 | | -options `-D/--dump`, `-k`, `-P/--plugins`; `ccfe.conf.5` documents the |
233 | | -previously-undocumented `mouse`, `restricted`, `restricted_allow` and the |
234 | | -`menu_global` colour attributes (`screen_attr`/`item_attr`/`selected_attr`/ |
235 | | -`title_attr`/`key_attr`), each verified against `load_config`. Man pages are put |
236 | | -on the MANPATH via `/usr/share/man` symlinks in `debian/ccfe.links` (the |
237 | | -self-contained tree under `/usr/lib/ccfe` is otherwise off the search path). All |
238 | | -eight `CCFE::*` modules gained `=head1 NAME/SYNOPSIS/DESCRIPTION/FUNCTIONS/SEE |
239 | | -ALSO` POD (podchecker-clean, Pod::Text renders), plus a contributor architecture |
240 | | -overview at `src/lib/CCFE/README.md`. Licence files consolidated to `LICENCE` |
241 | | -(British, the original); the stale upstream `ChangeLog` is no longer shipped by |
242 | | -`install.sh`. The pre-v2 `/usr/lib/ccfe` example paths in `ccfe_menu.5` were left |
243 | | -as-is — they are coincidentally correct for the deb's default `LIBDIR`. Note: no |
244 | | -config keyword controls resize (it is handled at runtime via `KEY_RESIZE`), so |
245 | | -there was nothing to document there. |
246 | | - |
247 | | ---- |
248 | | - |
249 | | -## TD-5 — Logging I/O polish *(performance, low, S)* |
250 | | - |
251 | | -**Why.** Genuinely low priority (a TUI), but two spots do per-event file I/O. |
252 | | - |
253 | | -**Do:** |
254 | | -- Hold the log handle open (or open once around the output-capture loop) instead |
255 | | - of open/append/close per line in the streaming `trace(…, $LOG_ACTION_OUT)` and |
256 | | - per warning in the `$SIG{__WARN__}` handler. Only bites when the log level is |
257 | | - raised, but cheap to make robust. |
258 | | -- In `trace()`, move the `LOG_FNAME` gate ahead of the unconditional |
259 | | - `localtime`/`@months` work. |
260 | | - |
261 | | -**Accept:** no behaviour change; a high-log-level streaming command no longer |
262 | | -open/closes the log per line. |
263 | | - |
264 | | -**Done.** A shared `_log_write()` holds the log handle open and reuses it across |
265 | | -calls (3-arg append open so a crafted `LOG_FNAME` cannot smuggle a pipe/redirect; |
266 | | -autoflush keeps each line on disk immediately, so the old open/append/close |
267 | | -per-line visibility is preserved). Both `trace()` and `$SIG{__WARN__}` write |
268 | | -through it. `trace()` now returns early on `!LOG_FNAME` / level-not-set before any |
269 | | -`localtime`/`@months`/`caller` work. Verified by t/10 and t/11 (which read the |
270 | | -log post-exit, including raised-level `resize_form` traces); full suite green. |
271 | | - |
272 | | ---- |
273 | | - |
274 | | -> Filing: these map cleanly onto GitHub issues (one per TD-N, with TD-1's |
275 | | -> sub-tasks as a checklist or four linked issues). Say the word and they can be |
276 | | -> opened against the remote. |
| 1 | +# CCFE — technical debt backlog (CLOSED) |
| 2 | + |
| 3 | +The work packages distilled from the M8 close-out audit (`M8-AUDIT.md`) are all |
| 4 | +resolved. This file is now a closeout record; the per-package scope/steps that |
| 5 | +used to live here are spent — see the git history and the cited tests for the |
| 6 | +detail. Throughout, the suite stayed green and the CI checks kept passing |
| 7 | +(`perl -c`, `prove -lr t/`, `perlcritic`, `perltidy`). |
| 8 | + |
| 9 | +| ID | Title | Dimension | Status | |
| 10 | +|----|-------|-----------|--------| |
| 11 | +| TD-1 | RESTRICTED-mode hardening | security | ✅ done | |
| 12 | +| TD-2 | Close the pty test-coverage gaps | coverage | ✅ done | |
| 13 | +| TD-3 | Break up the oversized `ccfe.pl` subs | quality | ✅ done | |
| 14 | +| TD-4 | Docs & packaging polish (man pages, POD) | docs | ✅ done | |
| 15 | +| TD-5 | Logging I/O polish | performance | ✅ done | |
| 16 | + |
| 17 | +**TD-1 — RESTRICTED is now a real boundary, not a guardrail.** Config-lock so |
| 18 | +user-writable config can't weaken it (TD-1a), refusal of user-writable |
| 19 | +object/config dirs (TD-1b), shell-free argv exec for `system:`/`exec:` so the |
| 20 | +allow-list actually constrains (TD-1c), and eval-free colour/attr config parsing |
| 21 | +(`attr_value`, TD-1d). Proven by `t/25`–`t/27`; README "Restricted mode" updated. |
| 22 | + |
| 23 | +**TD-2 — interactive coverage.** pty tests for the output-browser search/save |
| 24 | +paths, the issue-#1 empty-list repro, F7 shell-escape, and the `-s`/`-v`/`-h` |
| 25 | +CLI; three latent warnings fixed along the way (two in `list_shortcuts`). |
| 26 | + |
| 27 | +**TD-3 — every audited sub broken up**, each extraction test-guarded: |
| 28 | +`do_form` 1392→590, `load_config` 592→496, `run_browse` 543→272, `do_list` |
| 29 | +344→295, `do_menu` 331→230, `load_form` 224→120. The do_form/browser event-loop |
| 30 | +test net was built first (`t/29`–`t/32`). Finally `ccfe.pl` was made |
| 31 | +perltidy-clean and brought **into the CI gate** — perltidy alongside `src/lib`, |
| 32 | +and a dedicated `.perlcriticrc-ccfe` (severity 5, legacy idioms exempted) so the |
| 33 | +strict `src/lib` profile is untouched. |
| 34 | + |
| 35 | +**TD-4 — docs & packaging.** Man pages updated to 2.2 with the M6 options |
| 36 | +(`-D/--dump`, `-k`, `-P/--plugins`) and put on the MANPATH via |
| 37 | +`debian/ccfe.links`; `ccfe.conf(5)` gained the undocumented params (mouse, |
| 38 | +restricted[_allow], menu colour attrs) and the `variables {}` section; POD added |
| 39 | +to all eight `CCFE::*` modules plus a `lib/CCFE/README.md` architecture overview; |
| 40 | +the duplicate licence file and the stale shipped `ChangeLog` were consolidated. |
| 41 | + |
| 42 | +**TD-5 — logging I/O.** A shared, persistent, autoflushed log handle |
| 43 | +(`_log_write`, used by `trace()` and the `$SIG{__WARN__}` handler) replaces the |
| 44 | +open/append/close-per-line pattern, and `trace()` now gates on the log level |
| 45 | +before doing any timestamp work. |
| 46 | + |
| 47 | +> Future work is feature work — see `FEATURE-REQUESTS.md`. New high-severity |
| 48 | +> findings should be filed there or as fresh issues, not appended here. |
0 commit comments