fix(security): close protected billing and identity trust gaps - #589
fix(security): close protected billing and identity trust gaps#589seonghobae wants to merge 32 commits into
Conversation
📝 WalkthroughWalkthroughStripe 웹훅을 원시 본문 HMAC-SHA-256 검증 기반의 fail-closed 흐름으로 교체했습니다. SaaS 라우트를 공통 코어로 분리하고, 검증된 웹훅도 내구성 있는 조정 전에는 entitlement를 변경하지 않도록 했습니다. Changes웹훅 보안 및 애플리케이션 라우팅
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The PR closes the targeted billing, invite, and production OIDC trust gaps, but it is not yet merge-ready because rejected requests are recorded as OPTIONS instead of their original method, making security audit data inaccurate, while invite-token removal and mock-identity protection still depend on the shared route boundary rather than being enforced in the core graph. These bounded issues should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant Stripe
participant PublicApp
participant WebhookVerifier
participant OrganizationState
Stripe->>PublicApp: POST /api/stripe/webhook with raw body and signature
PublicApp->>WebhookVerifier: verifyStripeWebhookRequest(request, secret)
WebhookVerifier-->>PublicApp: validated event or validation error
PublicApp-->>Stripe: 200 { received: true } or 400/413/503 error
PublicApp->>OrganizationState: no direct entitlement update
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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.
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.
|
Strix job 97204514255 (run 32643656525) on exact head That is not a Stripe HMAC finding and does not restore unsigned |
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.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add a realistic API regression for the protected-develop invite bearer-token leak and cross-identity redemption path. The test requires roster responses to omit tokens, rejects a mismatched authenticated redeemer without membership side effects, preserves case-insensitive intended redemption, and retains one-time use.
Preserve the protected route/middleware order while wrapping the existing invite endpoints at their original route positions. Pending roster state no longer exposes bearer tokens, and the final invite handler rejects an authenticated identity whose canonical email does not match the pending invitation before any membership or acceptance mutation.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add a production-mode API regression proving that an absent OIDC issuer cannot silently activate the built-in mock identity provider. The start and mock-authorize paths must return the same stable not-configured response without redirecting or minting callback authority.
seonghobae
changed the title
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.
| async function bindInviteToAuthenticatedIdentity(c, next) { | ||
| // This guard only narrows access after confirming that the presented | ||
| // credential is still live. The core requireAuth middleware remains the | ||
| // authoritative authentication/RBAC boundary and repeats that validation | ||
| // before any invite mutation. | ||
| const identity = authenticatedIdentityHint(c); | ||
| if (!identity) return next(); | ||
|
|
||
| const invite = db.prepare('SELECT email, accepted_at FROM invites WHERE token = ?') | ||
| .get(c.req.param('token')); | ||
| if (!invite || invite.accepted_at) return next(); | ||
|
|
||
| const invitedEmail = normalizeIdentityEmail(invite.email); | ||
| if (!invitedEmail || !identity.email || invitedEmail !== identity.email) { | ||
| return guardRejectionThroughCoreAbuseControls(c, { error: 'invalid or used invite' }); | ||
| } | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
📝 Info: Invite guard queries the DB before rate limiting
bindInviteToAuthenticatedIdentity runs an identity lookup and an invite lookup before any rate-limit accounting, which happens only afterward via the OPTIONS probe or core middleware. An authenticated caller can drive DB work on the invite-accept path ahead of the limiter. Rate-limit policy is stated to be owned by a separate PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Owner-path repair is now active rather than merely deferred. I freshly reconciled exact #589 head 03cbe9fe227dc3b8fdddb5013ea9ab804d5946cc into the trusted-rate-limit owner lane #587, whose current head is e8ef4fdb00c9dfe55ee28c7e68a8d0cd799a12bf with #589 as a direct parent. That integration preserves #589's application_routes.mjs identity/invite guards but wraps the public route graph in #587's transport-peer-aware outer limiter before route-specific guard/database work; app_routes.mjs is now only a compatibility shim into the guarded shared boundary while the historical core limiter initializes disabled for the public composition. This is the causal owning-boundary repair for the pre-limiter invite DB lookups. Exact integrated #587 CI was freshly dispatched and is still queued/non-passing, so I am deliberately leaving this thread unresolved until the integrated head proves focused/full GREEN and is revalidated against its live base.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fresh supersession proof: exact #589 head #589 is therefore technically superseded by #587 rather than an independent merge candidate. Closing this PR loses no unique commits or repaired behavior; #587 remains the integration root. This does not transfer or manufacture merge authority: #587 still requires its own unchanged exact-head gates, current review state, live-base reconciliation, and qualifying independent approval before any merge. |
1 participant
Security hotfix
Protected
develop@2c328875e00e86537df3e965170be80532571cadstill carries three fail-open trust gaps that this branch closes without treating downstream callback data as entitlement authority:checkout.session.completedJSON could upgradeorgs.plantopro;OIDC_ISSUERwas absent, the built-in mock IdP could be reached outside explicit development mode and mint a session for a caller-selected email.Exact current identity
develop@2c328875e00e86537df3e965170be80532571cad;03cbe9fe227dc3b8fdddb5013ea9ab804d5946cc;fix/stripe-webhook-trust-develop-488;2c328875...;Any contributor-head or protected-base movement invalidates revision-sensitive evidence until the new revisions are freshly reconciled.
Current-head behavior
POST /api/stripe/webhookverifies Stripet+v1HMAC-SHA-256 over a bounded exact raw body before JSON parsing, rejects unsigned/stale/mutated deliveries, and acknowledges authentic deliveries without treating callback JSON as authority to mutate paid entitlements.ttext from the header while separately validating its numeric timestamp for freshness, matching the provider-signed payload bytes.server/application_routes.mjsis the supported shared security boundary. It installs production OIDC fail-closed, invite identity-binding, and pending-token-redaction guards before mounting the internal implementation graph.SCOPEWEAVE_DEV=1. Missing production OIDC configuration therefore fails closed even if a future internal consumer bypasses the outer shared boundary.token_versionstill matches, so revoked sessions defer to the authoritative core authentication boundary and cannot become an invite-email oracle.OPTIONSaccounting probe. The probe forwards the currentx-forwarded-forinput but no body or authorization; a process-local environment symbol carries the attempted HTTP method so structured logs retain the original method while the probe remains non-mutating. A core429becomes the public response; otherwise the intended generic404is returned.Test-first repair evidence
Revoked-session invite oracle
36c10372a8c8aab5437bc99e24e4ea22bb659201added a stale-session invite regression.bc50cc99577d95876f834c651bc545daf5fbe267checks the live persisted JWT token version before an email hint may narrow invite access.Guard-rejection abuse-control bypass
ab7c7fe1ed3778c5bc2af3e3e6bb8c27a66c8bcdregisteredtests/api/security-guard-abuse-controls.test.mjs; hosted Server Tests run32656577151, job97236135012, failed at the new assertion because repeated unconfigured-OIDC rejection remained404instead of reaching the limiter's429.72f6acce5c1ff233a97ea345a27d59364ff7515froutes rejected-guard accounting through the existing core abuse-control/observability middleware without executing the unsafe historical handler.Internal trust-boundary hardening
Current exact head
03cbe9fe227dc3b8fdddb5013ea9ab804d5946ccisfix(security): harden internal invite and OIDC boundaries. It removestokenfrom the core pending-invite roster query, repeats invite-email binding in core, corrects the core Stripe composition comment, restrictsoidcMockto explicit development mode, and makes the core OIDC start path return404 {"error":"sso not configured"}when production configuration is absent. These changes make the outer guards defense-in-depth rather than the sole protection against those failure modes.Current workflow state
All six repository workflow runs associated with exact contributor head
03cbe9fe227dc3b8fdddb5013ea9ab804d5946ccare terminal GitHub-success:32667073472— success;32667073515— success;32667073476— success;32667073761— success;32667073482— success; and32667073487— success.These labels are not immutable contributor-head merge authority under the known checkout defect. The current Server Tests run checked out synthetic PR merge SHA
59475c5466f4dd447e34ea9d3df3566e5fa7099e, not contributor SHA03cbe9f.... ScopeWeave #523 owns repository-native exact-head Server Tests/coverage integrity;ContextualWisdomLab/.github#1222owns reusable required SAST/Security exact-head checkout integrity. Predecessor, synthetic-only, status-only, model-only, pending, skipped, absent, or stale evidence is never promoted to passing.Review state
No formal APPROVED review exists on the current contributor head. Current Devin/CodeRabbit formal reviews are COMMENTED observations, not qualifying independent approval.
Verified defects around webhook middleware bypass, invite token exposure/identity binding, OIDC fail-open behavior, broken coverage-contract assertions, guard rejection method logging, stale composition comments, revoked-session narrowing, and literal Stripe timestamp handling are repaired in current source. Remaining unresolved inline observations are informational or belong to an already-separated owner lane: bounded raw-body handling; minor members-response rebuild overhead; the intentional invited-email equality constraint; declared-versus-streamed length mismatch with HMAC still covering the actual bounded bytes; invite guard DB work occurring before the current core limiter; dependence on core delegation for abuse accounting; and confirmation that literal timestamp HMAC handling is correct. They remain visible rather than being mass-resolved for status cleanliness.
Separate owning lanes
ContextualWisdomLab/.github#1222owns organization-required reusable SAST/Security exact-head checkout integrity.Billing lifecycle boundary
Durable Stripe event deduplication, provider-state reconciliation, and trusted entitlement activation remain separate billing-lifecycle work. This hotfix deliberately fails closed rather than granting paid rights directly from webhook JSON. A legitimate paid conversion therefore still requires the separately owned trusted reconciliation path; this PR does not claim paid-lifecycle completion.
Merge boundary
Do not merge or enable auto-merge until the unchanged exact contributor head remains reconciled to protected
develop, every applicable functional/browser/owned-coverage/docstring/CodeQL/SAST/security/dependency/supply-chain/package/provenance/required-workflow gate is substantively terminal-passing under corrected revision-evidence contracts, valid unresolved current-head defects are zero, and a qualifying independent current-head/last-push approval genuinely exists.Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, status-only, author-only, model-only, rate-limited, or infrastructure-only evidence is non-passing. Do not self-approve, manufacture approval, dismiss a valid current review, weaken protection, or transfer predecessor evidence.
Refs #488, #592, #593.