Backend refactor reconciliation + ASPA verification fix + silent-failure audit #2

Merged

rene.fichtmueller merged 12 commits from merge-prod-snapshot into main 2026-07-16 20:18:49 +00:00

Conversation 0 Commits 12 Files Changed 41 +49404 -1948

rene.fichtmueller commented 2026-07-16 19:19:48 +00:00

Owner

Copy Link

Reconciles production drift with the 2026-07-14 backend refactor, fixes a reversed hop-check direction in the ASPA path verification algorithm (empirically verified against the IETF draft), fixes a duplicate fetchRipeStatCached declaration that silently broke ~9 RIPE Stat checks, and fixes ~9 other silent-failure/fake-success patterns across server.js and local-db-client.js.

Already deployed to production (server.js, local-db-client.js, public/index.html, package.json/package-lock.json) and confirmed live-working (abuse_contact now returns real data, /api/health honestly reports degraded state).

Not yet deployed: the new internal API server wiring for 13 previously-orphaned Fastify routes (dist/api/, ecosystem.config.js peercortex-api entry) -- code-complete and locally tested, but blocked on Postgres credentials not yet configured on Erik.

See commit messages for full detail on each fix.

Reconciles production drift with the 2026-07-14 backend refactor, fixes a reversed hop-check direction in the ASPA path verification algorithm (empirically verified against the IETF draft), fixes a duplicate `fetchRipeStatCached` declaration that silently broke ~9 RIPE Stat checks, and fixes ~9 other silent-failure/fake-success patterns across server.js and local-db-client.js. **Already deployed to production** (server.js, local-db-client.js, public/index.html, package.json/package-lock.json) and confirmed live-working (abuse_contact now returns real data, /api/health honestly reports degraded state). **Not yet deployed**: the new internal API server wiring for 13 previously-orphaned Fastify routes (dist/api/, ecosystem.config.js peercortex-api entry) -- code-complete and locally tested, but blocked on Postgres credentials not yet configured on Erik. See commit messages for full detail on each fix.

rene.fichtmueller added 6 commits 2026-07-16 19:19:48 +00:00

snapshot: preserve production-only changes (Izzy PDF export, adoption tracker, audit scripts) ... 2766872aad

Working tree was 3+ months ahead of git in uncommitted local edits, sitting
on a detached HEAD at d3611a8 (2026-04-09), never reconciled with main.
Committing as-is on its own branch, without touching main or the detached
HEAD, to stop these files existing only on this one server:

- public/index.html + server.js: PDF export add-on gated to client Izzy,
  full Adoption Tracker overlay (ASPA + IPv6 tabs) -- neither exists
  anywhere in git history on any branch until now
- audit/: daily/rotating audit scripts + email report sender, previously
  untracked
- scripts/: tunnel-cleanup.sh, refresh-peeringdb.sh, previously untracked
- deploy-from-scp.sh, webhook-subs.json, hijack-alerts.json,
  aspa-adoption-history.json: previously untracked runtime/deploy state
- public/public/: duplicate mirror of the HTML variants found alongside
  the real public/ dir, preserved as found

No reconciliation with main attempted here -- that needs a deliberate pass,
not a side effect of a backup commit.

merge: reconcile prod-snapshot-2026-07-16 into main ... ceb49d0ab6

Merges the production-only changes (Izzy PDF export via puppeteer,
Adoption Tracker overlay, audit scripts, deploy/utility scripts) that
had been living as uncommitted edits directly on Erik since before the
2026-07-14 backend refactor, on top of current main.

4 real conflicts, resolved by keeping whichever side is actually
functional rather than picking a side mechanically:

- package.json / package-lock.json: trivial union, kept both the
  refactor's pg/playwright deps and the snapshot's puppeteer dep;
  lockfile regenerated via `npm install --package-lock-only` rather
  than hand-edited
- server.js top-of-file: additive, kept both the localDb require
  (backend refactor) and the puppeteer lazy-loader (PDF export)
