Skip to content

fix(review): warn at boot when PUBLIC_API_ORIGIN/PUBLIC_SITE_ORIGIN look private#4185

Merged
JSONbored merged 1 commit into
mainfrom
fix/visual-capture-public-origin-advisory-4180
Jul 8, 2026
Merged

fix(review): warn at boot when PUBLIC_API_ORIGIN/PUBLIC_SITE_ORIGIN look private#4185
JSONbored merged 1 commit into
mainfrom
fix/visual-capture-public-origin-advisory-4180

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Advances fix(review): visual-capture screenshots render as broken images on self-host (private origin) #4184: PR test(ui): add AGPL-3.0 badge to footer to validate visual-capture pipeline #4180's "Visual preview" table rendered both thumbnails as broken images with a blank "Before (production)" column. Root cause: this self-host instance's PUBLIC_API_ORIGIN is a bare Tailscale MagicDNS (*.ts.net) hostname — reachable from the tailnet, not from GitHub's own servers, which is what actually fetches the <img src> in a PR comment. PUBLIC_SITE_ORIGIN (the "before" source) was never set at all, and wasn't even documented in .env.example's self-host section.
  • The live instance's own env values are a deployment config change outside this repo (not something a PR can fix) — this PR adds the missing signal so that misconfiguration is loud at boot instead of silently producing broken PR comments for weeks.
  • Adds publicOriginReachabilityAdvisory + publicOriginAcknowledgedGaugeValue (src/selfhost/health.ts), mirroring the existing sqliteBackupAdvisory/backupAcknowledgedGaugeValue pattern exactly: a non-blocking boot-time warning (never a hard preflight failure, since a legitimately Tailscale-Funnel-exposed *.ts.net node would otherwise false-positive) plus a matching Prometheus gauge, with a PUBLIC_ORIGIN_ACKNOWLEDGED=true escape hatch for that case.
  • Documents PUBLIC_SITE_ORIGIN in .env.example for the first time, and fixes PUBLIC_API_ORIGIN's misleading "not needed once the App credentials are configured" framing in both example files (it stays load-bearing forever, for every visual-capture screenshot).

Scope

Validation

  • git diff --check
  • npm run typecheck
  • npm run docs:drift-check
  • npm run selfhost:env-reference — regenerated, PUBLIC_SITE_ORIGIN/PUBLIC_ORIGIN_ACKNOWLEDGED now appear.
  • npx vitest run test/unit/selfhost-health.test.ts — 30/30 passing, 100% lines/branches/functions on src/selfhost/health.ts.
  • npm run test:changed (against origin/main) — 67/67 passing across the 2 files it selected.
  • npm run test:workers, build:mcp, test:mcp-pack, ui:openapi:check, ui:lint, ui:typecheck, ui:build, npm audit — not re-run locally (no worker-entry/MCP/OpenAPI/UI-component code touched); CI runs them authoritatively.

Safety

  • No secrets, wallet/hotkey/trust-score/reward data anywhere.
  • src/server.ts is Codecov-ignored; the new pure logic lives in src/selfhost/health.ts, fully covered.
  • Non-blocking by design: a false-positive match (e.g. Funnel) degrades to a log line + a 0-value gauge, never a boot failure — an explicit, documented trade-off against catching real misconfigurations by default.
  • No UI changes — no UI Evidence section needed.

…ook private (#4184)

Visual-capture screenshots embed these origins directly as public <img src>
values in PR comments; a private/tailnet hostname (e.g. a bare Tailscale
MagicDNS address) makes every screenshot render as a broken image with no
other signal. Adds a boot-time advisory + gauge mirroring the existing
sqliteBackupAdvisory pattern, and documents PUBLIC_SITE_ORIGIN (previously
undocumented) alongside a corrected PUBLIC_API_ORIGIN description.
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 9116c46 Commit Preview URL

Branch Preview URL
Jul 08 2026, 10:24 AM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (7d4117c) to head (9116c46).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4185   +/-   ##
=======================================
  Coverage   93.73%   93.74%           
=======================================
  Files         387      387           
  Lines       36371    36393   +22     
  Branches    13316    13326   +10     
=======================================
+ Hits        34092    34115   +23     
  Misses       1621     1621           
+ Partials      658      657    -1     
Files with missing lines Coverage Δ
src/selfhost/health.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-08 10:36:53 UTC

6 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This adds a boot-time, non-blocking advisory that flags PUBLIC_API_ORIGIN/PUBLIC_SITE_ORIGIN when they resolve to a loopback/RFC1918/mDNS/.internal hostname or a bare *.ts.net MagicDNS address, mirroring the existing sqliteBackupAdvisory/backupAcknowledgedGaugeValue pattern exactly (advisory fn, gauge fn, ack env var, wiring in server.ts, docs in both .env example files, and env-reference regeneration). The private-hostname classification (loopback, 10/8, 172.16/12, 192.168/16, .local/.internal, bracketed ::1) and the softer *.ts.net-only-with-Funnel case are logically sound and correctly reachable at boot via process.env, and the test suite exercises both branches of every conditional (acknowledged vs not, API vs Site origin, boundary values at 172.15/172.32, unparseable-value silence). No schema/migration is implicated since this is pure env-driven config, and the change is appropriately scoped to advisory-only (never a hard preflight failure) per the PR's own stated rationale for avoiding Funnel false-positives.

Nits — 5 non-blocking
  • src/selfhost/health.ts: isDefinitelyPrivateHostname only covers IPv4 RFC1918/loopback and the literal `[::1]`; it doesn't flag IPv6 ULA (fc00::/7) or link-local (fe80::/10) hosts, so a self-host operator running dual-stack with a ULA address would get no warning — worth a follow-up if that's a realistic self-host deployment shape.
  • src/selfhost/health.ts:170-176: the RFC1918 octet comparisons (10, 172/16/31, 192/168) are unavoidable magic numbers here since they encode a well-known standard, but a short inline comment naming the RFC would save the next reader a lookup.
  • test/unit/selfhost-health.test.ts: consider also asserting the exact `suspect` value chosen when both PUBLIC_API_ORIGIN and PUBLIC_SITE_ORIGIN are simultaneously bad, to lock in the API-origin-checked-first ordering implied by `.find()`.
  • Consider extracting the RFC1918 octet bounds (16/31, 168) into small named constants in health.ts purely for readability given the assertive review profile — non-blocking.
  • A short CHANGELOG or CONTRIBUTING note pointing self-host operators at PUBLIC_ORIGIN_ACKNOWLEDGED could reduce support questions when the advisory fires for legitimately Funnel-exposed nodes, though the .env.example comment already covers this reasonably well.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 43 merged, 486 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 52 PR(s), 486 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 52 PR(s), 486 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop after /
/ mobile after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant