fix(security): address critical/high/medium audit findings on the write & auth paths#305
Open
ety001 wants to merge 4 commits into
Open
fix(security): address critical/high/medium audit findings on the write & auth paths#305ety001 wants to merge 4 commits into
ety001 wants to merge 4 commits into
Conversation
… honest verifySignature Three critical audit findings on the write/broadcast path: 1. recover-account: return 503 when MySQL is unavailable (was: skip the recovery-record cross-check and broadcast anyway). This is the only application-layer gate on the account-takeover path; fail-closed to match recovery/request and recovery/confirm. 2. Enforce the operation type on every broadcast route. Previously only account-update validated operations[0][0]; the other 15 routes relayed any signed operation verbatim, so a transfer could be posted to /vote to use its higher rate-limit budget. New assertSignedTxOpType() helper applied to all routes. 3. Rename the misleading verifySignature() to validateTransactionShape() with a security note: it checks transaction shape only, not signatures — the chain rejects bad signatures on broadcast. Real crypto verification is tracked separately (needs serializeTransaction exported upstream). Adds tests for the DB-unavailable→503 gate and the op-type helper.
…e rate limit, headers 1. CSRF: replace the forgeable base64(secret:ts:rand) token with an HMAC-signed token (base64url(ts).base64url(HMAC(ts))), constant-time comparison, and no insecure default secret. Production rejects all mutations if CSRF_SECRET is unset; docker-compose now requires it. 2. Login: reject (503) when Redis is unavailable instead of authenticating on a public-key match alone — the challenge signature check needs Redis. 3. Rate limiting: proxy-aware client IP via TRUST_PROXY_COUNT (no longer trusts spoofable X-Forwarded-For blindly), per-route scoped keys (the /vote budget no longer shares /transfer's), optional fail-closed when RATE_LIMIT_ALLOW_MEMORY_FALLBACK=false, and fix the hardcoded config_maxRequests in getRateLimitInfo. 4. Add security response headers: X-Frame-Options DENY, nosniff, Referrer-Policy, Permissions-Policy, HSTS, and CSP frame-ancestors 'none'.
… hardening - custom_json: allowlist op-ids (community/follow/reblog), validate id format and payload length; stop being an open arbitrary-payload relay. - analytics: require CSRF (client now mirrors the token); prevent log injection. - accounts query: hash the full names param for the cache key (was: truncated to 200 chars, causing distinct lists to collide / cross-user cache poisoning). - recovery/confirm: preflight CONVEYOR_POSTING_WIF config (high-value secret) — return clean 503 when missing/misconfigured instead of 500. - recovery/verify: tighten rate limit 20→10/300s (account_name is an enumeration surface, mitigated by 80-bit admin codes). - help-markdown: parse inline HTML then sanitize via rehype-sanitize (was: rehype-raw with no sanitization → stored XSS if a source file is tampered). - browser-storage: document the localStorage posting-key risk and mitigations. - redis: gate reconnection with a cooldown to avoid a retry storm on flapping. - cache-invalidate header helper: validate Steem-name format before reflecting username into the header (header-injection hardening). - client.ts/server/community: replace Math.random() with crypto for challenge and community-name entropy. - healthcheck: stop leaking docker_tag/source_commit to anonymous callers. - drizzle.config: require DATABASE_URL, drop hardcoded root:12345678 fallback. - strip internal error details from broadcast 500 responses (still logged server-side).
…mment order Self-review of PR #305: - community.ts generateCommunityOwnerName: when crypto.getRandomValues is unavailable it silently produced a deterministic 'hive-100000' (collision risk). Now throws like cryptoRandomHex does, instead of falling back. - vote/transfer routes: move the 'Broadcast the transaction' comment below the op-type guard so the comment matches the code that follows it.
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
1 participant
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
Fixes the security audit findings on the
nextbranch (3 Critical, 4 High, multiple Medium/Low). All changes verified:type-checkclean,lintclean, 424 tests pass, production build succeeds.Split into three commits by severity for review.
P0 — broadcast integrity (Critical)
recover-account: return 503 when MySQL is unavailable. Previously the recovery-record cross-check (status === 'closed', key match) was wrapped inif (db)and skipped on DB-down — broadcasting an account-takeover op with no application-layer gate. Now fail-closed, matchingrecovery/requestandrecovery/confirm.account-updatevalidatedoperations[0][0]; the other 15 routes relayed any signed op verbatim (e.g. atransfercould be posted to/voteto use its higher rate-limit budget). NewassertSignedTxOpType()applied to all routes.verifySignature()→validateTransactionShape()with an honest security note: it validates shape only, not signatures (the chain rejects bad sigs on broadcast). Real crypto verification is tracked separately — it needsserializeTransactionexported upstream (@steemit/steem-js); the library's built-inverifyTransactionis broken (verifies againstJSON.stringifyinstead of the binary digest).P1 — hardening (High)
base64(secret:ts:rand)token with HMAC-signed token (base64url(ts).base64url(HMAC(ts))), constant-time comparison, no insecure default secret. Production rejects all mutations ifCSRF_SECRETis unset.TRUST_PROXY_COUNT(no longer trusts spoofableX-Forwarded-Forblindly), per-route scoped keys, optional fail-closed (RATE_LIMIT_ALLOW_MEMORY_FALLBACK=false), fixed the hardcodedconfig_maxRequests.X-Frame-Options: DENY,nosniff,Referrer-Policy,Permissions-Policy, HSTS,frame-ancestors 'none'.P2 — info leakage, entropy, XSS, misc (Medium/Low)
custom_json: allowlist op-ids (community/follow/reblog) + payload validation.accountsquery: hash full param for cache key (fixes prefix-truncation collision).recovery/confirm: preflightCONVEYOR_POSTING_WIFconfig → clean 503.recovery/verify: tighten rate limit 20→10/300s.help-markdown: parse inline HTML then sanitize viarehype-sanitize.redis: reconnection cooldown to avoid retry storms.X-Cache-Invalidate: validate Steem-name format before reflecting into header.Math.random()withcryptofor challenge/community entropy.docker_tag/source_commit.drizzle.config: requireDATABASE_URL, drop hardcodedroot:12345678.Test plan
pnpm type-check— cleanpnpm lint— 0 errorspnpm test— 424 passed (includes new tests for the DB gate + op-type helper)pnpm build— succeedsNotes for reviewers
CSRF_SECRET(e.g.openssl rand -hex 32) andTRUST_PROXY_COUNT(proxy hop count). WithoutCSRF_SECRET, all mutations are rejected in production by design.verifyTransactionupstream fix) is documented in a separate plan; this PR's op-type + CSRF + chain-rejection defense-in-depth removes the exploitable surface meanwhile.