P0: Add security & deployment hardening guide (#168) - #186
Conversation
|
Thanks for writing this — a single opinionated "how to expose this safely" doc is exactly what the epic needs, and the read-only One correctness issue in the Built-in protections table is worth fixing before this lands, because it states the opposite of today's behavior and could break the mina-explorer. CORS default isn't "same-origin only". On That feeds a backwards-compat risk with the mina-explorer: it's a browser app served from its own origin that POSTs
would, if followed, block every browser Explorer/dashboard instance. Could we add a note, e.g.:
Two smaller things:
Thanks again — happy to help refine the CORS wording. |
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — fixed in CORS. You were right that the checklist was the dangerous part: On the "reads as current" problem — rather than a caveat naming in-flight PRs (which we'd have to remember to delete), I pinned it to a version: the table now says these controls arrive in 1.0.0, and that Introspection. Left the "keep it off" advice as-is: #193 lands in wave 1, well ahead of this, so Two things you didn't flag, found while in here:
|
|
Verdict: MERGEABLE ✅ Docs-only ( What I checked
Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3e12fb8 to
8df20e5
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
The guide is a genuine improvement and almost everything from round 1 is fixed. One factual error blocks it, because it is exactly the kind of error this document exists to prevent.
Blocker — docs/security.md:57 and :75 state the opposite of what #185 ships
docs/security.md:57 says:
TRUST_PROXYdefaults to0, which ignores the header entirely and keys on the socket address; that is correct for a directly-exposed server…
#185 (head cf9eb0fc) gives TRUST_PROXY no default. I checked src/server/rate-limit.ts directly:
trustProxyConfigured: hops !== undefined,
...
if (!config.trustProxyConfigured) {
warn('[rate-limit] TRUST_PROXY is not set — rate limiting is DISABLED. No default is safe…');
return {}; // <- empty plugin
}hopsFromEnv() returns undefined for missing or malformed input, and useRateLimit() then returns a no-op plugin. Unset is not 0. Unset means no rate limiting at all.
This compounds with the protections table at :75, which lists per-IP rate limiting as Default on. So an operator running a directly-exposed server reads :57 ("the default 0 is correct for me"), reads :75 ("rate limiting is on"), leaves TRUST_PROXY unset, and ships with the abuse control this guide promises silently absent. The checklist item at :139 only asks them to set TRUST_PROXY if a gateway is present, so they will skip it too.
This is the one document in the repo whose job is to tell an operator which protections are active. Getting that backwards is worth one more round.
Fix 1 — replace docs/security.md:57-59
`TRUST_PROXY` has **no default and no safe guess**: with `RATE_LIMIT_MAX > 0`
and `TRUST_PROXY` unset, the server logs a warning and **rate limiting stays
disabled**, because socket-keying behind a load balancer collapses every
client into one bucket while trusting `X-Forwarded-For` blindly lets any
caller mint a fresh bucket per request. Set `TRUST_PROXY=0` for a directly
exposed server, or to the real hop count behind a gateway.Fix 2 — replace the table row at :75
| Per-IP **rate limiting** | on once `TRUST_PROXY` is set | Bounds request volume per client. Disabled (with a startup warning) while `TRUST_PROXY` is unset |Fix 3 — replace the checklist item at :139, so a no-gateway deployment is covered too
- [ ] `TRUST_PROXY` explicitly set — `0` for a directly exposed server, or the
real hop count behind a gateway. Rate limiting is **off** until it is setEverything else verified good
- The
ENABLE_INTROSPECTIONcaveat (:119-124,:143-145) is accurate and correctly scoped to0.0.x— confirmed againstsrc/server/plugins.ts:30on main (if (!process.env.ENABLE_INTROSPECTION), so the string"false"enables introspection). Correctly conditioned on #193 fixing it. - Hop counts (
:97-99) now match #185'sclientId()implementation (forwarded[forwarded.length - trustProxy]), #185's own config docs, and #197's runbook. GCP ALB = 2 in all three. This was round 1's blocker and it is resolved. - Every default named in the protections table matches the sibling PRs: CORS same-origin (#184), query-cost limits on with depth 12 (#183), statement timeout and pool limits on (#182 → 15000/10), introspection off, field-suggestion blocking on.
- No real credentials anywhere;
PASSWORD 'change-me'at:152is an obvious placeholder. - Docs-only, so no downstream impact on mina-explorer or mina-explorer-api. The
CORS_ORIGINguidance at:131-143explicitly names mina-explorer as a reason to choose*, which is the right advice — that consumer calls this API cross-origin from the browser and #184's secure default would otherwise block it outright.
One note for whoever picks this up
#196 deploy/README.md:46 and #197 docs/runbook.md:95 carry the same "TRUST_PROXY=0 is the default" mental model. Neither is blocking on its own (#196's manifests set the variable explicitly; #197's is a wrong incident diagnosis rather than a wrong default), and I have left the exact replacement text on both. Correcting all three together is the efficient move, since it is one underlying misconception.
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8df20e5 to
c70376a
Compare
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c70376a to
3268d5c
Compare
The repo had no single place describing how to expose the API safely. Document the intended posture: a public, read-only GraphQL service meant to run behind a TLS-terminating gateway against a read-only Postgres role, with no application-level auth (gating, if needed, is an operator concern at the gateway). Adds docs/security.md covering the security model, network architecture (TLS gateway, X-Forwarded-For, private Postgres), the built-in abuse protections, a least-privilege read-only DB role (with SQL), operational practices, and a deployment checklist. Linked from the README and the setup guide. Closes #168. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3268d5c to
fc33f6b
Compare
What & why
Part of the production-readiness epic (#163). Closes #168.
The repo had no single place describing how to expose the API safely. This adds
docs/security.mddocumenting the intended posture, chosen for this service: a public, read-only GraphQL API meant to run behind a TLS-terminating gateway against a read-only Postgres role, with no application-level auth (per-caller gating, if needed, is an operator concern at the gateway — the app stays simple).Contents
X-Forwarded-Forfor per-client rate limiting, Postgres kept private (with a diagram).archive_api_roread-only role (SQL included).Linked from the README (new "Security & production deployment" section) and the setup guide's "Where to go next".
Note on sequencing
The "Built-in protections" table describes controls delivered by the sibling P0 PRs (#164 query-cost, #165 PG timeouts, #166 rate limiting, #167 CORS). This doc is best merged after / alongside those so every protection it references is present on
main. Cross-doc links only targetgetting-started.md#configuration, which already exists onmain, so the doc has no hard dependency on merge order.Testing
Docs-only.
npx prettier --debug-check .exits 0;npm run lintclean. Internal links verified against existing anchors.🤖 Generated with Claude Code