feat(identity): verify Google OIDC tokens locally - #24
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 (4)
📝 WalkthroughWalkthroughGoogle OIDC 클라이언트가 PKCE authorization code 교환과 RS256 ID token 검증을 수행합니다. JWKS 캐시·키 회전, 입력·응답 제한, 오류 변환을 추가했습니다. 회귀 테스트, 포맷 검사, 특정 pull request용 fixture 보정 CI 단계를 추가했습니다. ChangesGoogle OIDC 검증
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant GoogleOidcClient
participant GoogleTokenEndpoint
participant GoogleJwksEndpoint
Application->>GoogleOidcClient: authorization code와 PKCE 입력 전달
GoogleOidcClient->>GoogleTokenEndpoint: authorization code 교환 요청
GoogleTokenEndpoint-->>GoogleOidcClient: ID token 반환
GoogleOidcClient->>GoogleJwksEndpoint: 서명 키 조회
GoogleJwksEndpoint-->>GoogleOidcClient: JWKS 반환
GoogleOidcClient-->>Application: VerifiedGoogleIdentity 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/identity-service/src/google-oidc-client.ts (3)
483-497: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winJWKS 재조회가 중복 발생하고, 동시 요청이 중복 조회를 일으킵니다.
두 가지 문제가 있습니다.
- 캐시가 만료되어 Line 486에서 방금 갱신한 경우에도,
kid가 없으면 Line 490에서 다시 조회합니다. 한 번의 검증에 네트워크 호출이 두 번 발생합니다.- 진행 중인 조회를 공유하지 않습니다. 캐시 만료 시점에 동시 요청이 N개이면 JWKS 조회도 N번 발생합니다.
방금 갱신했는지 추적하고, 진행 중인 Promise를 공유하면 두 문제가 모두 해결됩니다.
♻️ 제안 변경
+ private pendingKeySet: Promise<CachedGoogleKeySet> | undefined; + private async signingKey(kid: string): Promise<KeyObject> { const nowMs = this.now().getTime(); + let refreshed = false; if (!this.keySet || this.keySet.expiresAtMs <= nowMs) { - this.keySet = await this.fetchKeySet(); + this.keySet = await this.sharedFetchKeySet(); + refreshed = true; } let key = this.keySet.keys.get(kid); - if (!key) { - this.keySet = await this.fetchKeySet(); + if (!key && !refreshed) { + this.keySet = await this.sharedFetchKeySet(); key = this.keySet.keys.get(kid); } if (!key) { return fail('Google ID token is invalid'); } return key; } + + private async sharedFetchKeySet(): Promise<CachedGoogleKeySet> { + this.pendingKeySet ??= this.fetchKeySet().finally(() => { + this.pendingKeySet = undefined; + }); + return await this.pendingKeySet; + }🤖 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/google-oidc-client.ts` around lines 483 - 497, Update signingKey to track whether the cache was refreshed during the current call, avoiding a second fetch when the requested kid is still absent. Also share an in-flight JWKS fetch Promise across concurrent signingKey calls, reusing it for cache-expiry refreshes and clearing it after completion so concurrent requests perform only one network fetch.
231-244: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuecontent-length가 없으면 본문 크기 제한이 적용되지 않습니다.
requireResponseLength는content-length헤더가 없으면 즉시 반환합니다. 이후requestJson과fetchKeySet은response.text()로 본문 전체를 읽습니다. 크기 검사는parseJsonObject에서 읽은 뒤에 수행됩니다. 따라서 chunked 응답에서는 메모리 사용량이MAXIMUM_HTTP_RESPONSE_BYTES를 넘을 수 있습니다.엔드포인트가 Google 고정 호스트이므로 위험도는 낮습니다. 다만 스트리밍 단계에서 바이트 수를 누적 검사하면 보장이 완전해집니다.
🤖 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/google-oidc-client.ts` around lines 231 - 244, Update requireResponseLength and the response-reading flows in requestJson and fetchKeySet so responses without a content-length header are still limited during streaming. Accumulate received body bytes while reading and fail immediately when the total exceeds MAXIMUM_HTTP_RESPONSE_BYTES, while preserving existing validation for declared content lengths and successful responses within the limit.
271-280: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRSA 공개 키의 최소 modulus 길이를 검사하십시오.
현재 검사는 512비트 RSA 키도 수용합니다.
key.asymmetricKeyDetails?.modulusLength가2_048미만이거나 정의되지 않은 경우 키를 거부하십시오. 프로젝트의 Node.js 대상 버전인 22에서 이 API를 사용할 수 있습니다.♻️ 제안 변경
if (key.asymmetricKeyType !== 'rsa') { return fail('Google signing key set is invalid'); } + const modulusLength = key.asymmetricKeyDetails?.modulusLength; + if (typeof modulusLength !== 'number' || modulusLength < 2_048) { + return fail('Google signing key set is invalid'); + } return { kid, key };🤖 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/google-oidc-client.ts` around lines 271 - 280, Update the RSA validation in the JWK handling flow after createPublicKey and the asymmetricKeyType check to reject keys when key.asymmetricKeyDetails?.modulusLength is undefined or below 2,048 bits, returning the existing fail('Google signing key set is invalid') result; continue returning { kid, key } only for valid RSA keys.apps/identity-service/src/google-oidc-client.test.ts (1)
209-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win캐시 적중(cache hit) 자체는 검증되지 않습니다. 테스트 이름과 실제 검증 범위가 다릅니다.
이 테스트의 이름은 "caches signing keys and refreshes once when Google rotates to an unknown kid"입니다. 그러나 두 번의
authenticateAuthorizationCode호출은 서로 다른 kid(key-one,key-two)를 사용하므로, 매 호출마다 JWKS 조회가 발생하는 것이 당연한 결과입니다.keyRequests가 2가 되는 것은 "회전 시 재조회"만 증명하며, "동일 kid를 재사용할 때 캐시가 실제로 적중해 추가 네트워크 호출을 피하는지"는 검증하지 않습니다.캐싱 동작 자체를 확인하려면, 동일한
key-one으로 서명한 토큰으로 세 번째 호출을 추가하고keyRequests가 여전히 2로 유지되는지 단언하십시오.♻️ 캐시 적중을 검증하는 테스트 케이스 추가 제안
await expect( client.authenticateAuthorizationCode(authenticationInput()), ).resolves.toMatchObject({ subject: '107691503500061507151' }); + await expect( + client.authenticateAuthorizationCode({ + ...authenticationInput(), + code: `${AUTHORIZATION_CODE}-repeat`, + }), + ).resolves.toMatchObject({ subject: '107691503500061507151' }); - expect(tokenRequests).toBe(2); - expect(keyRequests).toBe(2); + expect(tokenRequests).toBe(3); + expect(keyRequests).toBe(2);🤖 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/google-oidc-client.test.ts` around lines 209 - 246, Extend the test around authenticateAuthorizationCode to perform a third authentication using a token signed with the already cached key-one kid, then assert keyRequests remains 2. Update the token fixture sequence or fetcher responses as needed so this call uses the same key-one token while preserving verification of the key-two rotation refresh.
🤖 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 @.github/workflows/validate-google-oidc-verifier.yml:
- Around line 72-89: Update the “Commit validated formatting” workflow step so
it does not push the event SHA directly to feat/google-oidc-verifier. Prefer
removing the CI source push; otherwise fetch the current remote branch and
safely rebase or otherwise apply the generated commit onto its latest tip before
pushing, while preserving the existing validation and no-change behavior.
- Around line 8-9: Update the validation job’s permissions from contents: write
to contents: read and disable persisted Git credentials for the checkout before
pnpm install by setting persist-credentials to false. Keep any formatting commit
logic out of this validation job and move it to a separate trusted job.
- Around line 48-51: Commit the generated pnpm-lock.yaml for the repository,
then update the “Install dependencies” step in validate-google-oidc-verifier.yml
to use pnpm install --frozen-lockfile instead of --no-frozen-lockfile, ensuring
CI fails when package.json and the lockfile diverge.
In `@apps/identity-service/src/google-oidc-client.test.ts`:
- Around line 149-156: Update the mock token fixtures in the test response,
especially access_token near the identity verification assertions, to use
scanner-safe placeholder values such as MOCK_ACCESS_TOKEN_PLACEHOLDER and its
refresh-token equivalent. Preserve the test’s intent and verify no production
credentials are introduced; follow the repository’s AppGuardrail suppression
conventions if a fixture rename is insufficient.
---
Nitpick comments:
In `@apps/identity-service/src/google-oidc-client.test.ts`:
- Around line 209-246: Extend the test around authenticateAuthorizationCode to
perform a third authentication using a token signed with the already cached
key-one kid, then assert keyRequests remains 2. Update the token fixture
sequence or fetcher responses as needed so this call uses the same key-one token
while preserving verification of the key-two rotation refresh.
In `@apps/identity-service/src/google-oidc-client.ts`:
- Around line 483-497: Update signingKey to track whether the cache was
refreshed during the current call, avoiding a second fetch when the requested
kid is still absent. Also share an in-flight JWKS fetch Promise across
concurrent signingKey calls, reusing it for cache-expiry refreshes and clearing
it after completion so concurrent requests perform only one network fetch.
- Around line 231-244: Update requireResponseLength and the response-reading
flows in requestJson and fetchKeySet so responses without a content-length
header are still limited during streaming. Accumulate received body bytes while
reading and fail immediately when the total exceeds MAXIMUM_HTTP_RESPONSE_BYTES,
while preserving existing validation for declared content lengths and successful
responses within the limit.
- Around line 271-280: Update the RSA validation in the JWK handling flow after
createPublicKey and the asymmetricKeyType check to reject keys when
key.asymmetricKeyDetails?.modulusLength is undefined or below 2,048 bits,
returning the existing fail('Google signing key set is invalid') result;
continue returning { kid, key } only for valid RSA keys.
🪄 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: 215ea885-69b6-46f6-9557-d9010ce00a23
📒 Files selected for processing (5)
.github/workflows/validate-google-oidc-verifier.ymlapps/identity-service/src/google-oidc-client.test.tsapps/identity-service/src/google-oidc-client.tsdocs/superpowers/plans/2026-08-03-google-oidc-verifier-slice.mdpackage.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 53-102: Remove the entire “Apply scanner-safe token fixtures”
workflow step, including its branch-specific condition, Python rewrite logic,
formatting, commit, and push commands. Move the fixture definitions and
references into google-oidc-client.test.ts directly by using MOCK_ACCESS_VALUE
and MOCK_REFRESH_VALUE for the access and refresh token values and corresponding
assertions, leaving the existing CLIENT_CREDENTIAL pattern intact.
- Around line 39-40: Update the checkout step to set persist-credentials to
false, preventing the write-capable token from remaining available during
dependency installation and other job steps. Before the fixture push flow,
explicitly configure the required credentials only for that push. Remove
fetch-depth: 0 unless the workflow demonstrably requires the full Git history,
preserving the default shallow checkout otherwise.
🪄 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: f5982cc5-7bd5-4a46-b3d1-4fa0f5436193
📒 Files selected for processing (3)
.github/workflows/ci.ymlapps/identity-service/src/google-oidc-client.test.tsapps/identity-service/src/google-oidc-client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/identity-service/src/google-oidc-client.ts
- apps/identity-service/src/google-oidc-client.test.ts
98ad1a9 to
b7c7be7
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
2 participants
Summary
Continues issue #18 with the provider-verification boundary required before Google callbacks may provision identities or issue sessions.
Evidence
The design follows Google's current OpenID Connect and web-server OAuth guidance, Google's server-side ID-token verification requirements, RFC 7636 PKCE, and RFC 7517 JWK handling. Runtime discovery and caller-selected endpoints are intentionally excluded to avoid SSRF and malicious-endpoint classes.
Validation gate
Merge only after formatting, type checking, tests, build, AppGuardrail, Semgrep, security scanning, Commercial Readiness, and CodeRabbit complete on the exact current head with no actionable finding.
Part of #18.
Summary by CodeRabbit