feat(identity): bound OAuth provider HTTP requests - #25
Conversation
|
Warning Review limit reached
Next review available in: 38 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 (2)
📝 WalkthroughWalkthroughOAuth 공급자 HTTP 클라이언트를 추가했습니다. Google 및 GitHub 엔드포인트를 검증하고, 제한된 HTTPS 요청과 응답 처리를 수행합니다. 타임아웃, 리다이렉트, 본문 크기, UTF-8, JSON, 오류 비노출 동작을 테스트합니다. ChangesOAuth 공급자 HTTP 클라이언트
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BoundedOAuthProviderHttpClient
participant OAuthProvider
Client->>BoundedOAuthProviderHttpClient: OAuthProviderHttpRequest 전달
BoundedOAuthProviderHttpClient->>BoundedOAuthProviderHttpClient: URL, 헤더, 본문, 타임아웃 검증
BoundedOAuthProviderHttpClient->>OAuthProvider: 제한된 HTTPS 요청 전송
OAuthProvider-->>BoundedOAuthProviderHttpClient: 상태와 JSON 응답 반환
BoundedOAuthProviderHttpClient-->>Client: 검증된 OAuthProviderHttpResult 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (7)
apps/identity-service/src/oauth-provider-http-client.ts (2)
251-280: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value본문 상한 초과 시 스트림을 취소하세요.
상한 초과나 디코딩 실패에서
readBoundedBody는releaseLock만 호출합니다. 남은 응답 본문은 소비되지도 취소되지도 않습니다.execute의catch가abortController.abort()를 호출하므로 실제 누수는 완화됩니다. 방어적으로reader.cancel()을 추가하면 런타임 구현에 의존하지 않고 소켓을 즉시 해제합니다.♻️ 제안 변경
} catch { return failProviderRequest(); } finally { + void reader.cancel().catch(() => undefined); reader.releaseLock(); }🤖 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-provider-http-client.ts` around lines 251 - 280, Update readBoundedBody to cancel the response body reader when the byte limit is exceeded or decoding fails, before releasing its lock. Preserve the existing failProviderRequest behavior and ensure cancellation is handled defensively without replacing the existing finally cleanup.
290-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win기본
fetch를 바인딩하고 부재 시 실패를 명확히 하세요.
globalThis.fetch는 바인딩 없이 저장됩니다. Node 런타임에서는 동작합니다. 다른 fetch 구현이나 브라우저 유사 환경에서는 분리 호출이Illegal invocation을 발생시킬 수 있습니다.globalThis.fetch가 없으면execute시점에 일반화되지 않은TypeError가 발생합니다. 생성 시점에 검증하면 실패가 항상OAuth provider request failed로 유지됩니다.♻️ 제안 변경
constructor(options: OAuthProviderHttpClientOptions = {}) { - this.fetchFunction = options.fetchFunction ?? globalThis.fetch; + const defaultFetch = + typeof globalThis.fetch === 'function' + ? (globalThis.fetch.bind(globalThis) as OAuthProviderFetch) + : undefined; + this.fetchFunction = + options.fetchFunction ?? defaultFetch ?? failProviderRequest(); this.timeoutMs = requireTimeout(options.timeoutMs); }🤖 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-provider-http-client.ts` around lines 290 - 293, Update the constructor of OAuthProviderHttpClient to bind the default globalThis.fetch before storing it, while preserving custom options.fetchFunction unchanged. Validate that a usable fetch implementation exists during construction and raise the client’s standardized failure type/message so execute consistently surfaces “OAuth provider request failed” instead of an unhandled invocation or generic TypeError.apps/identity-service/src/tests/oauth-provider-http-client.test.ts (4)
249-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value경계값 수용 케이스를 추가하세요.
현재 테스트는 거부만 확인합니다.
timeoutMs: 100과timeoutMs: 10_000이 수용되는지도 확인하세요. 경계 조건의 off-by-one 회귀를 막습니다. 정수가 아닌 값에 대한 거부 케이스도 추가하세요.🤖 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/tests/oauth-provider-http-client.test.ts` around lines 249 - 256, Extend the unsafe timeout configuration test around BoundedOAuthProviderHttpClient to verify boundary values 100 and 10_000 are accepted, while retaining rejection checks for 99 and 10_001. Add rejection coverage for non-integer timeoutMs values as well.
168-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value한 테스트가 서로 다른 세 가지 동작을 검증합니다.
리다이렉트 상태 거부, 비 JSON 콘텐츠 타입 거부, 본문 크기 초과 거부를 각각의 테스트로 분리하세요. 실패 시 원인 식별이 쉬워집니다. 세 케이스가 모두 동일한 오류 메시지를 발생시키므로 현재 구성에서는 어느 단계가 실패했는지 구분되지 않습니다.
참고: fetch가 목이므로 이 케이스는
redirect: 'error'전달이 아니라 상태 코드 범위 검사를 검증합니다.🤖 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/tests/oauth-provider-http-client.test.ts` around lines 168 - 201, Split the combined test into three independent tests covering redirect status rejection, non-JSON content-type rejection, and oversized response-body rejection. Keep each test focused on one response and assert the existing generic “OAuth provider request failed” error, while preserving validation of status-code range checks rather than fetch redirect options.
118-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueURL 거부 케이스에 유효한 헤더를 사용하세요.
두 케이스는
headers: {}를 사용합니다. 헤더 검증만으로도 거부됩니다.execute가 URL을 먼저 검증하므로 현재는 통과합니다. 유효한 GitHub 헤더를 사용하면 URL 허용 목록만 검증하는 테스트가 됩니다. 요청 본문 크기 상한과 CRLF 헤더 값 거부 케이스 추가도 고려하세요.🤖 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/tests/oauth-provider-http-client.test.ts` around lines 118 - 134, Update the “query-bearing endpoint” and “unapproved endpoint” cases in the OAuth provider HTTP client tests to use valid GitHub request headers, ensuring rejection is caused by URL allowlisting rather than empty-header validation. Keep the existing URL scenarios and consider adding separate coverage for request-body size limits and CRLF-containing header values.
67-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winfetch 목 내부의 단언은 클라이언트의
catch에 삼켜집니다.
execute는try블록의 모든 예외를 잡아OAuth provider request failed로 변환합니다. 목 안에서expect가 실패하면 원래 단언 메시지가 사라집니다. 테스트는 실패하지만 원인은 드러나지 않습니다.init을 캡처한 뒤execute호출 후에 단언하세요.♻️ 제안 변경
- const fetchFunction: OAuthProviderFetch = vi.fn(async (_input, init) => { - expect(init.method).toBe('POST'); - expect(init.redirect).toBe('error'); - expect(init.credentials).toBe('omit'); - expect(init.cache).toBe('no-store'); - expect(init.referrerPolicy).toBe('no-referrer'); - expect(init.signal).toBeInstanceOf(AbortSignal); - return jsonResponse({ token_type: 'bearer' }); - }); + let capturedInit: RequestInit | undefined; + const fetchFunction: OAuthProviderFetch = vi.fn(async (_input, init) => { + capturedInit = init; + return jsonResponse({ token_type: 'bearer' }); + }); const client = new BoundedOAuthProviderHttpClient({ fetchFunction }); const result = await client.execute(googleTokenRequest()); + + expect(capturedInit).toMatchObject({ + method: 'POST', + redirect: 'error', + credentials: 'omit', + cache: 'no-store', + referrerPolicy: 'no-referrer', + }); + expect(capturedInit?.signal).toBeInstanceOf(AbortSignal);🤖 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/tests/oauth-provider-http-client.test.ts` around lines 67 - 78, Update the test around BoundedOAuthProviderHttpClient.execute so the mocked fetchFunction only captures init during the request and does not perform expect assertions inside the mock callback. After execute returns, assert the captured request options, preserving the existing method, redirect, credentials, cache, referrerPolicy, and signal checks so assertion failures expose their original messages.package.json (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value명시적 파일 목록 대신 글롭 사용을 검토하세요.
format:check는 모든 대상 파일을 수동으로 나열합니다. 새 파일을 추가할 때 목록 갱신을 잊으면 포맷 검사가 조용히 건너뜁니다. 이번 변경의 세 파일은 정상적으로 추가되었습니다. 향후 유지 비용을 줄이려면 디렉터리 글롭과.prettierignore조합으로 전환하세요.🤖 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 `@package.json` at line 15, Update the format:check script to use directory globs for supported repository files instead of maintaining a manually enumerated file list, and add or adjust .prettierignore entries for paths that should be excluded. Preserve formatting coverage for the currently included files while ensuring newly added files are checked automatically.
🤖 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/tests/oauth-provider-http-client.test.ts`:
- Around line 244-246: OAuth provider failure test의 `client.execute(request)` 예외
검증이 `cause`와 스택을 포함하지 않습니다. 예외를 먼저 캡처한 뒤 직렬화된 전체 오류 표현을 검증하고, 원본 자격 증명 문자열이
`message`, `cause`, `stack`을 포함한 결과 어디에도 나타나지 않는지 확인하세요.
---
Nitpick comments:
In `@apps/identity-service/src/oauth-provider-http-client.ts`:
- Around line 251-280: Update readBoundedBody to cancel the response body reader
when the byte limit is exceeded or decoding fails, before releasing its lock.
Preserve the existing failProviderRequest behavior and ensure cancellation is
handled defensively without replacing the existing finally cleanup.
- Around line 290-293: Update the constructor of OAuthProviderHttpClient to bind
the default globalThis.fetch before storing it, while preserving custom
options.fetchFunction unchanged. Validate that a usable fetch implementation
exists during construction and raise the client’s standardized failure
type/message so execute consistently surfaces “OAuth provider request failed”
instead of an unhandled invocation or generic TypeError.
In `@apps/identity-service/src/tests/oauth-provider-http-client.test.ts`:
- Around line 249-256: Extend the unsafe timeout configuration test around
BoundedOAuthProviderHttpClient to verify boundary values 100 and 10_000 are
accepted, while retaining rejection checks for 99 and 10_001. Add rejection
coverage for non-integer timeoutMs values as well.
- Around line 168-201: Split the combined test into three independent tests
covering redirect status rejection, non-JSON content-type rejection, and
oversized response-body rejection. Keep each test focused on one response and
assert the existing generic “OAuth provider request failed” error, while
preserving validation of status-code range checks rather than fetch redirect
options.
- Around line 118-134: Update the “query-bearing endpoint” and “unapproved
endpoint” cases in the OAuth provider HTTP client tests to use valid GitHub
request headers, ensuring rejection is caused by URL allowlisting rather than
empty-header validation. Keep the existing URL scenarios and consider adding
separate coverage for request-body size limits and CRLF-containing header
values.
- Around line 67-78: Update the test around
BoundedOAuthProviderHttpClient.execute so the mocked fetchFunction only captures
init during the request and does not perform expect assertions inside the mock
callback. After execute returns, assert the captured request options, preserving
the existing method, redirect, credentials, cache, referrerPolicy, and signal
checks so assertion failures expose their original messages.
In `@package.json`:
- Line 15: Update the format:check script to use directory globs for supported
repository files instead of maintaining a manually enumerated file list, and add
or adjust .prettierignore entries for paths that should be excluded. Preserve
formatting coverage for the currently included files while ensuring newly added
files are checked automatically.
🪄 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: 06d5b2e6-d09b-4206-b098-5f08b3d204b7
📒 Files selected for processing (4)
apps/identity-service/src/oauth-provider-http-client.tsapps/identity-service/src/tests/oauth-provider-http-client.test.tsdocs/superpowers/plans/2026-08-03-oauth-provider-http-client-slice.mdpackage.json
|
Addressed the provider transport cleanup, bound default fetch handling, timeout boundaries, focused response tests, valid-header URL tests, mock assertion placement, and full credential-redaction coverage. I left the repository-wide |
1 participant
Summary
Validation
Progresses #18.
Summary by CodeRabbit
새 기능
문서
테스트