fix(security): trust forwarding IPs only from configured proxies - #587
fix(security): trust forwarding IPs only from configured proxies#587seonghobae wants to merge 121 commits into
Conversation
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthrough공개 앱의 rate-limit 구현을 공유 모듈로 통합했습니다. trusted proxy 식별과 로그 마스킹을 강화했습니다. 생산 OIDC, 공용 HTTPS 전송, 이메일 정규화 및 Stripe 웹훅 검증 경계를 추가했습니다. 회귀 테스트와 CI checkout 검증을 갱신했습니다. Changes보안 경계와 라우팅
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The PR strengthens proxy-derived rate limiting and account trust boundaries, but supported production OIDC logins can still fail because the token request body is incompatible with the outbound transport. Merge should wait for that compatibility fix; one related test also needs deterministic environment setup. 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 24.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 77 functions across 30 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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.
|
@opencode-agent review Please review exact current head Verify from current source before issuing a SHA-bound verdict:
If a current defect is found, identify the first failing boundary and the smallest realistic RED regression rather than relying on historical summary text. |
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.
|
@opencode-agent review Review exact current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/dependency-review.yml— GitHub Actions review job.github/workflows/server-tests.yml— GitHub Actions review jobCHANGELOG.md— repository behaviordocs/deploy.md— operator or user guidancedocs/doctoring/stripe-webhook-trust-boundary.md— operator or user guidancedocs/doctoring/trusted-proxy-client-ip.md— operator or user guidanceindex.html— repository behaviorpackage-lock.json— repository behaviorpackage.json— repository behaviorserver/app.mjs— TypeScript or JavaScript runtimeserver/app_routes.mjs— TypeScript or JavaScript runtimeserver/application_routes.mjs— TypeScript or JavaScript runtimeserver/application_routes_core.mjs— TypeScript or JavaScript runtimeserver/application_routes_implementation.mjs— TypeScript or JavaScript runtimeserver/auth.mjs— TypeScript or JavaScript runtimeserver/db.mjs— TypeScript or JavaScript runtimeserver/public_https_transport.mjs— TypeScript or JavaScript runtimeserver/rate_limit.mjs— TypeScript or JavaScript runtimeserver/signup_metrics.mjs— TypeScript or JavaScript runtimeserver/stripe_webhook.mjs— TypeScript or JavaScript runtimetests/api/email-identity.test.mjs— regression suitetests/api/email-unicode-identity.test.mjs— regression suitetests/api/invite-security.test.mjs— regression suitetests/api/oidc-core-production-failclosed.test.mjs— regression suitetests/api/oidc-production-boundary.test.mjs— regression suitetests/api/oidc-provider-metadata.test.mjs— regression suitetests/api/rate-limit-prometheus.test.mjs— regression suitetests/api/rate-limit-shared-boundary.test.mjs— regression suitetests/api/ratelimit.test.mjs— regression suitetests/api/security-guard-abuse-controls.test.mjs— regression suitetests/api/stripe-webhook.test.mjs— regression suitetests/unit/coverage-script-contract.test.mjs— regression suitetests/unit/dependency-review-merge-base-contract.test.mjs— regression suitetests/unit/node-runtime-contract.test.mjs— regression suitetests/unit/public-https-transport.test.mjs— regression suitetests/unit/rate-limit-observability-failure.test.mjs— regression suitetests/unit/rate-limit-scoped-ipv6.test.mjs— regression suitetests/unit/stripe-webhook-boundary.test.mjs— regression suitetests/unit/toast-accessibility.test.mjs— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: dependency-review.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: dependency-review.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: server-tests.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: server-tests.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["Repository file: CHANGELOG.md"]
S3 --> I3["repository behavior"]
I3 --> R3["Review risk: Repository file: CHANGELOG.md"]
R3 --> V3["required checks"]
Evidence --> S4["Docs: deploy.md (3 files)"]
S4 --> I4["operator or user guidance"]
I4 --> R4["Review risk: Docs: deploy.md (3 files)"]
R4 --> V4["docs review"]
Evidence --> S5["Repository file: index.html"]
S5 --> I5["repository behavior"]
I5 --> R5["Review risk: Repository file: index.html"]
R5 --> V5["required checks"]
Evidence --> S6["Repository file: package-lock.json"]
S6 --> I6["repository behavior"]
I6 --> R6["Review risk: Repository file: package-lock.json"]
R6 --> V6["required checks"]
Evidence --> S7["Repository file: package.json"]
S7 --> I7["repository behavior"]
I7 --> R7["Review risk: Repository file: package.json"]
R7 --> V7["required checks"]
Evidence --> S8["TypeScript/JavaScript: app.mjs (11 files)"]
S8 --> I8["TypeScript or JavaScript runtime"]
I8 --> R8["Review risk: TypeScript/JavaScript: app.mjs (11 files)"]
R8 --> V8["package test plus coverage"]
Evidence --> S9["Test: email-identity.test.mjs (11 files)"]
S9 --> I9["regression suite"]
I9 --> R9["Review risk: Test: email-identity.test.mjs (11 files)"]
R9 --> V9["targeted test run"]
Evidence --> S10["Test: coverage-script-contract.test.mjs (8 files)"]
S10 --> I10["regression suite"]
I10 --> R10["Review risk: Test: coverage-script-contract.test.mjs (8 files)"]
R10 --> V10["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
f36eb24a7ab47b880a1115a0d978a6011fa26048 - Workflow run: 33239412993
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: dependency-review.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: dependency-review.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Workflow: server-tests.yml"]
S2 --> I2["GitHub Actions review job"]
I2 --> R2["Review risk: Workflow: server-tests.yml"]
R2 --> V2["actionlint plus required checks"]
Evidence --> S3["Repository file: CHANGELOG.md"]
S3 --> I3["repository behavior"]
I3 --> R3["Review risk: Repository file: CHANGELOG.md"]
R3 --> V3["required checks"]
Evidence --> S4["Docs: deploy.md (3 files)"]
S4 --> I4["operator or user guidance"]
I4 --> R4["Review risk: Docs: deploy.md (3 files)"]
R4 --> V4["docs review"]
Evidence --> S5["Repository file: index.html"]
S5 --> I5["repository behavior"]
I5 --> R5["Review risk: Repository file: index.html"]
R5 --> V5["required checks"]
Evidence --> S6["Repository file: package-lock.json"]
S6 --> I6["repository behavior"]
I6 --> R6["Review risk: Repository file: package-lock.json"]
R6 --> V6["required checks"]
Evidence --> S7["Repository file: package.json"]
S7 --> I7["repository behavior"]
I7 --> R7["Review risk: Repository file: package.json"]
R7 --> V7["required checks"]
Evidence --> S8["TypeScript/JavaScript: app.mjs (11 files)"]
S8 --> I8["TypeScript or JavaScript runtime"]
I8 --> R8["Review risk: TypeScript/JavaScript: app.mjs (11 files)"]
R8 --> V8["package test plus coverage"]
Evidence --> S9["Test: email-identity.test.mjs (11 files)"]
S9 --> I9["regression suite"]
I9 --> R9["Review risk: Test: email-identity.test.mjs (11 files)"]
R9 --> V9["targeted test run"]
Evidence --> S10["Test: coverage-script-contract.test.mjs (8 files)"]
S10 --> I10["regression suite"]
I10 --> R10["Review risk: Test: coverage-script-contract.test.mjs (8 files)"]
R10 --> V10["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
seonghobae
added
bug
ChatGPT Codex Connector
1 participant
Problem
Protected
developstill contains multiple buyer-relevant trust-boundary weaknesses that this descendant repairs together: caller-controlled forwarding identity for the opt-in limiter, incomplete invitation/OIDC hardening, Unicode-inconsistent account identity, and an unauthenticated Stripe webhook path that can promote an organization from caller-supplied JSON. This PR removes those direct trust assumptions while preserving fail-closed operational evidence.Exact current identity
fix/trusted-rate-limit-client-ip660d27259c7027f6807f01a35caec87b4bd4ce5e6aee02140021b316d92195ce50977f47300ad150develop@2c328875e00e86537df3e965170be80532571cade6baf270a91fe9df60f2e772ddabaf2d503a05cbAny head/base/review/check movement invalidates revision-sensitive evidence until freshly revalidated. This body is traceability, not merge authority.
Security / availability contract
X-Forwarded-Foris ignored unless the immediate network peer is explicitly listed inSCOPEWEAVE_TRUSTED_PROXY_IPS; trusted hops are parsed right-to-left and canonicalized, including IPv4-mapped/scoped IPv6 handling, while malformed evidence fails closed.SCOPEWEAVE_RATE_LIMIT_BUCKETS_MAXbounds process-local client state; overflow identities share a fail-closed bucket. Numeric limiter configuration is safe-integer validated, withSCOPEWEAVE_RATE_LIMIT_MAX=0as the explicit opt-out.429requests remain represented in JSON and Prometheus request/4xx observability without exposing client identity.orgs.planfrom event payload. Durable provider-authoritative billing/reconciliation remains owned by [Commercial Gap] Build monotonic, auditable Stripe subscription lifecycle #488 and its active stack.docs/deploy.mdand the doctoring notes.Exact-current-head evidence
All evidence below is anchored to contributor head
660d27259c7027f6807f01a35caec87b4bd4ce5eunless noted otherwise.33232835264, unit/API job99048556600: checked out the exact contributor head under supported Node22.13.0; unit/API suites and the owned c8 production-coverage path completed successfully. The browser E2E lane for the same exact head also completed successfully.33232835232, job99048567150: explicitly verifiedexpected_sha == actual_sha == 660d272..., used the pinned scanner image, scanned 74 files with 257 rules, and reported zero findings. The historical synthetic-checkout defect previously recorded in this body does not reproduce on this exact head.GitHub Advanced Security currently also emits neutral PR-attribution checks for
osv-scanner(99048594787, “1 configuration not found”) and Trivy. These are not named branch-protection contexts or organization-required workflows. The repository-native OSV workflow itself succeeds, and the exactosvscanner.ymlblob is unchanged from protecteddevelop; the neutral GHAS attribution signal is therefore not being promoted to passing evidence or used as merge authority.Current non-passing required control-plane evidence
Active organization ruleset
18156473applies to the default branch and requires one approving review, resolution of review threads, and the centrally owned required workflowsclose-empty-pr.yml,opencode-review.yml,pr-review-merge-scheduler.yml,security-scan.yml,strix.yml,sast-semgrep.yml, andnoema-review.yml. It dismisses stale reviews on push.Two required central review lanes are currently non-passing for reasons outside the ScopeWeave source boundary:
99048617966, run33232834290, fails because there is no formalopencode-agentAPPROVEDorCHANGES_REQUESTEDreview whosecommit_idequals660d272.... The gate correctly rejects fallback/model-unavailable text as approval. The real central provider/review owner path has fresh exact-head reproduction and acceptance evidence inContextualWisdomLab/.github#624; ScopeWeave does not synthesize a verdict or churn this clean head.99048593265, run33232834363, fails closed after the central review sidecar (pinned tocontextual-orchestrator@b21645116b352967e50fc497b87eb745b9cc8c61) first receives a 413, retries with reduced context, then routes a text security-review request through/v1/orchestrator/freeto incompatible video-model candidates including Hunyuan Video and Veo variants and repeatedly receives HTTP 500internal_error. No authoritative Strix report is produced. The real causal owner path is now advanced on existingContextualWisdomLab/contextual-orchestrator#901with the exact run/SHA, capability-routing RED acceptance, smallest-remedy boundary, and unchanged downstream GREEN canary.These are review/control-plane failures, not evidence of an unaddressed ScopeWeave source finding. They remain non-passing until their owners repair the causal route and regenerate authoritative evidence on this unchanged head or its freshly revalidated successor.
Merge gate
Do not merge or enable auto-merge until the unchanged exact contributor head remains reconciled to protected
develop, all applicable branch-protection and organization-required workflows are substantively terminal-passing on the correct revision, every current valid finding is addressed, and ruleset18156473has a qualifying independent approval that remains valid for the current head. In particular, OpenCode and Strix must regenerate authoritative current-head evidence after their owner repairs; model-only/fallback/status-only evidence cannot substitute for the formal review contract.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 when applicable. A green CI result never excuses a semantic regression or missing required review/security authority.
Summary by CodeRabbit
보안 강화
개선 사항
문서