From 2c5b0e23f592d21b77162a28cb576df0354a3e4f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 23:57:17 +0000 Subject: [PATCH 1/7] fix: address the 8 fourth-round Codex findings on #3, with tests Codex posted these on e7dcc61 about 17 minutes before #3 merged, so they reached main unaddressed. Each was traced to a real input; none declined. Soundness: - config.Diagnostic now owns its text (fixed 512-byte buffer, truncating) instead of pointing into the parse arena, which App.load frees on the error path before cli.run prints the message. Reproduced first: under the poisoning test allocator main's code reads the message back as "UUUU..."; the new app.zig test passes. bad() and the get*/rejectUnknown helpers no longer take an allocator they do not use. Security: - web: GET /item/N marks the item read only for a navigation the browser labels same-origin or user-typed (Sec-Fetch-Site: same-origin|none and Sec-Fetch-Mode: navigate). An or link on another site, a same-site page on another local port, or a client sending no Fetch Metadata leaves it unread, and the page then offers a POST "Mark read" button (covered by the existing Origin check). Chosen over POST-only read marks to keep "open = read" and ordinary links (new tab, copy). e2e test covers cross-site, same-site, absent, same-origin and none. Configuration: - check-config rejects a web.bind the listener cannot use: a host name ("localhost") or a non-loopback address without allow_non_loopback, so a passing check no longer hides a UI the daemon then disables. isLoopback moves to config.zig; web.zig re-exports it. - cert_warn_days: 0 is "off" as documented: no probe and no alerts, the expired case included (test). Correctness: - feed: without a text , an atom:link (rel absent or alternate) is used before the guid, and a guid is used only when it is a permalink (isPermaLink absent or true; rdf:about counts, per RSS 1.0). Test covers all four shapes, including a rel=self link that is ignored. - monitor.normalizeText treats '<' as markup only before a plausible tag opener, so "Price < 10" is hashed text and a change after it alerts. tagOpener moves to util.zig and serves store.plainText too. Containers: - build/container/stapeln/compose.toml (selur, Tier C): HERON_CONFIG / HERON_DATA instead of the template's unused APP_* variables, mounts ../heron.json read-only, publishes on 127.0.0.1 only, and drops the curl /health check heron cannot satisfy. Docs: usage.adoc (web.bind, cert_warn_days 0, the read-mark rule), architecture.adoc, CHANGELOG; ROADMAP brought to the merged state (review summary, code-owner self-review blocker, where the signed milestone history lives, upstream candidates, robots.txt-change feature idea). Verified: zig build test 94/94 (main: 92); zig fmt --check clean; ReleaseSafe build; check-config passes both example configs and rejects "localhost" and a missing file with intact messages; local gates check-adoc-renders (157 files), check-root-shape, check-no-placeholders and the repo-map regeneration pass. Not run here: the CLAUDE.md verifier (nickel absent in this container; deed and CLAUDE.md are unchanged). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J4QvDgkykTgAudYjoMWELf --- CHANGELOG.adoc | 13 +++ build/container/stapeln/compose.toml | 17 ++-- docs/architecture.adoc | 2 +- docs/status/ROADMAP.adoc | 56 +++++++++-- docs/usage.adoc | 5 +- src/app.zig | 19 +++- src/cli.zig | 2 +- src/config.zig | 137 ++++++++++++++++----------- src/feed.zig | 39 +++++++- src/monitor.zig | 20 +++- src/store.zig | 7 +- src/util.zig | 6 ++ src/web.zig | 44 +++++++-- 13 files changed, 270 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 09f77d2..fe7e8f0 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -75,6 +75,19 @@ the template commit. other than `24:00:00` are rejected; `heron fetch` cannot overflow its failure counter; the compose build context is the repository root; the documented monitor defaults and robots.txt behaviour now match the code. +* Fourth review round (posted as #3 merged, fixed on a follow-up branch): a + configuration error message no longer points into memory freed before it + is printed; opening `/item/N` marks the item read only for a navigation + the browser labels same-origin or user-typed (Fetch Metadata), so another + site cannot mark items read by embedding the URL, and otherwise the page + offers a POST "Mark read" button; `check-config` rejects a `web.bind` that + is a host name or a non-loopback address without `allow_non_loopback`; + `cert_warn_days: 0` turns the certificate check off entirely, expired + certificates included; an RSS `` is never used + as the item URL and an `atom:link` is preferred to a guid; a `<` that + cannot open a tag (`Price < 10`) is text for monitor change detection; the + selur (Tier-C) stack mounts a config, sets `HERON_*`, publishes on host + loopback only and drops the unusable health check. ==== Changed diff --git a/build/container/stapeln/compose.toml b/build/container/stapeln/compose.toml index 056948b..3b3c617 100644 --- a/build/container/stapeln/compose.toml +++ b/build/container/stapeln/compose.toml @@ -27,16 +27,19 @@ version = "1.0" # Primary application service [services.heron] image = "ghcr.io/hyperpolymath/heron:latest.ctp" -ports = ["8484:8484"] +# The web UI has no authentication: publish it on the host loopback only. +ports = ["127.0.0.1:8484:8484"] +# heron reads only HERON_CONFIG and HERON_DATA; bind address and port come from the config file. environment = { - APP_HOST = "[::]", - APP_PORT = "8484", - APP_LOG_FORMAT = "json", - APP_DATA_DIR = "/data", + HERON_CONFIG = "/config/heron.json", + HERON_DATA = "/data", } -volumes = ["heron-data:/data"] +# Without a config heron exits with ConfigNotFound. ../heron.json is the one compose.yaml mounts: +# it binds the UI to 0.0.0.0 inside the container and relies on the loopback mapping above. +volumes = ["heron-data:/data", "../heron.json:/config/heron.json:ro"] restart = "always" -healthcheck = { test = "curl -sf http://localhost:8484/health", interval = "30s", timeout = "5s", retries = 3 } +# No healthcheck: heron has no /health route and the runtime image carries no curl. heron is PID 1 +# and exits non-zero on a fatal error, which `restart` handles. # Rokur secrets gate: refuses to start (and refuses to proxy) unless the # declared secrets are present; emits a structured audit log of every access. diff --git a/docs/architecture.adoc b/docs/architecture.adoc index e2cd1b9..71bdb77 100644 --- a/docs/architecture.adoc +++ b/docs/architecture.adoc @@ -67,7 +67,7 @@ crash recovery and `heron rebuild` would be lies. | 4 | The web UI is reachable only from the machine unless explicitly allowed. -| `src/web.zig`: loopback bind by default, `allow_non_loopback` gate, Host-header allowlist, Origin check on POST, CSP, no JavaScript. +| `src/web.zig`: loopback bind by default, `allow_non_loopback` gate, Host-header allowlist, Origin check on POST, Fetch Metadata gate on the GET read mark, CSP, no JavaScript. | DNS-rebinding or LAN exposure of an unauthenticated UI. | 5 diff --git a/docs/status/ROADMAP.adoc b/docs/status/ROADMAP.adoc index d7a5330..9e4e809 100644 --- a/docs/status/ROADMAP.adoc +++ b/docs/status/ROADMAP.adoc @@ -16,7 +16,8 @@ build and test instructions moved to `README.adoc` and `CONTRIBUTING.adoc`. Milestones M0–M6 are complete and imported with their signed commit history. Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): -* `zig build test --summary all`: 85/85 tests pass. +* `zig build test --summary all`: 85/85 tests pass at import; 94/94 after + the four review rounds (see below). * `zig build -Doptimize=ReleaseSafe` and the static musl build both succeed. * `zig fmt --check` was NOT clean on seven files (Zig 0.17 renames `@intFromEnum` to `@backingInt` and re-wraps two literals); the formatter was @@ -24,6 +25,24 @@ Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): * Idle RSS ~8 MB (ReleaseSafe, 24 feeds + 6 monitors + web UI) was measured before import over one ~45 s window; not re-measured since. +=== Import review (2026-10-27 to 2026-11-04) + +The import merged to `main` as hyperpolymath/heron-polyfeed-reader#3 +(squash, 2026-10-27). Codex reviewed every push: 45 findings over four +rounds, each traced to a real input before it was fixed, all fixed with +tests, none declined. The fourth round (8 findings) was posted as #3 merged +and is fixed on a follow-up PR. Among them: panics or ReleaseSafe traps +on untrusted input (title-less RSS channel, page-number overflow, +out-of-range `cert_warn_days`), a use-after-free of configuration error +text, terminal-escape injection through feed titles printed by the CLI, a +cross-site GET that marked items read, mixed-content text order, index/log +consistency (stale read-only log length, replay limit, failed-commit replay, +truncated-log reindex), strict configuration (unknown keys, mistyped values, +unknown adapter types, unusable `web.bind`), robots.txt across redirects, and +the template container stacks (Tier-A image, compose and selur configs). +SonarCloud's quality gate passed. CodeRabbit did not review: the import's +216 files exceed its 100-file limit and the account had no credits. + == Milestones [cols="5,1",options="header"] @@ -37,7 +56,7 @@ Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): | M4: HTTP transport (timeouts, manual redirects), config, engine, daemon, CLI | 100% | M5: monitor adapter (up/down transitions, change detection, TLS expiry, robots) | 100% | M6: localhost-only web UI (list/search/item/read marks/status), daemon integration, e2e test | 100% -| M7: integrate into the RSR estate (template merge, mint, CI) | 90% +| M7: integrate into the RSR estate (template merge, mint, CI, review) | 95% | M8: second adapter (NNTP or Gemini) to prove the adapter seam | 0% | M9: live-site soak test and real-CA certificate-probe check | 0% | M10: web UI `monitors` summary and search pagination | 0% @@ -49,10 +68,18 @@ Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): . *Clade is UNASSIGNED* in the deed. Choosing it is the owner's act (the template says so); `just validate-rsr` fails until it is chosen. The ecosystem `:position-type` is UNASSIGNED for the same reason. -. *Signing.* The M7 integration commits were prepared in a session with no - signing key. They must be committed with `id_ed25519_signing` locally - before push; the eleven imported milestone commits keep their own - signatures. +. *Code-owner review of your own PR.* `main`'s Branch-Floor ruleset requires + a code-owner review, and CODEOWNERS names only `@hyperpolymath`, who also + authors these PRs; GitHub never counts an author's own review. The owner + merged #3 themselves. Every later PR from this account meets the same + wall unless a second code owner (a person or a team) is added or the rule + is relaxed for solo repositories; that is an estate-wide question for + `standards`, not this repo alone. +. *Commit signatures.* The M7 commits were signed by the cloud session's + key, not the owner's `id_ed25519_signing`; `main` holds the squash commit + GitHub made. The eleven milestone commits keep their own signatures in + #3's commits, which GitHub keeps at `refs/pull/3/head` (a squash + flattens them out of `main`; the source branch was deleted on merge). . *Idris2 ABI seam.* `just test` and `just build` run every detected language. The template's `src/interface/abi.ipkg` (Idris2) is detected alongside `build.zig`; without `idris2` on PATH the Zig steps pass but the @@ -67,8 +94,19 @@ Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): == Next actions -* Push the branch, let `zig-ci.yml` and the estate gates run, triage every - review-app finding and CI anomaly, merge. +* Merge the fourth-round fixes; then M7 is done except the owner decisions + above. +* Feature raised in review, not built: an alert when robots.txt itself + changes (today only a change that blocks the monitored path alerts). +* Upstream, outside this repo: +** `hyperpolymath/standards`: `scripts/check-no-vlang.sh` should exclude + `vendor/` (SQLite's `iPrevLangid` matches its case-insensitive pattern; + patched locally). +** `hyperpolymath/rsr-template-repo`: `just repo-init`'s OPENSSF line delete + leaves a table row in `docs/governance/TEMPLATE-STANDARDS-AUDIT.adoc` one + cell short; the template `compose.yaml` build context `..` resolves to + `build/`, not the repository root; its container stacks assume an + `APP_*` environment and a `/health` route a minted project may not have. * Owner: choose the clade (`ap` Applications is the obvious candidate: an end-user desktop daemon), set `:position-type`, register in gv-clade-index. * M8: second adapter (NNTP/NNTPS or Gemini with TOFU) with no core changes; @@ -85,8 +123,6 @@ Ground truth at import (2026-10-27, Zig 0.17.0 exactly, x86_64 Linux host): * Re-sync `src/vendor/tls_client.zig` whenever Zig is upgraded (`src/vendor/NOTICE`); the `[sfr patch]` markers carry the old project name and can be renamed then. -* Re-count the container `test` stage after the rename (host run was 85/85; - `zig-ci.yml` now builds that stage on every push). == Known gaps / untested diff --git a/docs/usage.adoc b/docs/usage.adoc index 91ba585..b4371f3 100644 --- a/docs/usage.adoc +++ b/docs/usage.adoc @@ -69,7 +69,7 @@ One JSON file (no YAML). `check-config` rejects unknown keys at every level and | `user_agent` | string | built-in | Sent on every request; put a contact address in it. | `timeout_s` | integer | 30 | Wall-clock timeout per request. | `web.enabled` | boolean | `false` | Serve the web UI from the daemon. -| `web.bind` | string | `127.0.0.1` | Bind address. Anything but loopback also needs `allow_non_loopback`. Requests must carry a Host header naming `127.0.0.1`, `localhost`, `[::1]` or this bound address (with the port); a wildcard bind (`0.0.0.0`, `::`) is not special-cased, so name the address you expose. +| `web.bind` | string | `127.0.0.1` | Bind address, numeric (`127.0.0.1`, `::1`); a host name such as `localhost` is rejected. Anything but loopback also needs `allow_non_loopback`. Requests must carry a Host header naming `127.0.0.1`, `localhost`, `[::1]` or this bound address (with the port); a wildcard bind (`0.0.0.0`, `::`) is not special-cased, so name the address you expose. | `web.port` | integer | 8484 | Listening port. | `web.allow_non_loopback` | boolean | `false` | Required to bind a non-loopback address; the UI has no authentication. | `sources[].name` | string | required | Short unique name used by the CLI. @@ -83,7 +83,7 @@ One JSON file (no YAML). `check-config` rejects unknown keys at every level and | `monitors[].expect_status` | integer | 200 | HTTP status that counts as UP. | `monitors[].detect_changes` | boolean | `true` | Fetch the body (GET), hash its visible text and alert on change (rate-limited to one alert per hour). `false` checks reachability with HEAD only. | `monitors[].check_robots` | boolean | `true` | Fetch and honour `robots.txt` (RFC 9309, cached 24 h): a BLOCKED alert when it starts disallowing the monitored path, and redirects to another origin are refused. Changes to robots.txt that leave the path allowed raise no alert. -| `monitors[].cert_warn_days` | integer | 14 | Alert when the leaf certificate expires within this many days. +| `monitors[].cert_warn_days` | integer | 14 | Alert when the leaf certificate expires within this many days, or has expired. `0` turns the certificate check off (no probe, no alerts). |=== `examples/heron.json` is a complete, valid example. @@ -106,6 +106,7 @@ One JSON file (no YAML). `check-config` rejects unknown keys at every level and * No WebSub; feeds are polled. * Relative entry links are resolved against the feed URL; `xml:base` is not consulted. * The web UI serves one request at a time with a 10 s per-connection deadline; it is a personal UI, not a public server. +* Opening an item in the web UI marks it read only when the browser sends Fetch Metadata saying the navigation came from the UI itself or was typed by you (`Sec-Fetch-Site: same-origin` or `none`, `Sec-Fetch-Mode: navigate`; every current browser does). Otherwise, for example from an older browser or a link on another site, the item stays unread and the page shows a *Mark read* button. == See also diff --git a/src/app.zig b/src/app.zig index 52c9cec..cc526a5 100644 --- a/src/app.zig +++ b/src/app.zig @@ -45,7 +45,7 @@ pub const App = struct { break :blk try std.fmt.allocPrint(a, "{s}/.config/heron-polyfeed-reader/config.json", .{h}); }; const text = Io.Dir.cwd().readFileAlloc(io, config_path, a, .limited(1024 * 1024)) catch { - diag.message = try std.fmt.allocPrint(a, "cannot read config file {s}", .{config_path}); + diag.set("cannot read config file {s}", .{config_path}); return error.ConfigNotFound; }; const cfg = config.parse(a, text, diag) catch |e| switch (e) { @@ -86,3 +86,20 @@ pub const App = struct { self.arena.deinit(); } }; + +test "a load failure's diagnostic outlives the arena load frees" { + var env = std.process.Environ.Map.init(std.testing.allocator); + defer env.deinit(); + var diag: config.Diagnostic = .{}; + // The path is allocated after the arena is gone, so the message must not point into it. + try std.testing.expectError(error.ConfigNotFound, App.load(std.testing.allocator, std.testing.io, &env, .{ .config_path = "/nonexistent/heron.json" }, &diag)); + try std.testing.expectEqualStrings("cannot read config file /nonexistent/heron.json", diag.message()); + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.writeFile(std.testing.io, .{ .sub_path = "bad.json", .data = "{\"web\":{\"bind\":\"localhost\"}}" }); + const path = try tmp.dir.realPathFileAlloc(std.testing.io, "bad.json", std.testing.allocator); + defer std.testing.allocator.free(path); + try std.testing.expectError(error.InvalidConfig, App.load(std.testing.allocator, std.testing.io, &env, .{ .config_path = path }, &diag)); + try std.testing.expect(std.mem.startsWith(u8, diag.message(), "web.bind must be a numeric IP address")); +} diff --git a/src/cli.zig b/src/cli.zig index 419bc57..1a5a21d 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -78,7 +78,7 @@ fn run(init: std.process.Init) !u8 { var diag: config.Diagnostic = .{}; var app = app_mod.App.load(gpa, io, init.environ_map, ov, &diag) catch |e| { try out.flush(); - std.log.err("{s}{s}{s}", .{ @errorName(e), if (diag.message.len > 0) ": " else "", diag.message }); + std.log.err("{s}{s}{s}", .{ @errorName(e), if (diag.message().len > 0) ": " else "", diag.message() }); return 1; }; defer app.deinit(); diff --git a/src/config.zig b/src/config.zig index e665879..d4a1842 100644 --- a/src/config.zig +++ b/src/config.zig @@ -57,15 +57,35 @@ pub const Config = struct { monitors: []MonitorCfg = &.{}, }; -pub const Diagnostic = struct { message: []const u8 = "" }; +/// Why loading failed. The text lives in the struct itself, not in the parse allocator, so it +/// outlives the arena a caller frees on error (App.load does). Longer text is truncated. +pub const Diagnostic = struct { + buf: [512]u8 = undefined, + len: usize = 0, + + pub fn set(d: *Diagnostic, comptime fmt: []const u8, args: anytype) void { + var w: std.Io.Writer = .fixed(&d.buf); + w.print(fmt, args) catch {}; // WriteFailed only means the buffer is full: keep the prefix + d.len = w.end; + } + + pub fn message(d: *const Diagnostic) []const u8 { + return d.buf[0..d.len]; + } +}; pub const Error = error{InvalidConfig} || Allocator.Error; -fn bad(a: Allocator, diag: *Diagnostic, comptime fmt: []const u8, args: anytype) Error { - diag.message = std.fmt.allocPrint(a, fmt, args) catch "invalid configuration"; +fn bad(diag: *Diagnostic, comptime fmt: []const u8, args: anytype) Error { + diag.set(fmt, args); return error.InvalidConfig; } +/// The loopback addresses the web UI may bind without `allow_non_loopback`. +pub fn isLoopback(addr: []const u8) bool { + return std.mem.eql(u8, addr, "127.0.0.1") or std.mem.eql(u8, addr, "::1") or std.mem.startsWith(u8, addr, "127."); +} + fn validName(n: []const u8) bool { if (n.len == 0 or n.len > 64) return false; for (n) |c| if (!(std.ascii.isAlphanumeric(c) or c == '.' or c == '_' or c == '-')) return false; @@ -80,86 +100,93 @@ fn validHttpUrl(u: []const u8) bool { /// A string field: absent -> null; present but not a JSON string -> InvalidConfig, so /// `"data_dir": false` cannot silently fall back to the default directory. -fn getStr(a: Allocator, diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?[]const u8 { +fn getStr(diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?[]const u8 { const v = obj.get(key) orelse return null; - if (v != .string) return bad(a, diag, "{s} must be a string", .{what}); + if (v != .string) return bad(diag, "{s} must be a string", .{what}); return v.string; } /// An integer field: absent -> null; present but not a JSON integer -> InvalidConfig, so a quoted /// `"port": "9000"` cannot silently fall back to the default. -fn getInt(a: Allocator, diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?i64 { +fn getInt(diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?i64 { const v = obj.get(key) orelse return null; - if (v != .integer) return bad(a, diag, "{s} must be an integer (unquoted)", .{what}); + if (v != .integer) return bad(diag, "{s} must be an integer (unquoted)", .{what}); return v.integer; } /// A typo such as "souces" must not parse as "no sources" and let the daemon run polling nothing. -fn rejectUnknown(a: Allocator, diag: *Diagnostic, obj: std.json.ObjectMap, comptime allowed: []const []const u8, what: []const u8) Error!void { +fn rejectUnknown(diag: *Diagnostic, obj: std.json.ObjectMap, comptime allowed: []const []const u8, what: []const u8) Error!void { var it = obj.iterator(); while (it.next()) |e| { var known = false; inline for (allowed) |k| { if (std.mem.eql(u8, e.key_ptr.*, k)) known = true; } - if (!known) return bad(a, diag, "{s}: unknown key '{s}'", .{ what, e.key_ptr.* }); + if (!known) return bad(diag, "{s}: unknown key '{s}'", .{ what, e.key_ptr.* }); } } /// A boolean field: absent -> null; present but not a JSON boolean -> InvalidConfig. A mistyped /// `"enabled": "false"` must never silently take the default (which for web.enabled is true). -fn getBool(a: Allocator, diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?bool { +fn getBool(diag: *Diagnostic, obj: std.json.ObjectMap, key: []const u8, what: []const u8) Error!?bool { const v = obj.get(key) orelse return null; - if (v != .bool) return bad(a, diag, "{s} must be true or false (unquoted)", .{what}); + if (v != .bool) return bad(diag, "{s} must be true or false (unquoted)", .{what}); return v.bool; } /// Parse and validate. All returned memory belongs to `a` (use an arena). pub fn parse(a: Allocator, text: []const u8, diag: *Diagnostic) Error!Config { const root = std.json.parseFromSliceLeaky(std.json.Value, a, text, .{}) catch |e| - return bad(a, diag, "not valid JSON: {s}", .{@errorName(e)}); - if (root != .object) return bad(a, diag, "top level must be an object", .{}); + return bad(diag, "not valid JSON: {s}", .{@errorName(e)}); + if (root != .object) return bad(diag, "top level must be an object", .{}); const obj = root.object; var cfg: Config = .{}; - try rejectUnknown(a, diag, obj, &.{ "data_dir", "user_agent", "timeout_s", "web", "sources", "monitors" }, "top level"); + try rejectUnknown(diag, obj, &.{ "data_dir", "user_agent", "timeout_s", "web", "sources", "monitors" }, "top level"); - if (try getStr(a, diag, obj, "data_dir", "data_dir")) |d| cfg.data_dir = d; - if (try getStr(a, diag, obj, "user_agent", "user_agent")) |d| cfg.user_agent = d; - if (try getInt(a, diag, obj, "timeout_s", "timeout_s")) |t| { - if (t < 1 or t > 600) return bad(a, diag, "timeout_s must be 1..600", .{}); + if (try getStr(diag, obj, "data_dir", "data_dir")) |d| cfg.data_dir = d; + if (try getStr(diag, obj, "user_agent", "user_agent")) |d| cfg.user_agent = d; + if (try getInt(diag, obj, "timeout_s", "timeout_s")) |t| { + if (t < 1 or t > 600) return bad(diag, "timeout_s must be 1..600", .{}); cfg.timeout_s = @intCast(t); } if (obj.get("web")) |w| { - if (w != .object) return bad(a, diag, "web must be an object", .{}); - try rejectUnknown(a, diag, w.object, &.{ "enabled", "bind", "port", "allow_non_loopback" }, "web"); - cfg.web.enabled = (try getBool(a, diag, w.object, "enabled", "web.enabled")) orelse true; - if (try getStr(a, diag, w.object, "bind", "web.bind")) |b| cfg.web.bind = b; - cfg.web.allow_non_loopback = (try getBool(a, diag, w.object, "allow_non_loopback", "web.allow_non_loopback")) orelse false; - if (try getInt(a, diag, w.object, "port", "web.port")) |p| { - if (p < 1 or p > 65535) return bad(a, diag, "web.port out of range", .{}); + if (w != .object) return bad(diag, "web must be an object", .{}); + try rejectUnknown(diag, w.object, &.{ "enabled", "bind", "port", "allow_non_loopback" }, "web"); + cfg.web.enabled = (try getBool(diag, w.object, "enabled", "web.enabled")) orelse true; + if (try getStr(diag, w.object, "bind", "web.bind")) |b| cfg.web.bind = b; + cfg.web.allow_non_loopback = (try getBool(diag, w.object, "allow_non_loopback", "web.allow_non_loopback")) orelse false; + if (try getInt(diag, w.object, "port", "web.port")) |p| { + if (p < 1 or p > 65535) return bad(diag, "web.port out of range", .{}); cfg.web.port = @intCast(p); } + // Checked here, not first at listen time, so that a configuration check that passes means + // the UI can actually start: the listener takes a numeric address, and only loopback + // unless allowed. + _ = std.Io.net.IpAddress.parse(cfg.web.bind, 0) catch + return bad(diag, "web.bind must be a numeric IP address such as 127.0.0.1 or ::1, not a host name", .{}); + if (!cfg.web.allow_non_loopback and !isLoopback(cfg.web.bind)) + return bad(diag, "web.bind {s} is not a loopback address; set web.allow_non_loopback to expose the unauthenticated UI", .{cfg.web.bind}); } var names: std.StringHashMapUnmanaged(void) = .empty; var sources: std.ArrayList(SourceCfg) = .empty; if (obj.get("sources")) |sv| { - if (sv != .array) return bad(a, diag, "sources must be an array", .{}); + if (sv != .array) return bad(diag, "sources must be an array", .{}); for (sv.array.items, 0..) |item, idx| { - if (item != .object) return bad(a, diag, "sources[{d}] must be an object", .{idx}); + if (item != .object) return bad(diag, "sources[{d}] must be an object", .{idx}); const o = item.object; - try rejectUnknown(a, diag, o, &.{ "name", "type", "url", "interval_s", "options" }, "source"); - const name = (try getStr(a, diag, o, "name", "source name")) orelse return bad(a, diag, "sources[{d}]: missing name", .{idx}); - if (!validName(name)) return bad(a, diag, "sources[{d}]: bad name '{s}' (use A-Z a-z 0-9 . _ -, max 64)", .{ idx, name }); - if ((try names.getOrPut(a, name)).found_existing) return bad(a, diag, "duplicate name '{s}'", .{name}); - const type_name = (try getStr(a, diag, o, "type", "source type")) orelse "feed"; - const url = (try getStr(a, diag, o, "url", "source url")) orelse return bad(a, diag, "source '{s}': missing url", .{name}); - const interval = (try getInt(a, diag, o, "interval_s", "source interval_s")) orelse default_interval_s; - if (interval < min_interval_s or interval > 30 * 86400) return bad(a, diag, "source '{s}': interval_s must be {d}..2592000", .{ name, min_interval_s }); - if (std.mem.eql(u8, type_name, "feed") and !validHttpUrl(url)) return bad(a, diag, "source '{s}': url must be http(s)", .{name}); + try rejectUnknown(diag, o, &.{ "name", "type", "url", "interval_s", "options" }, "source"); + const name = (try getStr(diag, o, "name", "source name")) orelse return bad(diag, "sources[{d}]: missing name", .{idx}); + if (!validName(name)) return bad(diag, "sources[{d}]: bad name '{s}' (use A-Z a-z 0-9 . _ -, max 64)", .{ idx, name }); + if ((try names.getOrPut(a, name)).found_existing) return bad(diag, "duplicate name '{s}'", .{name}); + const type_name = (try getStr(diag, o, "type", "source type")) orelse "feed"; + const url = (try getStr(diag, o, "url", "source url")) orelse return bad(diag, "source '{s}': missing url", .{name}); + const interval = (try getInt(diag, o, "interval_s", "source interval_s")) orelse default_interval_s; + if (interval < min_interval_s or interval > 30 * 86400) return bad(diag, "source '{s}': interval_s must be {d}..2592000", .{ name, min_interval_s }); + if (std.mem.eql(u8, type_name, "feed") and !validHttpUrl(url)) return bad(diag, "source '{s}': url must be http(s)", .{name}); var options: []const u8 = ""; if (o.get("options")) |opt| { - if (opt != .object) return bad(a, diag, "source '{s}': options must be an object", .{name}); + if (opt != .object) return bad(diag, "source '{s}': options must be an object", .{name}); options = try std.json.Stringify.valueAlloc(a, opt, .{}); } try sources.append(a, .{ .name = name, .type_name = type_name, .url = url, .interval_s = @intCast(interval), .options = options }); @@ -169,28 +196,28 @@ pub fn parse(a: Allocator, text: []const u8, diag: *Diagnostic) Error!Config { var monitors: std.ArrayList(MonitorCfg) = .empty; if (obj.get("monitors")) |mv| { - if (mv != .array) return bad(a, diag, "monitors must be an array", .{}); + if (mv != .array) return bad(diag, "monitors must be an array", .{}); for (mv.array.items, 0..) |item, idx| { - if (item != .object) return bad(a, diag, "monitors[{d}] must be an object", .{idx}); + if (item != .object) return bad(diag, "monitors[{d}] must be an object", .{idx}); const o = item.object; - try rejectUnknown(a, diag, o, &.{ "name", "url", "interval_s", "expect_status", "detect_changes", "check_robots", "cert_warn_days" }, "monitor"); - const name = (try getStr(a, diag, o, "name", "monitor name")) orelse return bad(a, diag, "monitors[{d}]: missing name", .{idx}); - if (!validName(name)) return bad(a, diag, "monitors[{d}]: bad name '{s}'", .{ idx, name }); - if ((try names.getOrPut(a, name)).found_existing) return bad(a, diag, "duplicate name '{s}'", .{name}); - const url = (try getStr(a, diag, o, "url", "monitor url")) orelse return bad(a, diag, "monitor '{s}': missing url", .{name}); - if (!validHttpUrl(url)) return bad(a, diag, "monitor '{s}': url must be http(s)", .{name}); - const interval = (try getInt(a, diag, o, "interval_s", "monitor interval_s")) orelse 300; - if (interval < min_interval_s or interval > 30 * 86400) return bad(a, diag, "monitor '{s}': interval_s must be {d}..2592000", .{ name, min_interval_s }); + try rejectUnknown(diag, o, &.{ "name", "url", "interval_s", "expect_status", "detect_changes", "check_robots", "cert_warn_days" }, "monitor"); + const name = (try getStr(diag, o, "name", "monitor name")) orelse return bad(diag, "monitors[{d}]: missing name", .{idx}); + if (!validName(name)) return bad(diag, "monitors[{d}]: bad name '{s}'", .{ idx, name }); + if ((try names.getOrPut(a, name)).found_existing) return bad(diag, "duplicate name '{s}'", .{name}); + const url = (try getStr(diag, o, "url", "monitor url")) orelse return bad(diag, "monitor '{s}': missing url", .{name}); + if (!validHttpUrl(url)) return bad(diag, "monitor '{s}': url must be http(s)", .{name}); + const interval = (try getInt(diag, o, "interval_s", "monitor interval_s")) orelse 300; + if (interval < min_interval_s or interval > 30 * 86400) return bad(diag, "monitor '{s}': interval_s must be {d}..2592000", .{ name, min_interval_s }); var m: MonitorCfg = .{ .name = name, .url = url, .interval_s = @intCast(interval) }; - if (try getInt(a, diag, o, "expect_status", "monitor expect_status")) |s| { - if (s < 100 or s > 599) return bad(a, diag, "monitor '{s}': expect_status out of range", .{name}); + if (try getInt(diag, o, "expect_status", "monitor expect_status")) |s| { + if (s < 100 or s > 599) return bad(diag, "monitor '{s}': expect_status out of range", .{name}); m.expect_status = @intCast(s); } - if (try getBool(a, diag, o, "detect_changes", "monitor detect_changes")) |b| m.detect_changes = b; - if (try getBool(a, diag, o, "check_robots", "monitor check_robots")) |b| m.check_robots = b; - if (try getInt(a, diag, o, "cert_warn_days", "monitor cert_warn_days")) |d| { + if (try getBool(diag, o, "detect_changes", "monitor detect_changes")) |b| m.detect_changes = b; + if (try getBool(diag, o, "check_robots", "monitor check_robots")) |b| m.check_robots = b; + if (try getInt(diag, o, "cert_warn_days", "monitor cert_warn_days")) |d| { // Bound before the narrowing cast: an out-of-range integer is a diagnostic, not a trap. - if (d < 0 or d > 3650) return bad(a, diag, "monitor '{s}': cert_warn_days must be 0..3650", .{name}); + if (d < 0 or d > 3650) return bad(diag, "monitor '{s}': cert_warn_days must be 0..3650", .{name}); m.cert_warn_days = @intCast(d); } try monitors.append(a, m); @@ -250,10 +277,12 @@ test "rejections carry a message" { "{\"user_agent\":1}", "{\"web\":{\"bind\":[]}}", "{\"sources\":[{\"name\":\"a\",\"type\":7,\"url\":\"http://x/\"}]}", + "{\"web\":{\"bind\":\"localhost\"}}", // the listener takes numeric addresses only + "{\"web\":{\"bind\":\"0.0.0.0\"}}", // not loopback, and allow_non_loopback is not set }; for (cases) |text| { var d: Diagnostic = .{}; try std.testing.expectError(error.InvalidConfig, parse(arena.allocator(), text, &d)); - try std.testing.expect(d.message.len > 0); + try std.testing.expect(d.message().len > 0); } } diff --git a/src/feed.zig b/src/feed.zig index 1b1da74..0fb193d 100644 --- a/src/feed.zig +++ b/src/feed.zig @@ -88,8 +88,24 @@ fn rssItem(a: Allocator, source: []const u8, n: *const xml.Node) Allocator.Error guid = ab; }; if (link.len == 0) { - // RSS where is missing: a permalink guid or atom:link href stands in. - if (std.mem.startsWith(u8, guid, "http")) link = guid else if (n.child("link")) |l| link = l.attr("href") orelse ""; + // RSS without a text : an atom:link href (rel absent or "alternate") stands in, else + // the guid, but only when it is a permalink (RSS 2.0: isPermaLink defaults to true; "false" + // means an opaque identifier that merely looks like a URL). + var c = n.first_child; + while (c) |l| : (c = l.next) { + if (!l.is("link")) continue; + const href = l.attr("href") orelse continue; + const rel = l.attr("rel") orelse "alternate"; + if (std.mem.eql(u8, rel, "alternate")) { + link = href; + break; + } + } + if (link.len == 0 and std.mem.startsWith(u8, guid, "http")) { + // No : the identifier is RDF's rdf:about, which RSS 1.0 says equals the item link. + const permalink = if (n.child("guid")) |g| !std.mem.eql(u8, g.attr("isPermaLink") orelse "true", "false") else true; + if (permalink) link = guid; + } } const date_raw = firstNonEmpty(&.{ try childText(a, n, "pubDate"), try childText(a, n, "date"), try childText(a, n, "published"), try childText(a, n, "updated") }); const body = firstNonEmpty(&.{ try childText(a, n, "encoded"), try childText(a, n, "description"), try childText(a, n, "summary") }); @@ -329,6 +345,25 @@ test "edge: channel without a title yields an empty title, not a panic" { try testing.expectEqual(@as(usize, 1), r.items.len); } +test "edge: without , an atom:link beats a non-permalink guid; a permalink guid stands in" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + const p = try parse(a, "t", + \\c + \\1https://ex.org/id/1 + \\2https://ex.org/id/2 + \\3https://ex.org/post/3 + \\4https://ex.org/post/4 + \\ + , null); + try testing.expectEqual(@as(usize, 4), p.items.len); + try testing.expectEqualStrings("https://ex.org/post/1", p.items[0].url); // explicit link, not the opaque guid + try testing.expectEqualStrings("", p.items[1].url); // an opaque identifier is never used as the URL + try testing.expectEqualStrings("https://ex.org/post/3", p.items[2].url); // isPermaLink defaults to true + try testing.expectEqualStrings("https://ex.org/post/4", p.items[3].url); // rel=self is not the article +} + test "resolveLink: relative references resolve against the feed URL; absolute and schemed pass through" { var arena = std.heap.ArenaAllocator.init(testing.allocator); defer arena.deinit(); diff --git a/src/monitor.zig b/src/monitor.zig index ec24824..b9f637e 100644 --- a/src/monitor.zig +++ b/src/monitor.zig @@ -92,7 +92,8 @@ pub fn normalizeText(a: Allocator, html: []const u8) Allocator.Error![]u8 { var space = true; while (i < html.len) { const c = html[i]; - if (c == '<') { + // Only a plausible tag opener starts markup: "Price < 10" is visible text and must hash. + if (c == '<' and i + 1 < html.len and util.tagOpener(html[i + 1])) { const rest = html[i..]; if (std.mem.startsWith(u8, rest, "")) |e| e + 3 else rest.len; @@ -342,13 +343,13 @@ pub fn check(self: *MonitorAdapter, ctx: *adapter.Context, source: adapter.Sourc }; } - // ---- TLS certificate expiry ---------------------------------------------------------------- - if (target.https and !skip_request and (st.cert_checked == 0 or now - st.cert_checked >= cert_recheck_s)) { + // ---- TLS certificate expiry (cert_warn_days = 0 turns it off: no probe, no alerts) ---------- + if (opt.cert_warn_days > 0 and target.https and !skip_request and (st.cert_checked == 0 or now - st.cert_checked >= cert_recheck_s)) { if (ctx.transport.certNotAfter(target.host, target.port)) |na| { st.cert_not_after = na; st.cert_checked = now; const days_left = @divFloor(na - now, day_s); - const kind: ?[]const u8 = if (na <= now) "expired" else if (opt.cert_warn_days > 0 and days_left <= opt.cert_warn_days) "warn" else null; + const kind: ?[]const u8 = if (na <= now) "expired" else if (days_left <= opt.cert_warn_days) "warn" else null; if (kind) |k| { const id = try std.fmt.allocPrint(a, "cert:{d}:{s}", .{ na, k }); if (!std.mem.eql(u8, id, st.cert_alert)) { @@ -672,6 +673,14 @@ test "tls certificate: warn once per cert, expired, renewal re-arms, http skippe plain.fake.cert = 1; _ = try plain.step(now); try testing.expectEqual(@as(u32, 0), plain.fake.cert_calls); + + // cert_warn_days = 0 is "off": no probe, so not even an expired certificate alerts. + var off = Harness.init(); + defer off.deinit(); + off.src.options = "{\"cert_warn_days\":0}"; + off.fake.cert = now - 3600; + try testing.expectEqual(@as(usize, 0), (try off.step(now)).len); + try testing.expectEqual(@as(u32, 0), off.fake.cert_calls); } test "normalizeText" { @@ -682,4 +691,7 @@ test "normalizeText" { try testing.expectEqualStrings("A & B", try normalizeText(a, "A & B")); try testing.expectEqualStrings(try normalizeText(a, "A & B"), try normalizeText(a, "A & B")); // same text, same hash try testing.expectEqualStrings("x", try normalizeText(a, "x