|
| 1 | +# M8 — Non-functional close-out audit |
| 2 | + |
| 3 | +Five-dimension review of CCFE at the post-M7 state (v2.1.1, `use v5.36`, 313 |
| 4 | +tests). Each dimension below gives a verdict, the findings, and whether each was |
| 5 | +**fixed in this audit** or **filed** for later. Cheap, safe fixes were applied |
| 6 | +now; structural and design-level items are filed (tracked in |
| 7 | +`FEATURE-REQUESTS.md`). |
| 8 | + |
| 9 | +Overall: the codebase is in good shape. The pure `CCFE::*` modules are |
| 10 | +excellent across every dimension; the residual debt is concentrated in the |
| 11 | +legacy `ccfe.pl` body (size/complexity) and in the **security model of |
| 12 | +RESTRICTED mode**, which is the one finding that materially changes how CCFE |
| 13 | +should be described and deployed. |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## 1. Test coverage |
| 18 | + |
| 19 | +**Verdict: strong where it's been invested (modules), with real gaps in the |
| 20 | +`ccfe.pl` interior.** Measured with `Devel::Cover` (require-based + module |
| 21 | +tests; the pty tests exec a separate, uninstrumented process, so true `ccfe.pl` |
| 22 | +coverage is higher than the raw number): |
| 23 | + |
| 24 | +| Unit | stmt | branch | sub | total | |
| 25 | +|------|------|--------|-----|-------| |
| 26 | +| Action / Config / Context | 100 | 100 | 100 | **100** | |
| 27 | +| MenuFile | 100 | 100 | 100 | 97.3 | |
| 28 | +| Restrict | 100 | 100 | 100 | 97.0 | |
| 29 | +| Layout | 100 | 87.5 | 100 | 96.6 | |
| 30 | +| FormFile | 98.9 | 93.7 | 100 | 92.7 | |
| 31 | +| Theme | 70.6 | 75.0 | 80 | 70.1 | |
| 32 | + |
| 33 | +Findings: |
| 34 | +- **[med] Output-browser interior untested** — `run_browse`'s `/`+`n` search |
| 35 | + (`get_search_buff`/`search_next`/`search_all`) and the save-to-file / |
| 36 | + save-to-runnable-script paths never run. The pty harness makes these |
| 37 | + testable. *Filed.* |
| 38 | +- **[med] Error/failure branches untested** — `exec_command` command-not-found, |
| 39 | + the silent `list_cmd` failure behind issue #1 (guarded only by source-pattern |
| 40 | + match in `t/02`), file-open errors in save. The pty harness those tests asked |
| 41 | + for now exists. *Filed.* |
| 42 | +- **[med] F7 shell-escape runtime path** — denial is unit-tested; the allowed |
| 43 | + spawn-and-return is not. *Filed.* |
| 44 | +- **[low] `Theme.pm` at 70 %** — the colour-pair init paths need a live |
| 45 | + terminal; could be covered with a small `Curses` mock or a pty assertion. |
| 46 | + *Filed.* |
| 47 | +- **[low] CLI: `-s`/`list_shortcuts`, `--help`/`--version`** untested (vs |
| 48 | + `-k`/`--dump`/`--plugins` which are). Trivial to add to `t/07`. *Filed.* |
| 49 | + |
| 50 | +## 2. Code quality |
| 51 | + |
| 52 | +**Verdict: modules pristine; the debt is all in `ccfe.pl`, correctly kept out |
| 53 | +of the CI perlcritic gate.** `perlcritic src/lib` is clean at severity 3. |
| 54 | +`ccfe.pl` shows 107 sev-5 / 441 sev-3 — but the bulk is intentional legacy idiom |
| 55 | +(bareword filehandles, stringy `eval` for colour config, non-lexical loop |
| 56 | +iterators) that would be churn-for-churn to "fix". |
| 57 | + |
| 58 | +**Fixed now:** |
| 59 | +- 3 string-vs-numeric operator mismatches on counts (`$c eq 0`, `$nfound eq 2`, |
| 60 | + `$nfound ge 2` → `==`/`>=`). |
| 61 | +- 2 confirmed-unused lexicals removed (`$labelLen`, `$sc` — the latter left over |
| 62 | + from the FormFile extraction). |
| 63 | +- `ralign()` rewritten from an `eval`-string to a plain `sprintf` (also a |
| 64 | + security fix — see §4). |
| 65 | +- Version strings reconciled to 2.1.1 (README, RPM spec, Alpine APKBUILD, |
| 66 | + packaging README) and the RPM/Alpine perl floor set to `>= 5.36`. |
| 67 | + |
| 68 | +**Filed:** |
| 69 | +- **Six oversized subs** dominate the complexity — `do_form` (1392 lines, |
| 70 | + cyclomatic 285), `load_config` (582), `run_browse` (543), `do_list` (350), |
| 71 | + `do_menu` (331), `load_form` (224). `do_form` is the clear refactor target. |
| 72 | +- The colour/attribute config dispatch is a long copy-pasted `if/elsif` of |
| 73 | + `eval "$ctx->{cfg}{X} = …"` arms — table-driveable (and see §4 for the eval). |
| 74 | +- `ccfe.pl` is ~817 lines off perltidy; reformatting is high-churn and best done |
| 75 | + alongside breaking up the big subs, so it stays ungated for now. |
| 76 | + |
| 77 | +## 3. Performance |
| 78 | + |
| 79 | +**Verdict: sound for an interactive TUI — no must-fix.** Scrolling uses a |
| 80 | +pre-built `curses` pad with O(1) `prefresh`; menu/form navigation delegates to |
| 81 | +the ncurses `menu_driver`/`form_driver`; heavy window/field rebuild is correctly |
| 82 | +gated to `KEY_RESIZE`, not run per keystroke. Startup parsing is linear and |
| 83 | +one-shot. |
| 84 | + |
| 85 | +Findings (all low, all filed): |
| 86 | +- The streaming output-capture loop calls `trace(…, $LOG_ACTION_OUT)` per line, |
| 87 | + and both `trace()` and the new `$SIG{__WARN__}` handler open/append/close the |
| 88 | + log **per event**. In normal use this is dormant (`$LOG_ACTION_OUT` is off at |
| 89 | + the default log level; warnings are rare on hot paths), but a user who raises |
| 90 | + the log level gets per-line file I/O while a command streams. Cheap future |
| 91 | + fix: open the log once around the capture loop / hold a handle. |
| 92 | +- `trace()` builds `@months` + `localtime` before checking whether logging is |
| 93 | + even on — trivial, move the gate first. |
| 94 | +- `/`-search reads each pad cell with a char-at-a-time `inchnstr`; one-shot per |
| 95 | + search, bounded by `MAX_PAD_LINES`. Could read a row per call. Low. |
| 96 | + |
| 97 | +## 4. Security |
| 98 | + |
| 99 | +**Verdict: RESTRICTED mode is a *guardrail*, not a security boundary.** It |
| 100 | +reliably blocks the casual interactive escapes it was built for (F7 shell, |
| 101 | +save-as-runnable-script, `system:`/`exec:` of non-allowlisted program names), |
| 102 | +and the supporting hygiene is sound — `harden_child_env` strips `LD_*`/`BASH_ENV` |
| 103 | +/`ENV`/`CDPATH` and resets `IFS`; temp files use `O_EXCL`; persistent files are |
| 104 | +`0600` under the user's own dir; the log is `umask 0177`. But it is **not** a |
| 105 | +boundary against a motivated local user. |
| 106 | + |
| 107 | +**Fixed now:** |
| 108 | +- `ralign()` `eval`-string → `sprintf` (removed a latent Perl-injection smell on |
| 109 | + boolean-field defaults). |
| 110 | +- Documented the real scope of RESTRICTED in the README (it previously read as a |
| 111 | + stronger guarantee than the code provides). |
| 112 | + |
| 113 | +**Filed (the RESTRICTED hardening — needs design, see FEATURE-REQUESTS):** |
| 114 | +- **[high] User config can switch RESTRICTED off.** `load_config` applies every |
| 115 | + file on `@cnf_path` in order (system → `~/.config/ccfe` → legacy) with |
| 116 | + last-wins and no "system locks the value", and the search paths honour |
| 117 | + user-controlled `CCFE_*_DIR`/`XDG_*`. A user re-sets `restricted = no`. Fix |
| 118 | + direction: a locked/system-config mode that user config cannot weaken. |
| 119 | +- **[high] Menus/forms load from user-writable dirs.** `@mf_path` includes |
| 120 | + `~/.local/share/ccfe` and honours `CCFE_OBJ_DIR`/`-l`. A user authors any |
| 121 | + `run:` action (never allowlisted) → arbitrary command execution. Fix |
| 122 | + direction: refuse user-writable object/config dirs in restricted deployments. |
| 123 | +- **[high] Allowlist is bypassable by shell chaining.** Actions run via |
| 124 | + `/bin/sh -c`, but the allowlist only checks the first token's basename, so |
| 125 | + `system: df; sh`, `df $(sh)`, backticks all pass. Same for `%{FIELD}` values |
| 126 | + substituted into args before the check. Fix direction: argv-based (shell-free) |
| 127 | + exec so the allowlist actually constrains what runs. |
| 128 | +- **[med] `eval "$VAR = $attrv"` in `load_config`** executes arbitrary Perl from |
| 129 | + a config value (colour/attr settings). Trusted-config assumption, but combined |
| 130 | + with the above it's an in-process code-exec primitive. Fix direction: validate |
| 131 | + `$attrv` against the expected `A_*`/`COLOR_PAIR(n)`/`color_pair('x','y')` |
| 132 | + grammar (or parse, don't eval). |
| 133 | +- **[low] CWD (`.`) on `$PATH`** in `run_browse`; **[low]** `$prompt` in |
| 134 | + `call_shell` interpolated into a shell string. Both filed (the `.`-on-PATH |
| 135 | + change risks breaking menus that call a script in their own dir, so it needs a |
| 136 | + deliberate decision, not a drive-by edit). |
| 137 | + |
| 138 | +## 5. Documentation |
| 139 | + |
| 140 | +**Verdict: README prose is good and current on M6 features; the gaps are the |
| 141 | +man pages and a couple of self-contradictions, mostly cheap.** |
| 142 | + |
| 143 | +**Fixed now:** |
| 144 | +- README version line 2.0 → 2.1.1; the `man ccfe-menu`/`ccfe-form` (hyphen) |
| 145 | + references corrected to the actual `ccfe_menu`/`ccfe_form` page names; the |
| 146 | + stale "a future release reorganises these paths" caveat updated (that reorg |
| 147 | + shipped in 2.0). |
| 148 | +- ROADMAP self-contradiction (the `$ctx` bullet still tagged 🔄 "in progress" |
| 149 | + while the rest says M7 complete) → ✅; `M7-CTX-PLAN.md` given a COMPLETE |
| 150 | + banner. |
| 151 | + |
| 152 | +**Filed:** |
| 153 | +- **[high] Man pages are stale** — `ccfe.1` etc. still say version 1.58, document |
| 154 | + the pre-v2 `/usr/lib/ccfe` layout, and omit every M6 flag (`-k`, `-D/--dump`, |
| 155 | + `-P/--plugins`) and feature (mouse, colour, resize). Needs a rewrite pass. |
| 156 | +- **[med] Deb ships man pages off the MANPATH** (`/usr/lib/ccfe/man/...`), so |
| 157 | + `man ccfe` won't resolve from a package install despite the README saying it |
| 158 | + will. Packaging fix (symlink/install into the man hierarchy). |
| 159 | +- **[med] No POD / contributor architecture doc** — the `CCFE::*` modules have |
| 160 | + good header comments but zero POD, and there's no "lib/CCFE overview" for a |
| 161 | + new contributor. Additive. |
| 162 | +- **[low] Two licence files** (`LICENCE` 2021 + `LICENSE` 2026) and a stale |
| 163 | + `doc/ChangeLog` in the staged tree — tidy up. |
| 164 | + |
| 165 | +--- |
| 166 | + |
| 167 | +## What was fixed in this audit |
| 168 | + |
| 169 | +`ccfe.pl`: 3 numeric operators, 2 dead lexicals, `ralign` eval→sprintf. |
| 170 | +Docs: README (version, man-page names, RESTRICTED scope, backup caveat), |
| 171 | +ROADMAP + M7-CTX-PLAN consistency. Packaging: version 2.1.1 across |
| 172 | +RPM/Alpine/packaging-README, perl `>= 5.36` floor on RPM/Alpine. All 313 tests |
| 173 | +stay green. |
| 174 | + |
| 175 | +## What is filed (recommended next, roughly by priority) |
| 176 | + |
| 177 | +1. **RESTRICTED-mode hardening** (the §4 high items) — locked system config, |
| 178 | + read-only object dirs in restricted deployments, shell-free argv exec, and |
| 179 | + `eval`-free colour-config parsing. This is the highest-value follow-up. |
| 180 | +2. **Coverage**: pty tests for the browser search/save paths, `exec_command` |
| 181 | + failure (the real issue-#1 repro), and F7 shell-escape. |
| 182 | +3. **`do_form` (and the other oversized subs) refactor**, with a perltidy pass |
| 183 | + and man-page rewrite once the structure settles. |
| 184 | +4. **Packaging**: man pages onto the MANPATH; module POD + a contributor doc. |
| 185 | + |
| 186 | +Performance items are genuinely low priority. None of the filed items block a |
| 187 | +2.2 release. |
0 commit comments