fix(brief): unblock whyMatters analyst endpoint (middleware 403) + DIGEST_ONLY_USER filter#3255
Conversation
…R filter Three changes, all operational for PR #3248's brief-why-matters feature. 1. middleware.ts PUBLIC_API_PATHS allowlist Railway logs post-#3248 merge showed every cron call to /api/internal/brief-why-matters returning 403 — middleware's "short UA" guard (~L183) rejects Node undici's default UA before the endpoint's own Bearer-auth runs. The feature never executed in prod; three-layer fallback silently shipped legacy Gemini output. Same class as /api/seed-contract-probe (2026-04-15). Endpoint still carries its own subtle-crypto HMAC auth, so bypassing the UA gate is safe. 2. Explicit UA on callAnalystWhyMatters fetch Defense-in-depth. Explicit 'worldmonitor-digest-notifications/1.0' keeps the endpoint reachable if PUBLIC_API_PATHS is ever refactored, and makes cron traffic distinguishable from ops curl in logs. 3. DIGEST_ONLY_USER=user_xxx filter Operator single-user test flag. Set on Railway to run compose + send for one user on the next tick (then unset) — validates new features end-to-end without fanning out. Empty/unset = normal fan-out. Applied right after rule fetch so both compose and dispatch paths respect it. Regression tests: 15 new cases in tests/middleware-bot-gate.test.mts pin every PUBLIC_API_PATHS entry against 3 triggers (empty/short/curl UA) plus a negative sibling-path suite so a future prefix-match refactor can't silently unblock /api/internal/. Tests: 6043 pass. typecheck + typecheck:api clean. biome: pre-existing main() complexity warning bumped 74→78 by the filter block (unchanged in character from pre-PR).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR unblocks the Confidence Score: 5/5Safe to merge — the P0 fix is minimal and exact-match, the new feature is opt-in via env var, and all remaining findings are P2. All three changes are well-scoped: the middleware allowlist uses an exact Set lookup so no other /api/internal/ routes are accidentally unblocked, the DIGEST_ONLY_USER filter defaults to no-op (empty env var), and the new tests cover the regression path. The only finding is a P2 coverage gap in the negative sibling-path test suite that doesn't affect correctness. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(brief): unblock whyMatters analyst e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const path of SIBLING_PATHS) { | ||
| it(`${path} does NOT bypass the UA gate (exact-match allowlist)`, () => { | ||
| const res = call(path, EMPTY_UA); | ||
| assert.ok(res instanceof Response, `${path} must still hit the 403 guard`); | ||
| assert.equal(res.status, 403); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The sibling-path negative tests (SIBLING_PATHS) are tested only with EMPTY_UA. The two other trigger UAs (UNDICI_UA and CURL_UA) are exercised only on allowed paths. The PUBLIC_API_PATHS.has(path) check comes before both BOT_UA and the short-UA guard in middleware, so a sibling path with a curl or undici UA would naturally fall through to one of those two checks and still 403 — meaning the current tests aren't wrong. But they don't pin the exact guard that fires, so a future refactor that moves the PUBLIC_API_PATHS check later in the chain could inadvertently pass one of those UAs through without a CI failure on the sibling-path suite.
…r UAs Greptile flagged: `SIBLING_PATHS` was only tested with `EMPTY_UA`. Under the current middleware chain this is sufficient (sibling paths hit the short-UA OR BOT_UA 403 regardless), but it doesn't pin *which* guard fires. A future refactor that moves `PUBLIC_API_PATHS.has(path)` later in the chain could let a curl or undici UA pass on a sibling path without this suite failing. Fix: iterate the 3 sibling paths against all 3 trigger UAs (empty, short/undici, curl). Every combination must still 403 regardless of which guard catches it. 6 new test cases. Tests: 35 pass in the middleware-bot-gate suite (was 29).
|
Addressed Greptile's P2 in |
1 similar comment
|
Addressed Greptile's P2 in |
… 48h cap) (#3271) * fix(digest): DIGEST_ONLY_USER self-expiring (mandatory until= suffix, 48h cap) Review finding on PR #3255: DIGEST_ONLY_USER was a sticky production footgun. If an operator forgot to unset after a one-off validation, the cron silently filtered every other user out indefinitely while still completing normally (exit 0) — prolonged partial outage with "green" runs. Fix: mandatory `|until=<ISO8601>` suffix within 48h of now. Otherwise the flag is IGNORED with a loud warn, fan-out proceeds normally. Active filter emits a structured console.warn at run start listing expiry + remaining minutes. Valid: DIGEST_ONLY_USER=user_xxx|until=2026-04-22T18:00Z Rejected (→ loud warn, normal fan-out): - Legacy bare `user_xxx` (missing required suffix) - Unparseable ISO - Expiry > 48h (forever-test mistake) - Expiry in past (auto-disable) Parser extracted to `scripts/lib/digest-only-user.mjs` (testable without importing seed-digest-notifications.mjs which has no isMain guard). Tests: 17 cases covering unset / reject / active branches, ISO variants, boundaries, and the 48h cap. 6066 total pass. typecheck × 2 clean. Breaking change on the flag's format, but it shipped 2h before this finding with no prod usage — tightening now is cheaper than after an incident. * chore(digest): address /ce:review P2s on DIGEST_ONLY_USER parser Two style fixes flagged by Greptile on PR #3271: 1. Misleading multi-pipe error message. `user_xxx|until=<iso>|extra` returned "missing mandatory suffix", which points the operator toward adding a suffix that is already present (confused operator might try `user_xxx|until=...|until=...`). Now distinguishes parts.length===1 ("missing suffix") from >2 ("expected exactly one '|' separator, got N"). 2. Date.parse is lenient — accepts RFC 2822, locale strings, "April 22". The documented contract is strict ISO 8601; the 48h cap catches accidental-valid dates but the documentation lied. Added a regex guard up-front that enforces the ISO 8601 shape (YYYY-MM-DD optionally followed by time + TZ). Rejects the 6 Date-parseable-but-not-ISO fixtures before Date.parse runs. Both regressions pinned in tests/digest-only-user.test.mjs (18 pass, was 17). typecheck × 2 clean.
1 participant
Summary
Three operational changes for the brief whyMatters feature shipped in #3248. All scoped to unblocking and validating that one rollout.
🚨 P0: middleware 403 blocking the feature
Railway logs post-#3248 merge showed every cron call to `/api/internal/brief-why-matters` returning 403:
```
[digest] brief-why-matters endpoint HTTP 403
[digest] brief-why-matters endpoint HTTP 403
[digest] brief-why-matters endpoint HTTP 403
... (repeated ~200x per tick)
```
Root cause: middleware.ts's "No user-agent or suspiciously short — likely a script" guard (~L183) rejects Node undici's default UA. The endpoint's own Bearer-auth (`server/_shared/internal-auth.ts`) never runs. The feature shipped but never executed in prod; the three-layer fallback absorbed it silently and the cron kept producing legacy Gemini output.
Same class as `/api/seed-contract-probe` 2026-04-15 (see the existing comment in `PUBLIC_API_PATHS`).
Changes
1. `middleware.ts` — add to `PUBLIC_API_PATHS` allowlist
Endpoint carries its own subtle-crypto HMAC Bearer-auth, so bypassing the generic UA gate is safe. Comment documents the 2026-04-21 incident so the next person understands why it's there.
2. `scripts/seed-digest-notifications.mjs` — explicit UA on fetch
Defense-in-depth. Sets `User-Agent: worldmonitor-digest-notifications/1.0` + `Accept: application/json` so:
3. `DIGEST_ONLY_USER=user_xxx` filter (feature request)
Operator single-user test flag. Set on the Railway digest-notifications service → next cron tick runs compose + send for exactly that user only → unset. Intended for validating new features end-to-end without fanning out to every PRO user. Empty/unset = normal fan-out (production default).
Applied immediately after `rules` is fetched from Convex, so both the compose and dispatch paths respect the filter. Structured log line `[digest] DIGEST_ONLY_USER=user_xxx — filtered N rules → M` makes it visible in Railway output.
Testing
New middleware tests (`tests/middleware-bot-gate.test.mts`)
15 new cases pin every entry of `PUBLIC_API_PATHS` against 3 UA triggers that would normally 403 (empty, short/undici, curl), plus a negative sibling-path suite:
Catches any future prefix-match refactor that would silently unblock the whole `/api/internal/` subtree.
Gates
Deploy + Validation
Immediate (P0 fix)
Merge → Vercel deploys → next digest cron tick (~30 min) runs the analyst path for real. Confirm with Railway logs:
```bash
railway logs --service digest-notifications --lines 400 | grep -E "whyMatters source=|endpoint HTTP"
```
Expected: `whyMatters source=analyst producedBy=analyst hash=...` lines, zero `endpoint HTTP 403`.
Single-user test (validation feature)
Set `DIGEST_ONLY_USER=user_xxx` on Railway → wait for next cron tick → inspect that user's brief at `/api/brief//`. Unset when done.
Post-Deploy Monitoring & Validation
🤖 Generated with Claude Opus 4.7 (1M context, extended thinking) via Claude Code