- server.js /api/health aspa_adoption block: main referenced
  `aspaAdoptionHistory`, a variable that is never declared anywhere
  in this file (confirmed by grep) -- a pre-existing dead reference
  that would throw if this code path ever ran. Kept the snapshot's
  working `aspaAdoptionDailyHistory`/`roaStore.count` version, which
  matches the live /api/health output from production. Also fixed
  the identical bug in the neighboring .catch() fallback block a few
  lines down (same undefined variable, not part of the conflict
  itself -- pre-existing on main, same fix applied for consistency).
- server.js hijack-subscribe/webhooks section (186 lines): main's
  side was just a 2-line comment ("Migrated to
  src/features/hijack-subscribe/"). Kept the snapshot's actual
  working implementation instead, because server.js does not
  require() or import anything from src/features/ or dist/ anywhere
  -- confirmed by grep. The "migration" only ever happened on the
  src/features/ side; server.js's own HTTP routing was never wired
  up to call into it.

IMPORTANT CAVEAT, found while investigating the above and NOT fixed
here: server.js still has 13 other "// Migrated to src/features/..."
comments (bgp-communities, irr-audit, asset-expand, rpki-history,
aspath, looking-glass, ix-matrix, submarine-cables, global-infra,
hijack-alerts (Fastify), changelog, rib, prefix-changes) where the
same thing is true -- the route implementation was deleted from
server.js and never replaced with a working call into src/features/.
These weren't touched by this merge (git didn't flag them as
conflicts, because production's snapshot never touched those
sections either -- only main changed them, by deleting them). If
this server.js is ever deployed as the new production file as-is,
those 13 endpoints will silently 404 or fall through, exactly like
the hijack-subscribe one would have if I'd taken main's side. This
is a pre-existing gap in the 2026-07-14 refactor (PR #1), unrelated
to the Izzy/Adoption Tracker work, and needs a deliberate decision
(wire server.js to src/features/, or confirm those routes are meant
to live on a different process/port entirely) before this branch
should be treated as deployable.

`node --check server.js` passes. Not deployed to Erik as part of
this commit -- Erik's currently-live server.js/public/index.html
are untouched.

fix: remove unused critical-RCE dependency, bump node-cron to drop uuid vuln ... 3b978c4427

- node-whois was declared in package.json but required/imported nowhere
  in the codebase (verified via repo-wide grep) -- pure dead weight that
  was dragging in a critical arbitrary-code-execution chain (underscore
  1.5.2 via optimist, CVSS 9.8). No fixed version exists upstream (latest
  2.1.3 still depends on the same vulnerable optimist/underscore), so the
  only real fix was removal. Zero behavior change since nothing called it.
- node-cron 3.0.3 -> 4.6.0: only real usage is two plain cron.schedule()
  calls in src/features/hijack-alerts/retry-scheduler.ts and
  src/features/aspa-adoption/scheduler.ts -- API unchanged, low risk.
  4.6.0 has zero runtime deps (drops the vulnerable uuid transitive dep)
  and ships its own TS types, so @types/node-cron was also removed to
  avoid duplicate/conflicting type declarations.
- repository/bugs URLs in package.json corrected from the GitHub repo
  (deleted 2026-07-14) to the actual Gitea repo.

12 -> 6 vulnerabilities (6 critical/1 high/5 moderate -> 2 critical/1
high/3 moderate). Remaining 6 are entirely in the vitest/vite/esbuild
dev-tooling chain (vite dev-server SSRF, GHSA-67mh-4wv8-2f99) -- not
production-exposed, and fixing requires a vitest 2.x -> 4.x major bump
that needs its own test-suite verification pass, not folded into this
commit.

tsc --noEmit confirms no new type errors from the node-cron/types
change (remaining TS6133 unused-var warnings in src/sources/*.ts are
pre-existing skeleton-source stubs, unrelated to this commit).

chore: add .security-scan-allowlist for known false positives ... 04bc8e3c4c

Own product name/domain/deploy-path self-references, the maintainer's
public contact address, the bogon-detection feature's hardcoded RFC
5735/6890 range constants, and one already-reviewed home-LAN DB_HOST
default -- all previously confirmed false positives that forced a
manual --no-verify judgment call on every single push to this repo.
Verified against the actual scanner (~/.claude/hooks/lib/security-scan-core.sh):
0 findings remain.

feat: wire up the 13 orphaned Fastify routes via internal API server proxy ... 141911537d

The 2026-07-14 backend refactor (PR #1) moved 13 features' route logic
out of server.js into src/features/*/routes.ts as Fastify plugins, and
left "// Migrated to src/features/X/" comments behind -- but never
actually built or started anything that served those plugins. Found a
complete, unused Fastify app bootstrap already sitting at
src/api/server.ts (added in an earlier commit, 5554c1a, alongside the
BGP hijack/PDF-export/ASPA-adoption work) that imports and registers
all 16 route modules, including 3 (pdf-export, aspa-adoption,
hijack-alerts) with proper Postgres-backed implementations, tests, and
PDF templates -- more capable than the puppeteer-based inline version
kept from the production snapshot. Also confirmed there is no code
anywhere that calls initializeDatabase() or startApiServer(): this
Fastify server had never actually been run, not even once.

What this commit does:

- Fixes 6 real TypeScript compile errors blocking `tsc` from building
  the affected modules cleanly (all pre-existing, none introduced by
  this branch):
  - src/routes/hijack-alerts.ts: route generic was missing `Body` in
    its type param, so it didn't match the handler's own declared type
  - src/features/aspa-adoption/scheduler.ts: node-cron v4's
    ScheduledTask dropped `.destroy()` (relevant now that this branch
    already bumped node-cron 3->4 for the uuid CVE fix) -- `.stop()`
    alone is sufficient
  - src/features/{bgp-communities,rpki-history}/routes.ts:
    `response.json()` typed as `{}` under this @types/node version,
    cast to `any` to match this file's existing loose-typing style
  - src/features/pdf-export/cache-manager.ts: `NodeJS.Timer` ->
    `NodeJS.Timeout` (what setInterval/clearInterval actually use)
  - src/features/pdf-export/renderer.ts: this uses Playwright, not
    Puppeteer -- `timeout` isn't a page.pdf() option in Playwright,
    moved to page.setDefaultTimeout() before the call

- Adds src/api/index.ts as the actual entry point (there wasn't one).
  Calls initializeDatabase() before dynamically importing ./server.js,
  because src/routes/hijack-alerts.ts calls getDatabase() at module
  load time -- a static top-level import of ./server would have
  crashed on startup before initializeDatabase() ever ran. Verified
  this boots cleanly and serves real responses (changelog-data,
  ix-matrix, rib/prefix graceful-503, bgp-communities with a live
  RIPE Stat call) in local testing.

- Wires server.js to reverse-proxy the 11 genuinely-orphaned paths
  (submarine-cables, global-infra, communities, irr-audit,
  asset-expand, rpki-history, aspath, looking-glass, ix-matrix,
  changelog-data, rib/*, prefix-changes) to this new internal server
  via a plain http.request() proxy -- chosen over exposing a second
  Cloudflare Tunnel ingress rule so this stays a same-origin,
  internal-only change with no DNS/tunnel config to touch. Verified
  the proxy mechanism itself against a live instance of the new server.
  Deliberately did NOT proxy /api/webhooks, /api/hijacks, or
  /api/hijack-subscribe -- server.js already serves those paths
  inline (file-backed, live, working), and src/routes/hijack-alerts.ts
  registers the same path names against an empty Postgres table.
  Proxying would have silently shadowed working data with nothing.

- Default port 3102, not 3100: checked Erik and found 3100 already
  bound by an unrelated docker-proxy container. 3102 is free and
  reads as "the second peercortex port" next to the existing 3101.

- Adds a `peercortex-api` PM2 app to ecosystem.config.js on Erik
  (backed up first via rollback-standard.sh) -- config only, NOT
  started. Nothing is deployed to Erik as part of this commit: no
  dist/ upload, no `npm install` for fastify/pg on Erik, no `pm2
  start`. server.js and public/index.html on Erik are untouched.

fix: correct ASPA verification algorithm + fix systemic silent-failure/fake-data patterns ... cc6b81ba8e

2026-07-16 deep-dive audit (server.js, local-db-client.js, src/aspa/validator.ts)
found the flagship ASPA path verification producing wrong results, and a
recurring pattern where an upstream API or the local Postgres DB failing
would present as "checked, clean" instead of surfacing failure. All fixes
in this commit are empirically or textually verified against primary
sources (the IETF draft's actual algorithm text, RFC 5735/6890 bogon
ranges, and hand-built controlled test cases with known-correct answers)
-- not just read-and-guessed.

CRITICAL: ASPA hop-check direction was reversed
--------------------------------------------------
verifyUpstream/verifyDownstream/detectValleys called
hopCheck(collapsed[i-1], collapsed[i]) -- checking the PROVIDER-side AS's
ASPA object for the CUSTOMER, the opposite of
draft-ietf-sidrops-aspa-verification section 5.3's authorized(A(I), A(I+1))
(A(I) = customer, always checked first). Proven empirically: a hand-built,
fully-attested, zero-leak 3-AS path returned "Invalid" before this fix.

verifyDownstream needed a full rewrite, not just an argument swap -- a
downstream path can legitimately contain both an up-ramp AND a down-ramp
(e.g. origin -> providers -> peering point -> validator's providers ->
validator), and the old K/L/uMin/vMax computation didn't correctly
implement the draft's max_up_ramp/min_up_ramp/max_down_ramp/min_down_ramp
four-way computation. Rewrote it from the draft's exact halt conditions,
verified against a worked example fetched from the draft itself (an N=4
path with a single unattested "kink" correctly resolves Valid) plus 3
hand-built scenarios (pure up-ramp, mixed up+down clean, and a definite
leak with failures on both ends -- Invalid).

Same bug existed independently in src/aspa/validator.ts (the MCP-server-
side validator, different codebase, same conceptual error) -- validateUpstream
was already correct there, but validateDownstream naively reversed the path
and reapplied upstream logic, which cannot represent a legitimate down-ramp.
Rewrote it with the same four-ramp algorithm, cross-validated against the
same test scenarios through the compiled output.

Also in the same /api/aspa/verify handler: aspaStore.set(providerAsn, new
Set()) for detected-but-unattested providers made hopCheck report
"NotProviderPlus" (implying a real ASPA object that denies the hop) instead
of "NoAttestation" (we don't know) -- removed; hopCheck's own `!providers`
branch already handles the unset case correctly. Also removed a fallback
that fabricated an ASPA declaration from heuristically-detected BGP
neighbours when aspa_object_exists is false, which contradicted that exact
field in the same response.

ROOT CAUSE of ~9 broken checks: duplicate function declaration
--------------------------------------------------------------
Two `async function fetchRipeStatCached` declarations existed ~150 lines
apart (the original real RIPE-Stat-fetch+cache+throttle implementation,
and a newer local-DB-routing wrapper added during the Postgres refactor).
In JS, the second declaration silently wins for every call site in the
file. The wrapper only recognized 5 URL patterns (announced-prefixes,
asn-neighbours, as-overview, visibility, prefix-size-distribution) and
returned null unconditionally for everything else -- silently breaking
blocklist (Spamhaus), abuse-contact-finder, bgp-updates, routing-status,
looking-glass, whois, reverse-dns-consistency, maxmind-geo-lite-pfx, and
ixs lookups. Renamed the original to fetchRipeStatCachedFromApi and made
the wrapper fall through to it instead of returning null. Verified via an
isolated routing test (blocklist/abuse-contact-finder URLs now reach the
real implementation; the 5 known patterns still route to local DB).

Silent-failure / fake-success patterns fixed
---------------------------------------------
- local-db-client.js: 5 getRipeStat* functions returned status:'ok' with
  empty data on Postgres error (fabricated success) -- now status:'error'.
  server.js's /api/lookup now tracks which sources degraded, surfaces
  meta.degraded_sources, and caches a degraded result for 90s instead of
  the full 5min so a DB hiccup doesn't get repeated to every visitor.
- validateRPKIWithCache: DB errors and genuine "no covering ROA" both
  produced status:"not_found" -- split into "not_found" (real) vs
  "unavailable" (DB error). Updated all 5 call sites that compute RPKI
  coverage percentages to exclude "unavailable" from the denominator (a DB
  hiccup must not lower a network's displayed RPKI/ASPA score), and fixed
  a pre-existing separate typo in the PDF report generator that checked
  for 'not-found' (hyphen) when the actual value is 'not_found' (underscore).
- crossCheckRpki: an empty sample or a failed RIPE-validator lookup both
  counted as a fabricated 100%/"agreement" instead of being excluded --
  now returns agreement_pct:null when nothing was actually compared, and
  the caller no longer averages null into overall_confidence. Also
  documented (not silently left) that bgp.he.net fetching is hardcoded
  off, so the "2-source" prefix/neighbour cross-checks never actually run
  -- dataQuality.cross_checks now reports sources:1 honestly instead of
  claiming a cross-check that never happened.
- lookupAspaFromRpki: added feedLoaded so a Cloudflare-RPKI-feed fetch
  failure (rpkiAspaLastFetch never set) is distinguishable from "this ASN
  genuinely has no ASPA object" -- surfaced as aspa_feed_healthy on both
  ASPA endpoints.
- Bogon/IRR/Spamhaus-blocklist/reverse-DNS/BGP-visibility checks: fetch
  failure -> empty result -> "pass" (or, for abuse-contact/rdns, a
  misleading "fail") -- now report "info" (excluded from the weighted
  health score, matching the existing checkManrsMembership pattern) when
  the underlying data source didn't actually respond.
- Hijack monitoring: checkHijacksForAsn returned [] on DB error,
  indistinguishable from a genuine zero-prefix result. On a subscription's
  first monitoring cycle this permanently persisted an empty baseline,
  silently disabling leak detection forever while the UI kept showing it
  as active. Now returns null on failure; runHijackCheck skips the cycle
  entirely (retries next time) instead of locking in a bad baseline. Fixed
  2 other call sites that would otherwise crash or persist null onto disk.
- WHOIS cache: a transient failure across all 5 RIR sources (RIPE + 4
  RDAP endpoints) was cached as "not found in any RIR database" for 24h,
  identical to a genuinely non-existent ASN. Added a 10min TTL for that
  specific case instead.
- /api/relationships: a neighbourData fetch failure produced "0
  upstreams/downstreams/peers", cached unconditionally for 10min. Applied
  the same 90s-on-suspicious-zero guard the codebase already uses for
  /api/validate (commit 4cf1673) but never applied here.

Verification
------------
node --check on both .js files, tsc --noEmit clean on every touched
TS module (remaining errors are 100% pre-existing, in the unrelated
mcp-server subsystem), npm run build succeeds, isolated empirical tests
for both the ASPA algorithm (3 hand-built scenarios with known-correct
answers, cross-checked against draft text fetched live) and the
fetchRipeStatCached routing fix, and a full local boot test reaching the
exact same pre-existing sandbox-only crash point (missing PEERINGDB_API_KEY,
unrelated to this commit) as a baseline boot before any of today's changes.

Not deployed to Erik as part of this commit -- server.js and
public/index.html there are untouched.

rene.fichtmueller added 1 commit 2026-07-16 19:40:23 +00:00

ci: add build-verify workflow + git-pull-based deploy script ... 0caf7a271e

Two consecutive PeerCortex deploys today (2026-07-16) broke in the same
way: server.js requires local files (src/backend/config.js,
src/backend/services/smtp.js) that were never actually uploaded to Erik,
because the deploy process was ad-hoc 'cat file | ssh' per-file uploads
with no systematic check that every require() target actually exists on
the target machine. A missing npm dependency (pg) caused the same class
of problem separately.

- .github/workflows/build-verify.yml: runs on every push/PR (Gitea Actions
  already has a working runner for this repo, confirmed via the existing
  .github/workflows/security-scan.yml on the github-import/main branch).
  npm ci, syntax-checks server.js + the other top-level .js files, verifies
  every local require() target resolves to an actual file, typechecks
  (informational for now -- see the step comment for why), builds dist/,
  runs the existing vitest suite. Catches both of today's failures before
  they'd ever reach Erik.

- deploy-from-git.sh: replaces the per-file upload process with a single
  git pull + npm ci + require()-check + syntax-check + build + backup +
  pm2 restart sequence, run manually on Erik. Deliberately NOT wired to a
  Gitea Action with SSH secrets -- Erik hosts many unrelated production
  services and main can contain code that hasn't been live-verified yet,
  so the actual deploy trigger stays a human decision. CI catches
  what's broken; this makes running the fix a single repeatable command
  instead of a manually-assembled file list.

rene.fichtmueller added 1 commit 2026-07-16 19:49:31 +00:00

fix: don't fail build-verify/deploy on pre-existing mcp-server tsc exit code ... b11eed9cf6

Discovered live on Erik just now: 'npm run build' returns exit code 2
because of pre-existing src/mcp-server/index.ts type errors (unrelated to
the server.js/API-server deploy path -- already documented in the
2026-07-16 audit), but with noEmitOnError left at its default (false) it
still correctly emits dist/api/index.js, dist/api/server.js, etc. Both
build-verify.yml and deploy-from-git.sh were treating that non-zero exit
as a hard failure -- the CI run for the previous commit failed on exactly
this, and deploy-from-git.sh's  would have aborted a real deploy
at the same step had I used it instead of manual commands. Now both check
for the specific required output files instead of trusting tsc's exit code.

rene.fichtmueller added 1 commit 2026-07-16 19:58:12 +00:00

test: fix pdf-export mocks/assertions for the setDefaultTimeout API change ... 140d730b60

The ASPA/silent-failure-audit fix to renderer.ts (page.pdf() has no
'timeout' option in real Playwright; moved to page.setDefaultTimeout()
before the call) was only typechecked at the time, never run against the
test suite -- the mocks didn't implement setDefaultTimeout and two
assertions still expected the old timeout-in-pdf()-options shape. Caught
by the new build-verify CI workflow on its first real run. 215/215 tests
pass now.

rene.fichtmueller added 1 commit 2026-07-16 20:09:45 +00:00

ci: constrain vitest to a small fixed fork pool for the CI runner ... db83d77162

'npm test' (default vitest thread pool) fails in ~1s on this Gitea Actions
runner for no reason reproducible locally (plain run, CI=true, and a fully
fresh rm -rf node_modules && npm ci all pass 215/215). Forcing the forks
pool with a small fixed worker count (min 1, max 2) is a standard fix for
this class of CI-only vitest worker-pool crash.

rene.fichtmueller added 1 commit 2026-07-16 20:12:54 +00:00

docs: revert speculative vitest pool fix, document as open runner-level issue ... a3ed032687

The forks-pool change didn't fix it (failure went from 1s to 0s, if
anything worse) -- reverting to plain 'npm test' since it's no more or
less broken and is simpler. Documenting this as a known open item needing
someone with actual authenticated Gitea dashboard access, rather than
continuing to guess blindly from outside.

rene.fichtmueller added 1 commit 2026-07-16 20:16:56 +00:00

fix: bogon check didn't recognize local-db-client status:error as unavailable ... 46f96755ac

Follow-up from live testing the 2026-07-16 deploy: bogonDataUnavailable
only checked '!prefixData'/'!neighbourData' (falsy/null), but
local-db-client.js's DB-error signal is a truthy {status:'error', ...}
object, not null (server.js's own fabricated-success fix from earlier
today). So when the Postgres-backed prefix/neighbour source is what's
actually degraded, this check showed status:'pass' at 0 checked prefixes
instead of the more honest 'info' -- data wasn't fabricated (0 was
visible), just the status label didn't reflect it. Fixed with an explicit
isSourceUnavailable() check for both falsy and status:'error'.

rene.fichtmueller merged commit 1edbc345fc into main 2026-07-16 20:18:49 +00:00

rene.fichtmueller referenced this issue from a commit 2026-07-16 20:18:51 +00:00

Merge pull request 'Backend refactor reconciliation + ASPA verification fix + silent-failure audit' (#2) from merge-prod-snapshot into main

rene.fichtmueller referenced this issue from a commit 2026-07-17 06:09:53 +00:00

refactor: extract relationships/compare/quick-ix/peers-find/prefix-detail/ix-detail/topology/whois/enrich routes

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

rene.fichtmueller

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rene.fichtmueller/PeerCortex#2

No description provided.