diff --git a/README.md b/README.md index b61c994..5985b59 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,8 @@ The current build expects: - PCRE2 development headers and library (`libpcre2-dev` on Debian / Ubuntu) - standard Linux development headers for `dl`, `pthread`, sockets, and backtrace support +SQLite is vendored under `src/3rdparty/sqlite/` and compiled by `scripts/build_linux.sh`; no system SQLite package is required. + The binary is written to: ```bash @@ -80,9 +82,11 @@ Useful helpers for that data model include: - `json_encode(String)` for emitting JavaScript-safe string literals directly - `ascii_safe_name(String)` for conservative ASCII identifier normalization - `path_join(base, child)` for filesystem-style path assembly +- `sqlite_connect()`, `sqlite_query()`, and related helpers for embedded SQLite storage with named prepared parameters - `zip_create()`, `zip_list()`, `zip_read()`, and `zip_extract()` for minimal ZIP archive workflows - `gz_compress()` and `gz_uncompress()` for gzip-format byte strings - `server_start_http()` / `server_stop()` for runtime-managed custom HTTP listeners backed by `SERVE_HTTP` handlers +- `map()`, `filter()`, `list_unique()`, `dtree_filter()`, `dtree_map()`, `dtree_pick()`, and related helpers for route/menu/card data shaping near render code Named component handlers are also supported: @@ -206,15 +210,19 @@ The current error page includes: - request URI - resolved script path +- generated C++ path when available - high-level error summary +- source/generated excerpts and raw compiler output paths for template/component/unit failure modes - signal number and name when applicable - a native backtrace +Compile failures are also formatted with the source path, generated C++ path, compile-output artifact path, a nearby source/generated excerpt when a line can be identified, and the raw compiler output. + This recovery path currently covers normal request handling. It is not yet the universal recovery path for every runtime subsystem. ## Docs And Tests -The most current user-facing reference lives under `site/doc/`, and the demo pages live under `site/test/`. +The most current user-facing reference lives under `site/doc/`, and the demo pages live under `site/test/`. Developers coming from React, Next, or Remix should start with `site/doc/pages/coming_from_react.txt` / `/doc/index.uce?p=coming_from_react` for the concept map and starter-router notes. Useful entry points: diff --git a/RECOMMENDATIONS.md b/RECOMMENDATIONS.md new file mode 100644 index 0000000..5b0acde --- /dev/null +++ b/RECOMMENDATIONS.md @@ -0,0 +1,492 @@ +# UCE Code Review — Full Findings (2026-06-11) + +Scope: the pending working-tree changes (73 modified tracked files plus new +untracked sources — notably `src/lib/sqlite-connector.cpp/.h` and the +uce-starter theme/router rework). + +Method: seven independent review angles (line-by-line diff scan, +removed-behavior audit, cross-file call tracing, reuse, simplification, +efficiency, altitude), followed by a verification pass on the correctness +candidates. Status legend: + +- **Confirmed** — verified against the working tree, decisive lines quoted. +- **Plausible** — surfaced by a review angle, not individually re-verified. +- **Refuted** — investigated and found not to be an issue (kept here so + nobody re-flags it). + +--- + +## Part 1 — Correctness (all confirmed) + +### 1.1 XSS: island props break out of single-quoted attribute + +**File:** `site/examples/uce-starter/components/theme/web_affordances.uce:12` + +The island component emits JSON props inside a single-quoted attribute +(`data-props=''`), but `html_escape()` +(`src/lib/functionlib.cpp:936`) escapes only `& < > "`, and `json_encode` / +`json_escape` (`src/lib/dtree.cpp:794`) never escape apostrophes. Any `'` in a +prop value terminates the attribute. + +**Failure:** a view passes user-influenced prop text containing an apostrophe, +e.g. `x' autofocus onfocus='alert(1)` — the attribute terminates early and +attacker-controlled attributes/event handlers land on the div. Even benign +values like `Don't` silently truncate the payload. + +**Fix:** use a double-quoted attribute (html_escape covers `"`), or extend +`html_escape` to escape `'` as `'`. + +**Status:** fixed, but fix not verified yet. + +### 1.2 Crash: positional `?` placeholders in sqlite_query + +**File:** `src/lib/sqlite-connector.cpp:86` + +`bind_params` constructs a `String` directly from +`sqlite3_bind_parameter_name(stmt, i)`, which returns NULL for nameless +positional `?` parameters. `std::string(nullptr)` is UB and crashes before the +`if(name == "") continue;` guard on the next line can run. + +**Failure:** any page calling +`sqlite_query(db, "select * from notes where id = ?", params)` with a bare `?` +instead of `:name` segfaults the worker (500 recovery page). + +**Fix:** remove support for positional placeholders from sqlite and mysql API in favor of named placeholders, adjust the documentation accordingly. + +**Status:** fixed, but fix not verified yet. + +### 1.3 HTTP parsing: split_http_headers assumes lines[0] is the request line + +**File:** `src/lib/functionlib.cpp:754` + +The rewritten parser unconditionally treats the first line as the HTTP request +line. The old parser located the first colon-free line, which tolerated a +leading CRLF (RFC 7230 §3.5) and header-only input. Two failure modes: + +- The direct-HTTP caller (`src/fastcgi/src/fcgicc.cc:693`) passes the raw + buffer unstripped — a client sending a stray leading CRLF before + `GET /x.uce HTTP/1.1` gets the request line silently dropped (empty + `REQUEST_METHOD`, request fails). +- Header-only input (`Host: example.test\nX-Token: abc`, e.g. page code + parsing a raw header block) parses `Host:` as `REQUEST_METHOD` and loses + `HTTP_HOST` entirely. The function is directly callable from .uce pages. + +**Fix:** skip leading empty lines before consuming the request line, and only +treat a colon-free first line as the request line. + +**Status:** fixed, but fix not verified yet. + +### 1.4 Silent data loss: multi-statement SQL drops everything after the first `;` + +**File:** `src/lib/sqlite-connector.cpp:178` + +`SQLite::query` passes `0` for `pzTail` to `sqlite3_prepare_v2` and never +checks for remaining SQL, so multi-statement strings execute only the first +statement and still report `ok`. + +**Failure:** a migration page runs +`sqlite_query(db, "create table t(id integer); insert into t values(1);")` — +only the CREATE executes, the INSERT is silently dropped, `sqlite_error()` +says `ok`, leaving the database half-migrated with no error signal. + +**Fix:** capture `pzTail` and either loop over remaining statements or raise +an error when trailing SQL is present. + +**Status:** fixed, but fix not verified yet. + +### 1.5 Diagnostics regression: fault backtrace captured after siglongjmp + +**File:** `src/linux_fastcgi.cpp:895` + +The in-handler `capture_backtrace_string` call (`request_fault_trace`) was +deleted; the trace is now captured after `siglongjmp` back in +`handle_complete`, where the faulting stack has already been unwound. No +capture remains in `on_request_fault_signal`. + +**Failure:** a `.uce` page null-derefs → SIGSEGV → the error page's Trace +shows only `handle_complete`/`main` frames. The faulting unit's frames are +gone, making crash reports undiagnosable beyond the signal number (the README +still advertises a native backtrace of the failure). + +**Fix:** restore the capture inside `on_request_fault_signal` (on the faulting +stack) and hand the result across the longjmp, as the previous code did. + +**Status:** fixed, but fix not verified yet. + +### 1.7 Memory leak: SQLite wrapper objects never freed by request cleanup + +**File:** `src/lib/sqlite-connector.cpp:248` + +`cleanup_sqlite_connections()` closes the raw `sqlite3` handles tracked in +`resources.sqlite_connections` but never deletes the heap-allocated `SQLite` +wrappers from `sqlite_connect` (`new` without `delete`; no arena allocator is +compiled in). `sqlite_disconnect` itself is correct (`delete db` after +unregistering). Note: this mirrors a pre-existing identical leak in the MySQL +connector (`cleanup_mysql_connections`, `mysql-connector.cpp:298-304`). + +**Failure:** a page calls `sqlite_connect()` per request and relies on +end-of-request cleanup instead of `sqlite_disconnect()` — exactly what the +cleanup path exists for. One wrapper leaks per request; unbounded RSS growth +in the long-lived FastCGI worker. + +**Fix:** track the wrapper objects (not raw handles) in +`resources.sqlite_connections` and delete them in cleanup; fix the MySQL +connector the same way, or factor a shared registry (see 5.1). + +**Status:** fixed, but fix not verified yet. + +### 1.8 Asset shims emit stylesheets mid-`
` + +**File:** `site/examples/uce-starter/components/example/marketing_assets.uce:8` +(also `theme_assets.uce`, `gauges/assets.uce`) + +The new ONCE-based asset shims fire inside the view's `ob_start()` capture in +`index.uce` (lines 67-85: view is captured into `fragments["main"]`, then +`themes/page` renders), so their `` output is baked +into the main fragment and spliced into the content div — inside ``, +not ``. + +**Failure:** every marketing/theme/gauges page ships its stylesheet mid-body +(FOUC, invalid-ish markup). The deleted `page_shell` asset registry rendered +these in ``. While this behavior is often okay in practice, we should +improve on this to encourage cleaner output. + +**Fix:** Part A: restore a registration mechanism: record component and asset output +that's intended as once per page in context.call["fragments"]["once"] by default +and the page template component can then explicitly slot this in where appropriate. + +Part B: Modify the preprocessor so directives support attributes like this: +ONCE(Request& context) +@fragment my-fragment-name +{ + ... +} + +Which will then automatically (in this example) slot the output into +context.call["fragments"]["my-fragment-name"] instead of the default slot name "once". +In the future we'll introduce more attributes with this syntax. Using this flexible mechanism for the fragment slot +name, we can leave it up to the page template where to slot in what. ONCE, COMPONENT, +and RENDER should support the fragment attribute. + +**Status:** fixed, but fix not verified yet. + +### 1.9 Error page "Generated C++" hint prints a nonexistent path + +**File:** `src/linux_fastcgi.cpp:79` + +`render_request_failure` computes the hint as +`path_join(BIN_DIRECTORY, SCRIPT_FILENAME) + ".cpp"`, but `path_join` returns +an absolute child unchanged (`src/lib/sys.cpp:179`), while the compiler builds +the real path by plain concatenation `BIN_DIRECTORY + src_path` +(`src/lib/compiler.cpp:631-636`). SCRIPT_FILENAME is always absolute in the +FastCGI path. + +**Failure:** every runtime failure page prints e.g. +`Generated C++: /var/www/site/page.uce.cpp` — the BIN_DIRECTORY prefix is +silently dropped and the path never exists (real file: +`/var/cache/uce/work/var/www/site/page.uce.cpp`). + +**Fix:** export one helper from `compiler.cpp` that maps a source file to its +generated artifact (`su->pre_path + "/" + su->pre_file_name` — the new +`compiler_format_compile_failure` in this same changeset already computes it +correctly) and use it in both places. + +**Status:** fixed, but fix not verified yet. + +### 1.10 O(n²) and per-row deep copies in dtree_map / dtree_filter + +**File:** `src/lib/functionlib.cpp:182` + +Both helpers call `tree.is_list()` inside the per-element callback — +`is_list()` (`src/lib/dtree.cpp:165`) walks the entire map, making the +operation O(n²) — even though both already compute `is_list()` once before +the loop. Additionally, `DTree::each` (`dtree.h:22`) takes the callback +element by value (`DTree t`), deep-copying every subtree per iteration. + +**Failure:** mapping over a 1000-row `sqlite_query` result does ~1M +key-validation checks plus a full deep copy of each row tree. + +**Fix:** hoist `is_list()` into a `bool` local reused in the callback; change +`each()` to pass `const DTree&`. + +**Status:** fixed, but fix not verified yet. + +### 1.11 Proactive compiler child can std::terminate on lock failure + +**File:** `src/lib/compiler.cpp:288` + +`compiler_with_registry_lock` now throws `std::runtime_error` when the +registry lock file can't be opened (previously: warning + proceed unlocked). +`run_proactive_compiler` (`src/linux_fastcgi.cpp:1270-1372`) calls +`compiler_list_known_units` / `compiler_set_known_units` with no try/catch +anywhere in the chain, so a transient failure (fd exhaustion, disk full) +aborts the forked child via `std::terminate`. + +**Blast radius is small:** the proactive compiler runs in a forked child that +the main loop respawns each iteration, and request-path callers are protected +by the try/catch in `handle_complete`. Still, a catch-and-log around the +proactive scan is cheap insurance. + +**Status:** fixed, but fix not verified yet. + +--- + +## Part 2 — Reuse / duplication (plausible unless noted) + +### 2.1 request_query_path() duplicates request_query_route() + +**File:** `src/lib/uri.cpp:311` + +`request_query_path()` copy-pastes `request_query_route()`'s (line 325) +first-keyless-segment scan verbatim — two identical "find first &-part +without =" loops plus `route_path_sanitize` calls that must be kept in sync. +It also has **zero callers** in `src/` or `site/` (only a doc page references +it). + +**Fix:** implement as +`return request_query_route(context, default_path)["l_path"].to_string();`. + +**Status:** fixed, but fix not verified yet. + +### 2.2 list_filter / list_map re-implement the generic filterSmall data-shaping helpers are useful for route lists, nav records, cards, and other render-adjacent structures.
+= join(labels, "\n") ?>
= json_encode(titles) ?>
= json_encode(grouped) ?>
= var_dump(p) ?>+
This demo stores rows in = db_path ?> with runtime SQLite helpers and named prepared parameters.
= error ?>} ?> +