diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 09f77d2..30d80e8 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -75,6 +75,27 @@ 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. Review of the follow-up + PR: a prerendered item page is declined rather than marked read; an RSS + text `` is found even after an `atom:link`, and `rel` compares + case-insensitively; the selur config path starts with `./` as + selur-compose requires; status views hide certificate expiry while + `cert_warn_days` is 0; an alternate link the UI cannot follow (`ftp:`) no + longer displaces a permalink guid; the selur environment tables are + single-line so its TOML 1.0 parser accepts the file, and the proxy note + covers the Origin check on POST. ==== Changed diff --git a/build/container/stapeln/compose.toml b/build/container/stapeln/compose.toml index 056948b..aa6ebc3 100644 --- a/build/container/stapeln/compose.toml +++ b/build/container/stapeln/compose.toml @@ -27,16 +27,29 @@ version = "1.0" # Primary application service [services.heron] image = "ghcr.io/hyperpolymath/heron:latest.ctp" -ports = ["8484:8484"] -environment = { - APP_HOST = "[::]", - APP_PORT = "8484", - APP_LOG_FORMAT = "json", - APP_DATA_DIR = "/data", -} -volumes = ["heron-data:/data"] +# 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. +# Inline tables stay on one line: selur-compose parses with the toml 0.8 crate (TOML 1.0), which +# rejects a newline inside `{ }`. +environment = { HERON_CONFIG = "/config/heron.json", HERON_DATA = "/data" } +# Without a config heron exits with ConfigNotFound. This is the heron.json compose.yaml mounts: it +# binds the UI to 0.0.0.0 inside the container and relies on the loopback mapping above. +# selur-compose takes a volume as a bind mount only when it starts with "./" or "/" (anything else +# must be a declared named volume), hence "./../" rather than "../". Run it from this directory. +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. +# +# The rokur/svalinn gateway services below are template extras and do NOT make the UI reachable as +# configured: the UI refuses a Host header that is not the bound address or a loopback name +# (DNS-rebinding defence), so a gateway must forward Host as 127.0.0.1:8484. Rokur's documented +# settings offer no Host rewrite, so either front heron with a proxy that can, or drop this tier +# (`just container-keep oci portable`) and use the loopback port above. Such a proxy must also make +# the forwarded Origin match the forwarded Host (http://127.0.0.1:8484), after validating the +# browser's Origin itself: the UI refuses a POST whose Origin differs from its Host (CSRF defence), +# so mark-read/unread and mark-all-read would otherwise answer 403. # Rokur secrets gate: refuses to start (and refuses to proxy) unless the # declared secrets are present; emits a structured audit log of every access. @@ -44,13 +57,7 @@ healthcheck = { test = "curl -sf http://localhost:8484/health", interval = "30s" [services.rokur] image = "ghcr.io/hyperpolymath/rokur:latest.ctp" ports = ["8081:8081"] -environment = { - ROKUR_BACKEND = "http://heron:8484", - ROKUR_LISTEN = "[::]:8081", - ROKUR_REQUIRED_SECRETS = "", - ROKUR_AUDIT_LOG = "/var/log/rokur/audit.jsonl", - ROKUR_LOG_FORMAT = "json", -} +environment = { ROKUR_BACKEND = "http://heron:8484", ROKUR_LISTEN = "[::]:8081", ROKUR_REQUIRED_SECRETS = "", ROKUR_AUDIT_LOG = "/var/log/rokur/audit.jsonl", ROKUR_LOG_FORMAT = "json" } volumes = ["rokur-audit:/var/log/rokur"] depends_on = ["heron"] restart = "always" @@ -61,11 +68,7 @@ healthcheck = { test = "curl -sf http://localhost:8081/health", interval = "30s" [services.svalinn] image = "ghcr.io/hyperpolymath/svalinn:latest.ctp" ports = ["443:443", "80:80"] -environment = { - SVALINN_BACKEND = "http://rokur:8081", - SVALINN_POLICY_FILE = "/etc/svalinn/gatekeeper.yaml", - SVALINN_TLS_AUTO = "true", -} +environment = { SVALINN_BACKEND = "http://rokur:8081", SVALINN_POLICY_FILE = "/etc/svalinn/gatekeeper.yaml", SVALINN_TLS_AUTO = "true" } volumes = ["svalinn-config:/etc/svalinn:ro"] depends_on = ["rokur"] restart = "always" 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..5ca12b9 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,26 @@ 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/` (an FTS3 identifier in SQLite's amalgamation 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, + and that rokur proxies to the app (`ROKUR_BACKEND`), which rokur's own + documented settings do not include. Its selur `compose.toml` and + `compose.example.toml` use multi-line inline tables, which selur's TOML 1.0 + parser (toml 0.8) rejects, so neither loads (fixed here in `compose.toml`). +** `metadatastician/selur`: `selur-compose` treats a volume that does not + start with `./` or `/` as a named volume, so `../file` bind mounts fail + validation (heron works round it with `./../heron.json`). * 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 +130,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..0f90551 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. A prefetch or prerender (`Sec-Purpose: prefetch`) of an item page is declined with 503, so only the real visit marks it read. == See also diff --git a/heron-polyfeed-reader_chora.deed b/heron-polyfeed-reader_chora.deed index 23aed20..4683fad 100644 --- a/heron-polyfeed-reader_chora.deed +++ b/heron-polyfeed-reader_chora.deed @@ -30,7 +30,7 @@ ; equal canon.lock [canon.artifacts]; canon.lock [canon.lockstep].order is ; spine-adopts-then-canon-releases, so this clause moves FIRST. (canon - :version "2.1.3" + :version "2.1.4" :criteria-sha256 "6a5aa8857bd0d0d58ef48827938ca17c251b6b854dacf59d306388d61694d82a" :gates-sha256 "2f31f46d4753432e0aa8c9c28f339e18e062c98dc6e6ef4ecbb76993cf37638b" :lockstep-since "2026-09-17" 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..7c67f15 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(); @@ -246,8 +246,9 @@ fn cmdMonitors(app: *app_mod.App, store: *store_mod.Store, out: *Io.Writer, aren const state: []const u8 = if (ms.blocked) "blocked" else if (ms.up) |u| (if (u) "up" else "DOWN") else "unknown"; const since = if (ms.since > 0) util.fmtSpan(&b1, now - ms.since) else "-"; const http_s = if (ms.status > 0) try std.fmt.bufPrint(&b2, "{d}", .{ms.status}) else "-"; - const cert = if (ms.cert_not_after > 0) - (if (ms.cert_not_after <= now) "EXPIRED" else util.fmtSpan(&b3, ms.cert_not_after - now)) + const not_after = monitor_mod.shownCertNotAfter(arena, job.options, ms); + const cert = if (not_after > 0) + (if (not_after <= now) "EXPIRED" else util.fmtSpan(&b3, not_after - now)) else "-"; try out.print("{s:<20} {s:<8} {s:>9} {s:>6} {s:>10} {s}\n", .{ job.name, state, since, http_s, cert, try util.termSafe(arena, ms.err) }); diff --git a/src/config.zig b/src/config.zig index e665879..de34380 100644 --- a/src/config.zig +++ b/src/config.zig @@ -57,15 +57,44 @@ 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`: 127.0.0.0/8, ::1 and +/// the IPv4-mapped ::ffff:127.0.0.0/104. Decided on the parsed value, not the spelling, so +/// `0:0:0:0:0:0:0:1` is loopback and `127.evil` (not an address) is not. +pub fn isLoopback(addr: []const u8) bool { + const ip = std.Io.net.IpAddress.parse(addr, 0) catch return false; + return switch (ip) { + .ip4 => |a| a.bytes[0] == 127, + .ip6 => |a| std.mem.eql(u8, &a.bytes, &std.Io.net.Ip6Address.loopback(0).bytes) or + (std.mem.eql(u8, a.bytes[0..12], &ip4_mapped_prefix) and a.bytes[12] == 127), + }; +} + +const ip4_mapped_prefix = [12]u8{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff }; + 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 +109,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 +205,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 +286,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..e4c50d8 100644 --- a/src/feed.zig +++ b/src/feed.zig @@ -70,6 +70,16 @@ fn nodeText(a: Allocator, n: *const xml.Node) Allocator.Error![]const u8 { return std.mem.trim(u8, try n.deepText(a), " \t\r\n"); } +/// An http(s) URL or a relative reference (no scheme, RFC 3986 §4.2): the only links the web UI +/// renders as anchors. A `ftp:` or `mailto:` alternate must not displace a usable permalink. +fn webOrRelative(href: []const u8) bool { + const colon = std.mem.indexOfScalar(u8, href, ':') orelse return true; + const scheme = href[0..colon]; + // A ':' after the first '/', '?' or '#' belongs to the path or query, not to a scheme. + if (std.mem.indexOfAny(u8, scheme, "/?#") != null) return true; + return std.ascii.eqlIgnoreCase(scheme, "http") or std.ascii.eqlIgnoreCase(scheme, "https"); +} + fn firstNonEmpty(cands: []const []const u8) []const u8 { for (cands) |c| if (c.len > 0) return c; return ""; @@ -82,14 +92,36 @@ fn childText(a: Allocator, n: *const xml.Node, name: []const u8) Allocator.Error fn rssItem(a: Allocator, source: []const u8, n: *const xml.Node) Allocator.Error!?Item { const title = try childText(a, n, "title"); - var link = try childText(a, n, "link"); var guid = try childText(a, n, "guid"); if (guid.len == 0) if (n.attr("about")) |ab| { // RDF: rdf:about is the identifier guid = ab; }; + // The item URL, in order: the first RSS with text (an atom:link may come before it in + // document order, so every link child is looked at), else an atom:link href whose rel is absent + // or "alternate" (RFC 8288: relation names compare case-insensitively), 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 link: []const u8 = ""; + var alt_href: []const u8 = ""; + var c = n.first_child; + while (c) |l| : (c = l.next) { + if (!l.is("link")) continue; + const text = try nodeText(a, l); + if (text.len > 0) { + link = text; + break; + } + if (alt_href.len == 0) if (l.attr("href")) |href| { + if (std.ascii.eqlIgnoreCase(l.attr("rel") orelse "alternate", "alternate") and webOrRelative(href)) alt_href = href; + }; + } 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 ""; + link = alt_href; + 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 +361,56 @@ 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 + + const q = try parse(a, "t", + \\c + \\5https://ex.org/post/5 + \\6x6 + \\ + , null); + try testing.expectEqual(@as(usize, 2), q.items.len); + try testing.expectEqualStrings("https://ex.org/post/5", q.items[0].url); // text after an atom:link + try testing.expectEqualStrings("https://ex.org/post/6", q.items[1].url); // rel compares case-insensitively + + // An alternate the UI cannot link (ftp:) does not displace a usable permalink guid; a relative one is kept. + const r = try parse(a, "t", + \\c + \\7https://ex.org/post/7 + \\8x8 + \\ + , null); + try testing.expectEqualStrings("https://ex.org/post/7", r.items[0].url); + try testing.expectEqualStrings("/post/8", r.items[1].url); // resolved against the feed URL later, by the adapter +} + +test "webOrRelative" { + try testing.expect(webOrRelative("https://ex.org/a")); + try testing.expect(webOrRelative("HTTP://ex.org/a")); + try testing.expect(webOrRelative("/a/b:c")); + try testing.expect(webOrRelative("a?x=1:2")); + try testing.expect(webOrRelative("post/9")); + try testing.expect(!webOrRelative("ftp://ex.org/a")); + try testing.expect(!webOrRelative("mailto:x@ex.org")); + try testing.expect(!webOrRelative("javascript:alert(1)")); +} + 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..b1c2f01 100644 --- a/src/monitor.zig +++ b/src/monitor.zig @@ -85,6 +85,13 @@ pub fn parseOptions(a: Allocator, text: []const u8) Options { return std.json.parseFromSliceLeaky(Options, a, text, .{ .ignore_unknown_fields = true }) catch .{}; } +/// The certificate expiry a status view should show (0 = none). While `cert_warn_days` is 0 the +/// check is off, so nothing is shown even if saved state still holds an earlier probe's result: the +/// poll that clears it can be a whole interval away, for instance after a restart. +pub fn shownCertNotAfter(a: Allocator, options: []const u8, st: State) i64 { + return if (parseOptions(a, options).cert_warn_days == 0) 0 else st.cert_not_after; +} + /// Visible-text normalisation for change hashing. pub fn normalizeText(a: Allocator, html: []const u8) Allocator.Error![]u8 { var out: std.ArrayList(u8) = .empty; @@ -92,7 +99,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 +350,20 @@ 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) ---------- + // Turning it off also forgets what an earlier probe found, or `heron monitors` and the web + // status page would show a stale countdown (or EXPIRED) for a check that no longer runs. + if (opt.cert_warn_days == 0) { + st.cert_not_after = 0; + st.cert_checked = 0; + st.cert_alert = ""; + } + 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 +687,33 @@ 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); + + // Switching an existing monitor to 0 clears the certificate status an earlier probe saved. + var was_on = Harness.init(); + defer was_on.deinit(); + was_on.fake.cert = now + 5 * day_s; + _ = try was_on.step(now); + try testing.expect(was_on.state().cert_not_after != 0); + was_on.src.options = "{\"cert_warn_days\":0}"; + _ = try was_on.step(now + 60); + try testing.expectEqual(@as(i64, 0), was_on.state().cert_not_after); + try testing.expectEqual(@as(i64, 0), was_on.state().cert_checked); + try testing.expectEqualStrings("", was_on.state().cert_alert); + + // Until that poll (e.g. right after a restart), status views still hide the saved value. + const saved: State = .{ .cert_not_after = now + 5 * day_s }; + const a = was_on.arena.allocator(); + try testing.expectEqual(@as(i64, 0), shownCertNotAfter(a, "{\"cert_warn_days\":0}", saved)); + try testing.expectEqual(now + 5 * day_s, shownCertNotAfter(a, "{\"cert_warn_days\":14}", saved)); + try testing.expectEqual(now + 5 * day_s, shownCertNotAfter(a, "{}", saved)); // default 14 } test "normalizeText" { @@ -682,4 +724,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