Skip to content

fix(selfhost): redact Orb broker enrollment tokens from telemetry - #3193

Merged
JSONbored merged 1 commit into
mainfrom
fix/orb-token-value-redaction
Jul 4, 2026
Merged

fix(selfhost): redact Orb broker enrollment tokens from telemetry#3193
JSONbored merged 1 commit into
mainfrom
fix/orb-token-value-redaction

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Advances chore(orb): verify brokered self-host relay and telemetry #1825 (brokered self-host relay + telemetry audit). This PR is the code-level audit requested by the issue plus one genuine, narrow fix it turned up. It does not close chore(orb): verify brokered self-host relay and telemetry #1825 — see "What's not done" below for the parts of the issue this PR does not cover.

  • Fix: the Orb broker's enrollment id/secret (createOpaqueToken("orbenr"/"orbsec"), src/orb/broker.ts) are bare opaque tokens with no token/secret-named field to trip the existing key-based redaction when they appear as a plain value — e.g. quoted inside a broker error message. Five independent value-based token scrubbers across the codebase already recognized ghp_/github_pat_/gts_/glpat-/sk- (and xoxb- in one case) but missed the Orb broker's own prefixes:

    • src/selfhost/sentry.ts (SECRET_VALUE, the Sentry beforeSend scrubber)
    • src/db/repositories.ts (PRODUCT_USAGE_TOKEN_VALUE, persisted product-usage telemetry)
    • src/services/control-panel-roles.ts (sanitizeRoleText)
    • src/services/miner-dashboard-recommendations.ts (FORBIDDEN_TOKEN)
    • src/services/weekly-value-report.ts (sanitizeReportText)

    All five now also match orbenr_/orbsec_. Every fix is a one-line regex addition; each is covered by a new regression test that fails without the fix and passes with it (verified locally by reverting the five source lines and re-running the new tests — see Validation).

