fix(security): trust forwarding IPs only from configured proxies - #587
fix(security): trust forwarding IPs only from configured proxies#587seonghobae wants to merge 83 commits into
Conversation
📝 WalkthroughWalkthrough공개 앱을 보안 엔벨로프로 분리하고 rate-limit과 trusted proxy IP 처리를 강화했습니다. OIDC와 초대 경계를 추가했습니다. Stripe 웹훅은 원시 본문 HMAC 검증 후 확인 응답만 반환합니다. API, 단위 테스트와 CI 커버리지 계약도 갱신했습니다. Changes보안 경계와 라우팅
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The change hardens forwarded-IP trust and limiter controls, but a directly mounted application boundary is still described as using the older, weaker limiter policy and performing invite or authentication work before enforcement. That leaves a concrete security and availability exposure, and the required independent approval and exact-head merge evidence are absent, so the PR is not ready to merge. Sequence Diagram(s)sequenceDiagram
participant Stripe
participant PublicApp as server/app.mjs
participant Verifier as verifyStripeWebhookRequest
Stripe->>PublicApp: 원시 웹훅 요청
PublicApp->>Verifier: 본문과 stripe-signature 전달
Verifier-->>PublicApp: 검증된 이벤트 또는 오류
PublicApp-->>Stripe: received 응답 또는 HTTP 오류
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 32.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 13 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keep the public copy-and-replace composition and make the protected-graph Stripe route fail closed on HMAC verification so a direct mount cannot upgrade orgs.plan from unsigned JSON. Record the trust boundary in CHANGELOG and doctoring.
Replace the source-text serve-path assertion with a runtime Hono route-graph regression that fails while the public facade registers /toast-state.css twice. Remove the redundant facade route so the protected route graph remains the single SaaS implementation.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Problem
ScopeWeave's opt-in fixed-window limiter historically trusted caller-controlled forwarding data without anchoring it to an explicitly trusted transport peer. This lane hardens that boundary and adjacent authentication/invite/webhook trust boundaries while preserving operator-visible abuse-control evidence.
Exact current identity
fad52de37a9f749738118d27ca31e47b6fc547ad;e63d1ad98427867519f9ac298fd163a1946d5959;fix/trusted-rate-limit-client-ip;develop@2c328875e00e86537df3e965170be80532571cad;614f35b9dc083d165f0a842705d8bf69ce4a6bef;Any head/base/review/check movement invalidates revision-sensitive evidence until freshly revalidated. This body records traceability and is not merge authority.
Security / availability contract
X-Forwarded-Foris ignored unless the immediate network peer is explicitly listed inSCOPEWEAVE_TRUSTED_PROXY_IPS.SCOPEWEAVE_RATE_LIMIT_BUCKETS_MAXbounds regular in-memory client buckets; excess identities share a fail-closed overflow bucket.SCOPEWEAVE_RATE_LIMIT_MAX=0is the documented opt-out; malformed, negative, or infinite values fail startup rather than silently weakening protection.429requests remain represented in request/4xx observability without exposing client identity; current regressions cover both JSON and Prometheus behavior.docs/deploy.md.Current repair evidence
The current descendant integrates the prior Stripe webhook, invite, and OIDC trust-boundary work that was previously isolated in #589. Current-head review findings that demonstrated real defects were repaired test-first, including trusted-proxy identity, bounded limiter state, fail-closed numeric configuration, blocked-request/Prometheus observability, direct-consumer limiter ordering, overflow-test determinism, shared limiter deduplication, bounded OIDC state, slashed stacked-base resolution, and the coverage producer-contract guards restored by
fad52de3.A fresh review-thread sweep on this exact head returns zero unresolved threads. Remaining historical/current informational observations are resolved only after verification against the current source; they are not being counted as approvals. Formal review submissions are COMMENTED/model or author submissions only; no qualifying independent current-head APPROVED review exists.
Exact-head workflow truth
For exact contributor head
fad52de37a9f749738118d27ca31e47b6fc547ad, the associated repository/organization pull-request workflow runs are terminal GitHub-success at the run level:32854298556;32854298576;32854298537;32854298570;32854298579; and32854299261.Repository Server Tests executed the current unit/API/coverage contract successfully; the current
test:coveragepath and its regression guards require the real coverage producer/reporter chain rather than recursive or report-only evidence.The required central
opencode-reviewcheck97860655574is terminal GitHub-success, but check success is not a qualifying formal APPROVED review. The centralcoverage-evidencecheck97848057343is also GitHub-success, but its executed log explicitly states that it preserves the stable branch-protection context without executing pull-request content. It is therefore a placeholder status rather than substantive current-head production-coverage evidence and cannot authorize integration by itself.Known central exact-head evidence defect
Fresh logs for this exact PR reproduce the centrally owned checkout defect already tracked by
ContextualWisdomLab/.github#1222:32854298570, job97822399158, checked out synthetic PR merge revision614f35b9dc083d165f0a842705d8bf69ce4a6befrather than immutable contributor headfad52de37a9f749738118d27ca31e47b6fc547ad.32854298537, Trivy job97822400530, likewise checked out and scanned synthetic revision614f35b9dc083d165f0a842705d8bf69ce4a6befrather than the contributor head.Those scans may still be useful merge-tree diagnostics, but under the repository's exact-head commercial evidence contract they are not immutable-head SAST/Security authority. The central
.githubdedicated owner path is already actively repairing this causal boundary; ScopeWeave does not create a competing source writer or a leaf workaround. Corrected central workflows must regenerate substantive evidence on this unchanged exact head or its freshly revalidated successor before merge classification.Merge gate
Do not merge or enable auto-merge until the unchanged exact contributor head remains reconciled to protected
develop, the centrally owned SAST/Security checkout repair is protected-shipped and regenerated against the exact contributor head where head-oriented semantics apply, every applicable CI/browser/owned statement-branch-function-line coverage/docstring/CodeQL/SAST/security/dependency/supply-chain/package/SBOM/provenance gate is substantively terminal-passing, every valid current finding remains resolved, and the live required approving-review rule is satisfied by a qualifying independent current-head approval (currently one approval under active ruleset18156473; no latest-push approval is required by that ruleset).Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, placeholder-only, status-only, author-only, model-only, rate-limited, or infrastructure-only evidence is non-passing.