feat(identity): add secure OAuth HTTP boundary - #22
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOAuth HTTP 경계와 애플리케이션 계층을 추가했습니다. 쿠키와 콜백 입력을 검증하고, OAuth 시작·세션 조회·로그아웃·고정 리디렉션을 처리합니다. 관련 테스트, 계획 문서와 포맷 검사 대상도 추가했습니다. ChangesOAuth HTTP 경계
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/identity-service/src/oauth-http-application.ts (1)
47-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win알 수 없는 provider 값에 대한 방어가 없습니다.
requireProviderConfiguration은configuration.providers[provider]가 없을 때를 확인하지 않습니다.provider가 실제로'google'또는'github'가 아니면providerConfiguration은undefined가 되고, 다음 줄의providerConfiguration.clientId가 처리되지 않은TypeError를 던집니다.
beginAuthorization(82-96번째 줄)은 이 함수를 런타임provider인자로 호출합니다. 생성자 호출(75-76번째 줄)은 고정 리터럴만 사용하므로 안전하지만,beginAuthorization은 향후 컨트롤러 슬라이스에서 라우트 파라미터를 그대로 받을 가능성이 높은 지점입니다. 현재 방어가 없으면 잘못된 provider 값이 자격 증명 없는 RFC 9457 문제 응답 대신 처리되지 않은 예외로 이어집니다.🛡️ 제안하는 수정
function requireProviderConfiguration( provider: IdentityProvider, configuration: OAuthHttpApplicationConfiguration, ): OAuthProviderStartConfiguration { const providerConfiguration = configuration.providers[provider]; + if (!providerConfiguration) { + throw new Error('OAuth provider is not supported'); + } const clientId = providerConfiguration.clientId.trim();Also applies to: 82-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/identity-service/src/oauth-http-application.ts` around lines 47 - 60, Update requireProviderConfiguration to validate that configuration.providers[provider] exists before accessing clientId, and reject unknown provider values with the established handled error or RFC 9457 problem-response path used by beginAuthorization. Preserve the existing client ID trimming and redirect URI validation for configured providers.apps/identity-service/src/oauth-http-boundary.ts (1)
108-124: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win일반 객체를 쿠키 맵으로 사용하면 프로토타입 체인 속성과 충돌합니다.
cookies는 일반 객체 리터럴({})입니다. 쿠키 이름이constructor,toString,__proto__같은Object.prototype속성명과 같으면,cookies[name] !== undefined검사가 항상 참이 되어 정상적인 첫 쿠키도 중복으로 오판하고 거부됩니다.COOKIE_NAME_PATTERN은 이런 이름을 허용하므로 실제로 발생할 수 있습니다.현재는 이 오판이
failInvalidCookie()로 이어져 실제 프로토타입 오염까지는 차단되지만, 이는 우연한 방어이며 향후 로직 변경 시 위험해질 수 있습니다.Object.create(null)을 사용하거나Object.prototype.hasOwnProperty.call(cookies, name)으로 중복 검사를 하십시오.🛡️ 제안하는 수정
- const cookies: Record<string, string> = {}; + const cookies: Record<string, string> = Object.create(null); for (const segment of header.split(';')) { const separator = segment.indexOf('='); if (separator <= 0) { return failInvalidCookie(); } const name = requireCookieName(segment.slice(0, separator).trim()); const value = segment.slice(separator + 1).trim(); if ( !value || !OPAQUE_COOKIE_VALUE_PATTERN.test(value) || - cookies[name] !== undefined + Object.prototype.hasOwnProperty.call(cookies, name) ) { return failInvalidCookie(); } cookies[name] = value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/identity-service/src/oauth-http-boundary.ts` around lines 108 - 124, Update the cookies map and duplicate-name check in the cookie parsing loop to avoid prototype-chain collisions for names such as constructor, toString, and __proto__; use a null-prototype object or an own-property check while preserving rejection of actual duplicate cookie names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/identity-service/src/oauth-http-boundary.ts`:
- Around line 94-137: Update readOpaqueCookie to scan cookie segments directly
and skip unrelated cookie names, applying OPAQUE_COOKIE_VALUE_PATTERN validation
only to the requested cookie before returning it. Preserve invalid-header
handling for malformed structure and invalid target values, and add coverage for
headers containing unrelated RFC 6265 cookie values such as periods, equals
signs, or percent-encoding.
---
Nitpick comments:
In `@apps/identity-service/src/oauth-http-application.ts`:
- Around line 47-60: Update requireProviderConfiguration to validate that
configuration.providers[provider] exists before accessing clientId, and reject
unknown provider values with the established handled error or RFC 9457
problem-response path used by beginAuthorization. Preserve the existing client
ID trimming and redirect URI validation for configured providers.
In `@apps/identity-service/src/oauth-http-boundary.ts`:
- Around line 108-124: Update the cookies map and duplicate-name check in the
cookie parsing loop to avoid prototype-chain collisions for names such as
constructor, toString, and __proto__; use a null-prototype object or an
own-property check while preserving rejection of actual duplicate cookie names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97d9d739-4279-475e-864a-c95d5e173547
📒 Files selected for processing (5)
apps/identity-service/src/oauth-http-application.tsapps/identity-service/src/oauth-http-boundary.test.tsapps/identity-service/src/oauth-http-boundary.tsdocs/superpowers/plans/2026-08-03-oauth-http-boundary-slice.mdpackage.json
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Begins issue #18 with the browser-facing security boundary required before provider callback orchestration.
Included
Scope boundary
This PR does not yet expose production NestJS callback endpoints or implement Google JWKS verification, provider HTTP transport, or account provisioning orchestration. Those remain in issue #18 and are explicitly documented as the next slice.
Verification
Do not merge until CI, SAST Semgrep, Security Scan, AppGuardrail, Commercial Readiness, CodeRabbit, and all review feedback pass on the exact current head.
Refs #18.
Summary by CodeRabbit
새로운 기능
보안 및 오류 처리
테스트 및 문서