End-to-end audit notes (#1825 deliverable)

Audited by reading the code end to end (no live broker access, so this is a rigorous static audit, not a live integration test — see "What still needs an operator to verify"). Production runs ORB_RELAY_MODE=pull, so the pull path got the primary focus; push-mode code was read too since the issue asks for both.

Token broker (mint / cache / expiry / refresh / outage) — src/orb/broker-client.ts, src/github/app.ts, src/orb/broker.ts:

  • Minting: fetchBrokeredInstallationToken (src/orb/broker-client.ts:53) exchanges ORB_ENROLLMENT_SECRET for a token via POST /v1/orb/token, 25s timeout, throws on non-OK or a tokenless body.
  • Caching: mintInstallationToken (src/github/app.ts:223) writes the brokered token into the same installation-token cache the App-key path uses (installationTokenCache, ~1h TTL, 2‑min safety margin), single-flighted via inFlightMints (src/github/app.ts:130) so a cold cache doesn't thundering-herd the broker.
  • Expiry/refresh: an unparseable expiresAt falls back to a 50‑min default rather than propagating NaN into the cache (src/orb/broker-client.ts:75-79, with an explicit comment on why); withInstallationTokenRetry (src/github/app.ts:176) force-refreshes once on a 401/permission-scope rejection.
  • Broker-outage behavior: fails safe, not hammering the broker. On a mint failure, mintInstallationToken serves the still-valid cached token if one exists ("stale-token grace", src/github/app.ts:254-270, already tested in test/unit/github-app.test.ts:549 and :572); only when there is no valid cache does it rethrow (→ the queue's existing retry/DLQ handling). There is no busy-retry loop inside the mint path itself — already correct.
  • Server-side (src/orb/broker.ts): brokerOrbToken caches the minted token encrypted-at-rest, re-checks install eligibility on every exchange (not just at issue time), and orders the misconfiguration check after the enrollment-secret check so an unauthenticated caller can't fingerprint broker misconfiguration (#2710, already fixed).
  • Verdict: already handled correctly. No fix needed here.

Webhook relay — registration, drain, ack, retry, stale-event handling — src/orb/broker-client.ts, src/orb/relay.ts, src/selfhost/monitored-work.ts, src/server.ts:

  • Registration: registerOrbRelayTargetWithRetry (src/orb/broker-client.ts:195) retries on a 1‑minute timer with a 5‑minute backoff (ORB_RELAY_REGISTER_RETRY_BACKOFF_MS) once registration fails, instead of the old one-shot boot-time attempt — already fixed for #selfhost-runtime-drift.
  • Pull-mode degraded registration does not page incorrectly (explicit issue acceptance criterion): registerOrbRelayWithMonitor (src/selfhost/monitored-work.ts:112) logs a warning, not an error, for a pull-mode registration failure, because the drain loop still functions once a later attempt succeeds; push-mode failure is an error (the container is genuinely deaf with no fallback). Already tested (test/unit/selfhost-monitored-work.test.ts:222 and :243).
  • Drain (pull mode): drainOrbRelay (src/orb/broker-client.ts:222) POSTs the outstanding ack list and pulls the next batch every 15s (src/server.ts:1003); best-effort, returns [] on any failure so the next tick retries.
  • Ack: acks are only added to the pending list once enqueueWebhookByEnv durably accepts the event (drainOrbRelayWithMonitor, src/selfhost/monitored-work.ts:89) — an enqueue failure is never acked, so it is redelivered on the next drain.
  • Retry (push mode): retryFailedRelays (src/orb/relay.ts:377) runs off the retry-orb-relay cron, up to 5 attempts within a 1‑hour TTL, with a 5‑minute per-row backoff (RELAY_RETRY_BACKOFF_MINUTES) so a sustained outage doesn't re-hammer a down container or a degraded Orb every ~2‑minute tick.
  • Stale-event handling: both the pull-mode pending queue (orb_relay_pending, 24h TTL, RELAY_PENDING_TTL_HOURS) and the push-mode failure queue (orb_relay_failures, 1h TTL / 5 attempts) prune and emit an alertable error-level structured log on drop (orb_relay_pending_dropped / orb_relay_events_dropped) so silent event loss is visible — already tested in test/integration/orb-relay.test.ts:473 and :790.
  • Verdict: already handled correctly and already well-tested, including the exact "pull-mode degraded registration must not page" acceptance criterion.

Outcome telemetry export — src/selfhost/orb-collector.ts:

  • Anonymization default is true (ORB_ANONYMIZE), repo/PR identifiers are HMAC'd with a dedicated per-instance secret (never the App key or webhook secret) generated once and persisted in system_flags.
  • Air-gap: ORB_AIR_GAP=true short-circuits the export to a no-op before any network call, including for brokered instances — already tested (test/unit/selfhost-orb-collector.test.ts:92, :97).
  • Retry/failure visibility: a non-OK response or thrown fetch increments gittensory_orb_export_errors_total and returns 0 (no throw, no crash of the hourly export timer); the watermark cursor only advances on a confirmed 2xx, so a failed batch is retried whole on the next run — already tested (test/unit/selfhost-orb-collector.test.ts:184, :192).
  • Verdict: already handled correctly. No fix needed here.

Logging/Sentry context (the one gap found and fixed):

  • Key-based redaction (SECRET_KEY in src/selfhost/sentry.ts, matching token|secret|key|password|...) already catches any field literally named secret/token/etc. — confirmed still solid.
  • The gap was value-based redaction of free-text strings (an Error.message, a log line) where a secret appears as a bare value with no matching field name. None of the current throw new Error(...) call sites in src/orb/broker-client.ts interpolate the raw enrollment secret directly, so this was not an active leak in the code paths I traced — it is a defense-in-depth gap: if a future broker error message (or an upstream broker's own error body, echoed via safeOrbRelayRegisterErrorHint) ever quoted the secret bare, it would have reached Sentry/telemetry unredacted. Fixed by adding orbenr_|orbsec_ to the five value-scrubber regexes listed above.
  • Raw webhook bodies: never logged; enqueueWebhookByEnv only records deliveryId/eventName/action/installationId/repositoryFullName/payloadHash (src/db/repositories.ts recordWebhookEvent call sites), never the body itself.
  • Private repo config / unhashed identifiers: hashedInstallationContext (src/selfhost/sentry.ts:231) hashes installation_id before it reaches Sentry tags/context; PAYLOAD_KEY drops body/payload/patch/diff/config/repoConfig-named fields entirely rather than truncating them.

What's not done (honest scope boundary)

This PR does not attempt:

  • A reproducible pull-mode / push-mode smoke checklist (the issue's "Deliverables" ask) — that requires a live broker and is out of scope for a static-code-review PR.
  • Website docs updates for GitHub App / Orb setup and troubleshooting (also listed under Deliverables) — a separate, larger docs PR.
  • A "clear recommendation for stable ingress vs. pull-mode default for NAT/tailnet deployments" — the code already defaults sensibly (pull mode needs no public origin, push mode requires PUBLIC_API_ORIGIN), but a written operator-facing recommendation belongs in the docs PR above, not buried in a code-fix PR.

What still needs an operator to verify live

  • Actual broker behavior under a real network partition (DNS failure, TLS reset, slow-loris) versus the synthetic non-OK/timeout cases exercised in tests — static review can confirm the code paths exist and are exercised by unit tests, not that they behave identically against a real degraded broker.
  • Whether the 15s pull-drain interval and 60s registration-retry interval are the right cadence under real fleet-wide broker load (both are currently fixed constants, not configurable).
  • End-to-end confirmation that a push-mode relay target behind a real public ingress (TLS terminator, firewall, etc.) round-trips correctly — the code path is unit- and integration-tested against mocked fetch, not a live public endpoint.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue (Advances chore(orb): verify brokered self-host relay and telemetry #1825); this PR is a partial, honest step, not a full close (see above).

Validation

  • git diff --check
  • npm run actionlint (no workflow files touched; not run)
  • npm run typecheck
  • npm run test:coverage (not run in full locally per this repo's own guidance — CI already runs the full gate; ran the 5 directly affected test files plus npm run test:changed, both green, and verified revert-and-confirm-fails for each new test)
  • npm run test:workers (no Worker-runtime code touched; not run)
  • npm run build:mcp (no MCP code touched; not run)
  • npm run test:mcp-pack (no MCP code touched; not run)
  • npm run ui:openapi:check (no API/schema changes; not run)
  • npm run ui:lint / npm run ui:typecheck / npm run ui:build (no apps/gittensory-ui/** changes; not run)
  • npm audit --audit-level=moderate (no dependency changes; not run — CI's dependency-review job covers this)
  • New/changed behavior has tests: 5 new regression tests (one per fixed file), each verified to fail on the pre-fix code and pass on the post-fix code (git stash the 5 source-line fixes, re-run the 5 new tests → all 5 fail cleanly with no other regressions; restore, re-run → all pass). Also ran npm run test:changed against the full diff: 216 test files / 5477 tests passed, 0 failed.

If any required check was skipped, explain why:

  • This is a src/**-only change with no UI, MCP, Worker-runtime, API/schema, or dependency surface touched, so the corresponding gate steps are inapplicable; typecheck plus the directly affected unit tests plus test:changed (full affected-test sweep) are the faithful local signal for a change this narrow, and CI runs the complete test:ci gate including test:coverage regardless.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed. (All new test fixtures use obviously-fake placeholder tokens, e.g. orbsec_ + repeated filler characters — never real-looking secrets.)
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — this PR only extends existing redaction regexes, no auth/session logic changed; each extension has a positive-redaction regression test.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changed.)
  • Visible UI changes include a UI Evidence section. (N/A — no visible UI changed.)
  • Public docs/changelogs are updated where needed. (Docs updates are explicitly deferred to a follow-up PR per chore(orb): verify brokered self-host relay and telemetry #1825's own Deliverables list — see "What's not done" above.)

Notes

  • Full audit trace (file:line) for anyone picking up the remaining chore(orb): verify brokered self-host relay and telemetry #1825 work:
    • Broker client: src/orb/broker-client.ts
    • Broker server: src/orb/broker.ts
    • Relay (registration/drain/retry/stale-event): src/orb/relay.ts
    • Installation-token cache + brokered mint: src/github/app.ts:132-331
    • Server wiring (drain loop, registration retry loop): src/server.ts:939-1011
    • Monitored-work wrappers (Sentry cron monitors, pull/push logging asymmetry): src/selfhost/monitored-work.ts
    • Outcome telemetry exporter: src/selfhost/orb-collector.ts
    • Sentry scrubber (this PR's fix): src/selfhost/sentry.ts:37-47

)

The Orb broker's enrollment id/secret (createOpaqueToken("orbenr"/"orbsec"),
src/orb/broker.ts) are bare opaque tokens with no "token"/"secret"-named
field to trip key-based redaction when they appear as a plain value, e.g.
quoted inside an error message. The value-based token scrubbers in
src/selfhost/sentry.ts (Sentry beforeSend), src/db/repositories.ts
(product-usage telemetry), src/services/control-panel-roles.ts,
src/services/miner-dashboard-recommendations.ts, and
src/services/weekly-value-report.ts already recognized ghp_/github_pat_/
gts_/glpat-/sk- (and xoxb- in one case) but missed the Orb broker's own
prefixes, so a broker error/log line quoting a raw enrollment secret would
have reached Sentry or persisted telemetry unredacted.
@superagent-security

Copy link
Copy Markdown
Contributor

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

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

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-04 19:33:34 UTC

10 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The PR consistently extends the existing value-based telemetry scrubbers to recognize bare Orb enrollment id/secret prefixes across Sentry, product-usage metadata, role text, miner dashboard rerun reasons, and weekly reports. The changed paths are narrow and each has a regression test that exercises the relevant production sanitizer surface. I do not see a reachable correctness break in the visible diff, but the duplicated regex literals remain a drift risk and the tests use only simple repeated letters rather than the real opaque-token alphabet.

Nits — 7 non-blocking
  • nit: src/selfhost/sentry.ts:44 uses `[A-Za-z0-9_]` for `gts|orbenr|orbsec` while the other scrubbers use `[A-Za-z0-9_=-]`; confirm this exactly matches `createOpaqueToken` output, especially if it can emit `-`, or align the class with the other scrubbers.
  • nit: test/unit/selfhost-sentry.test.ts:263 and the other new tests use repeated `a`/`b` characters, so they do not prove redaction for the full opaque-token alphabet that production emits.
  • nit: src/db/repositories.ts:6037, src/services/control-panel-roles.ts:295, src/services/miner-dashboard-recommendations.ts:49, and src/services/weekly-value-report.ts:414 still duplicate the sensitive-token prefix list, so future token families can drift again.
  • src/selfhost/sentry.ts:44: either change the Orb/gts branch to the same token tail used by the other scrubbers or add a comment tying the narrower class to `createOpaqueToken` if that producer cannot emit hyphen/equals.
  • test/unit/selfhost-sentry.test.ts:263 and sibling tests: build the fixture tokens with representative opaque-token characters such as `_` and `-`, then assert `not.toContain` the exact original token in every changed sanitizer.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1825
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 60 registered-repo PR(s), 50 merged, 442 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 442 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 60 PR(s), 442 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
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.

🟩 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

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.73%. Comparing base (408ea48) to head (b357e18).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3193   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         270      270           
  Lines       29714    29714           
  Branches    10840    10840           
=======================================
  Hits        28151    28151           
  Misses        917      917           
  Partials      646      646           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.35% <100.00%> (ø)
src/selfhost/sentry.ts 100.00% <ø> (ø)
src/services/control-panel-roles.ts 98.82% <ø> (ø)
src/services/miner-dashboard-recommendations.ts 100.00% <100.00%> (ø)
src/services/weekly-value-report.ts 100.00% <ø> (ø)
🚀 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 gittensor manual-review Gittensor contributor context labels Jul 4, 2026
@JSONbored
JSONbored merged commit b3407f8 into main Jul 4, 2026
11 checks passed
@JSONbored
JSONbored deleted the fix/orb-token-value-redaction branch July 4, 2026 19:39
JSONbored added a commit that referenced this pull request Jul 4, 2026
…ke scenario (#3228)

Completes the remaining #1825 deliverables that PR #3193's audit left
open: a written stable-ingress-vs-pull-mode recommendation, a
reproducible pull-mode smoke-test scenario alongside the existing
push-mode one, and troubleshooting guidance for a failed relay
registration in each mode.

- GitHub App and Orb doc: new "Choosing a relay mode" section makes
  the existing implicit lean (pull mode for NAT/tailnet, no public
  ingress needed; push mode requires a real, SSRF-validated public
  origin) explicit, and expands the Brokered Orb env block to show
  ORB_RELAY_MODE.
- Beta release checklist: the brokered-mode scenario now runs BOTH
  relay modes, since they exercise genuinely different code paths.
- Troubleshooting: new guidance for selfhost_orb_relay_register_failed,
  covering what it means and what to check per relay mode.
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.

chore(orb): verify brokered self-host relay and telemetry

1 participant