fix: gate GET /metrics behind METRICS_TOKEN, verify hardening review findings - #609
Merged
Markadrian6399 merged 2 commits intoSep 1, 2026
Merged
Conversation
/metrics was registered on the public router with no authentication, exposing webhook delivery volume, latency, and — most usefully to an attacker — auth outcome counters (stellargate_auth_attempts_total let a credential-stuffing attempt watch its own progress). The endpoint is now gated behind `Authorization: Bearer <METRICS_TOKEN>`, mirroring the existing ADMIN_PROVISIONING_SECRET pattern: unset (the default) disables it entirely, returning 401 for every request rather than falling back to an open default. deploy/Caddyfile also blocks /metrics and /v1/metrics at the edge by default. Documented in README.md, DEPLOYMENT.md, .env.example, deploy/stellargate.env.example, and openapi.yaml. A router-level test (api::tests::metrics_enabled_refuses_an_unauthenticated_scrape) asserts an unauthenticated scrape is refused when the gate is enabled, alongside coverage for the default-disabled and wrong-token cases. Also restores a buildable baseline: main did not compile on this branch's base commit. A merge (b8fd3bc, itself compounding an earlier one at 9263a8e) resolved a conflict by taking one side's lib.rs/config.rs wholesale instead of a proper 3-way merge, silently reverting several previously-shipped, tested fixes across many files — the same failure mode a prior commit on this repo (1340fd4) already hit and described once. Restored, each independently verified against the code and tests that already existed for it: - TaskHealth/AppState (src/lib.rs): the trustline/http/payment metrics fields and started()/stopped() counters other files already called. - Config (src/config.rs): STREAM_IDLE_TIMEOUT_SECS, WEBHOOK_RETRY_MAX_DELAY_MS, and the worst-case-inline-delivery boot check (issues StellarGateLabs#312, StellarGateLabs#318, StellarGateLabs#238) that other restored code below depends on. Config knobs whose consuming code was equally reverted and is out of scope here (pagination limits, payment-amount bounds, SQLite pragma tuning, DB-pool/rate-limiter sizing, webhook redrive jitter) were left out — the orphaned tests that referenced them were trimmed rather than the feature resurrected; worth a dedicated follow-up. - Baseline security headers on every API response, not only the dashboard (issues StellarGateLabs#251-StellarGateLabs#254). - The Horizon SSE stream listener's idle timeout and reconnect metric (issue StellarGateLabs#312); the poller's Retry-After/backoff handling (issue StellarGateLabs#313); first-run cursor baselining with backward overlap (issue StellarGateLabs#311); the periodic trustline checker background task. - Inline webhook retry backoff with jitter (issue StellarGateLabs#318). - Audit logging on POST /merchants and POST /payments (issue StellarGateLabs#305). - Strict CORS: DELETE, Idempotency-Key, X-Admin-Secret, and exposed response headers (issue StellarGateLabs#281). - db::migrate wrapped in a transaction so a failed migration rolls back instead of leaving the schema half-migrated; a nullable merchants.api_key_hash; the payments(status, expires_at) partial index (issue StellarGateLabs#270); percent-encoded Horizon paging-cursor URLs. - /metrics's DB file-size gauges, previously always reporting absent. - openapi.yaml: /v1/payments was mis-keyed as /payments (colliding with the real deprecated alias), and a 3.0-only `nullable:` keyword remained under the declared 3.1.0. A handful of tests that assumed a richer TaskExit-returning supervisor was wired into main.rs's worker spawning (it isn't — main.rs still uses a simpler spawn/join helper) were adjusted to assert on the behavior that helper actually provides, rather than pulling that larger rewiring into this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tellarGateLabs#461, StellarGateLabs#462 Each of these "review" issues asked to confirm a security property already claimed by existing code/comments, not to change behavior. All three checked out; this adds the regression coverage that was missing so a future change can't silently break what the review confirmed. - StellarGateLabs#460 (GET /dashboard, /dashboard/app.css, /dashboard/app.js): confirmed safe to serve unauthenticated — the three handlers take no State/DB parameter and return include_str!-embedded static assets, so the shell cannot leak per-request or merchant data by construction. Already covered by test_dashboard_assets_served_unauthenticated and test_dashboard_data_endpoints_reject_missing_key; no new test needed. - StellarGateLabs#461 (POST /merchants / provision_merchant): confirmed it can't be used to mass-create merchant records. No request body to validate, and the route already sits in the base-rate "merchants" bucket (1x, not the 5x read bucket) on top of requiring ADMIN_PROVISIONING_SECRET. Added test_provision_merchant_rate_limit_exceeded_returns_429 — every other write route had this test, this one didn't. - StellarGateLabs#462 (POST/GET /merchants/:id/keys): confirmed a raw key is returned exactly once, at issuance — list_api_keys projects only key_id/prefix/label/timestamps/active, never the key material — and that only a SHA-256 digest is ever persisted. Added api_keys_are_stored_hashed_not_plaintext, asserting the stored api_keys.key_hash and legacy merchants.api_key_hash columns differ from the raw key and equal its digest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@Emeka-12 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /metricswas registered on the public router with no authentication, noallow-list, and no separate listener — any anonymous caller could scrape
webhook delivery volume, failure rate, latency histograms, and (most usefully
to an attacker) the auth-outcome counters, letting a credential-stuffing
attempt watch its own progress in real time.
/metricsnow requiresAuthorization: Bearer <METRICS_TOKEN>, followingthe same pattern as
ADMIN_PROVISIONING_SECRET: unset by default, whichdisables the endpoint entirely (
401for every request) rather thanfalling back to an open default.
deploy/Caddyfileblocks/metricsand/v1/metricsat the edge bydefault, as defense in depth on top of the app-side gate.
README.md,DEPLOYMENT.md,.env.example, anddeploy/stellargate.env.example.metrics_enabled_refuses_an_unauthenticated_scrape)asserts an unauthenticated scrape is refused when the gate is enabled — the
literal acceptance criterion from the issue — alongside coverage for the
default-disabled case and a wrong-token attempt.
Closes #250.
Also: verified the three related hardening-review issues
While addressing #250, I reviewed the other three open "API hardening
review" issues, since they cover adjacent unauthenticated/admin-gated
surface. Each asked to confirm a security property, not implement new
behavior — all three checked out against the current code, and each now has
a regression test so a future change can't silently break what the review
confirmed:
API hardening review: GET /dashboard, /dashboard/app.css, /dashboard/app.js #460 —
GET /dashboard,/dashboard/app.css,/dashboard/app.js:confirmed safe to serve unauthenticated. The three handlers take no
State/DB parameter — they returninclude_str!-embedded static assetsbaked in at compile time — so the shell cannot leak per-request or
merchant data by construction. Every figure the dashboard displays is
fetched client-side from the same authenticated endpoints a merchant would
call directly, using an API key the operator supplies in the browser.
Existing coverage (
test_dashboard_assets_served_unauthenticated,test_dashboard_data_endpoints_reject_missing_key) was already sufficient.API hardening review: POST /merchants (provision_merchant) #461 —
POST /merchants(provision_merchant): confirmed it can't beabused to mass-create merchant records. The handler takes no request body
to validate, and the route already sits in the base-rate
"merchants"bucket (1×, not the 5× read bucket) on top of requiring
ADMIN_PROVISIONING_SECRET. Addedtest_provision_merchant_rate_limit_exceeded_returns_429— every otherwrite route already had this test; this one didn't.
API hardening review: POST /merchants/:id/keys and GET /merchants/:id/keys (issue_api_key / list_api_keys) #462 —
POST/GET /merchants/:id/keys: confirmed a raw key isreturned exactly once, at issuance, and never again —
list_api_keysprojects only
key_id/prefix/label/timestamps/active, never keymaterial — and that only a SHA-256 digest is ever persisted
(
hash_api_keyindb.rs). Addedapi_keys_are_stored_hashed_not_plaintext,asserting the stored
api_keys.key_hashand legacymerchants.api_key_hashcolumns differ from the raw key and equal its digest.
Closes #460. Closes #461. Closes #462.
Unrelated: restored a batch of previously-shipped fixes
Landing #250 required building on
main, which did not compile at thisbranch's base commit — a merge (
b8fd3bc, compounding an earlier one at9263a8e) had resolved a conflict by taking one side'slib.rs/config.rswholesale instead of a proper 3-way merge, silently reverting several
previously-shipped, already-tested fixes across many files (the same failure
mode a prior commit on this repo,
1340fd4, already hit and described once).Restored, each independently verified against the code and tests that
already existed for it — full list in
CHANGELOG.mdunder[Unreleased].This is orthogonal to the
/metricsfix but was a prerequisite for thisbranch to build and test at all; happy to split it into its own PR if
preferred.
Test plan
cargo test— 259/260 passing. The one remaining failure(
ssrf::tests::validate_rejects_url_with_no_host) is pre-existing,unrelated to this change, and independently reproducible on
mainbefore this branch: the test assumes an
http(s)URL can parse withan empty host, which the
urlcrate's spec-compliant parser makesunreachable for any URL that parses successfully as
http/httpsinthe first place — a pre-existing test-design defect, not a runtime bug.
metrics_unset_token_refuses_every_request,metrics_enabled_refuses_an_unauthenticated_scrape,metrics_enabled_refuses_the_wrong_token,metrics_enabled_accepts_the_correct_token,test_provision_merchant_rate_limit_exceeded_returns_429,api_keys_are_stored_hashed_not_plaintext, plus theConfig::validate_metrics_tokenunit tests (empty/short/placeholder/valid).deploy/Caddyfilechange reviewed manually (blocks/metricsand/v1/metricsbefore the reverse proxy).Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com