feat(calendar): require trusted workspace context - #139
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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)
📝 WalkthroughWalkthrough캘린더 서비스에 HMAC-SHA256 기반 워크스페이스 컨텍스트 검증을 추가했다. 동기화 컨트롤러는 서명된 헤더를 검증한 뒤 워크스페이스 ID를 전달한다. 단위 테스트와 통합 테스트는 유효한 요청, 잘못된 요청, 만료 요청, 삭제 요청을 검증한다. Changes캘린더 컨텍스트 검증
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CalendarSyncController
participant requireTrustedCalendarWorkspaceContext
participant CalendarSyncService
Client->>CalendarSyncController: 서명된 워크스페이스 헤더와 동기화 요청
CalendarSyncController->>requireTrustedCalendarWorkspaceContext: 헤더와 비밀키 전달
requireTrustedCalendarWorkspaceContext-->>CalendarSyncController: 검증된 워크스페이스 ID 또는 오류
CalendarSyncController->>CalendarSyncService: 워크스페이스 ID와 요청 본문 전달
CalendarSyncService-->>Client: 동기화 응답
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
seonghobae
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/integration-calendar-service/src/calendar-sync.integration.test.ts (1)
92-99: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win성공 경로가 레거시 헤더와 서명 헤더를 같은 값으로 함께 보냅니다.
postSync는x-workspace-id와 서명된 컨텍스트 헤더를 모두 전송하며 두 값이 동일합니다. 컨트롤러가 다시x-workspace-id를 신뢰하는 회귀가 생겨도 결과가 같으므로 이 테스트는 통과합니다. 즉 이 PR의 핵심 보장이 성공 경로에서 증명되지 않습니다.레거시 헤더에 다른 워크스페이스 ID를 넣으십시오. 그러면 서명된 컨텍스트만 결과를 결정한다는 사실이 검증됩니다.
💚 제안 변경
'content-type': 'application/json', 'x-csrf-token': SYNTHETIC_CSRF_TOKEN, - 'x-workspace-id': workspaceId, + // The signed context must win over any client-selected workspace header. + 'x-workspace-id': 'attacker-selected-workspace', ...trustedWorkspaceHeaders(workspaceId),코딩 가이드라인 근거: "Tests must prove realistic domain accuracy and failure behavior, not only mocked call counts."
🤖 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/integration-calendar-service/src/calendar-sync.integration.test.ts` around lines 92 - 99, Update the successful postSync test request so the legacy x-workspace-id header contains a different workspace ID from the value supplied by trustedWorkspaceHeaders(workspaceId), while preserving the signed context for the intended workspace and all other request data.Source: Coding guidelines
🧹 Nitpick comments (7)
apps/integration-calendar-service/src/calendar-service-context.ts (2)
84-95: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win워크스페이스 ID 대소문자 계약을 명시하십시오.
UUID_V4_PATTERN은iu플래그로 대문자 UUID를 허용합니다. 그러나 서명은toLowerCase()후의 값으로 계산합니다. 게이트웨이가 대문자 UUID 문자열로 서명하면 검증은 항상 실패합니다. 원인은 헤더에 노출되지 않으므로 진단이 어렵습니다.소문자 정규 형식만 계약으로 정하십시오. 패턴에서
i플래그를 제거하면 형식 위반이 즉시 드러납니다.♻️ 제안 변경
-const UUID_V4_PATTERN = - /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/iu; +/** Canonical lowercase UUIDv4 form required in the signed workspace header. */ +const UUID_V4_PATTERN = + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/u;- const workspaceId = headers.workspaceId.toLowerCase(); + const workspaceId = headers.workspaceId;🤖 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/integration-calendar-service/src/calendar-service-context.ts` around lines 84 - 95, Update UUID_V4_PATTERN validation in the calendar context header check to accept only lowercase workspace IDs by removing its case-insensitive flag. Preserve the existing lowercase normalization used by expectedSignature, while ensuring uppercase UUID headers are rejected immediately as invalid input.
45-59: 🔒 Security & Privacy | 🔵 Trivial재사용 창 60초에 대한 리플레이 방어 확인.
현재 검증은 서명 유효 창 안에서 동일한 컨텍스트의 재사용을 막지 않습니다. 공격자가 헤더 3개를 가로채면 최대 65초 동안 같은 워크스페이스로 요청을 반복할 수 있습니다. 게이트웨이 앞단에서 TLS와 단일 사용 nonce 중 어떤 방어가 이 위험을 담당하는지 문서화하십시오. nonce를 이 계층에서 담당해야 하면, 서명 대상 문자열에 nonce를 추가하고 짧은 TTL 캐시로 중복을 거부하십시오.
🤖 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/integration-calendar-service/src/calendar-service-context.ts` around lines 45 - 59, requireIssuedAt 검증만으로는 서명 유효 기간 내 컨텍스트 재사용을 막을 수 없으므로, 게이트웨이의 TLS 또는 단일 사용 nonce 중 어느 계층이 리플레이 방어를 담당하는지 문서화하십시오. 이 계층이 nonce를 담당한다면 Calendar 컨텍스트 서명 대상에 nonce를 포함하고, 짧은 TTL 캐시에 nonce를 기록해 중복 요청을 거부하도록 관련 검증 흐름과 requireIssuedAt를 갱신하십시오.apps/integration-calendar-service/src/calendar-service-context.test.ts (1)
8-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동적 import 래퍼를 정적 import로 교체하십시오.
catch(() => ({}))가 실제 로딩 오류를 삼킵니다. 모듈에 구문 오류나 순환 참조가 생기면 테스트는 원인 대신typeof undefined만 보고합니다. 로컬 인터페이스CalendarContextModule도 실제 소스 타입과 따로 관리되므로 시그니처 변경을 감지하지 못합니다.정적 import를 사용하십시오. 타입스크립트가 컴파일 시점에 export 존재와 시그니처를 검증합니다.
♻️ 제안 변경
import { createHmac, randomBytes } from 'node:crypto'; import { describe, expect, it } from 'vitest'; +import { requireTrustedCalendarWorkspaceContext } from './calendar-service-context'; const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'; const TEST_CONTEXT_KEY = randomBytes(32).toString('base64url'); const NOW_SECONDS = 1_786_291_200; - -interface CalendarContextModule { - requireTrustedCalendarWorkspaceContext( - headers: Readonly<{ - workspaceId: unknown; - issuedAt: unknown; - signature: unknown; - }>, - secret: unknown, - nowSeconds?: number, - ): string; -} - -async function contextModule(): Promise<CalendarContextModule> { - const modulePath = './calendar-service-context'; - const module = (await import(modulePath).catch(() => ({}))) as Readonly< - Record<string, unknown> - >; - expect(typeof module.requireTrustedCalendarWorkspaceContext).toBe('function'); - return module as unknown as CalendarContextModule; -}🤖 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/integration-calendar-service/src/calendar-service-context.test.ts` around lines 8 - 27, Replace the dynamic import and swallowing catch in contextModule with a static import of requireTrustedCalendarWorkspaceContext from the calendar service context module. Remove the local CalendarContextModule interface and wrapper type assertions, and update the test setup to use the statically imported function so export presence and signature changes are checked at compile time.apps/integration-calendar-service/src/calendar-sync.integration.test.ts (2)
102-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win컨텍스트 사용 불가 경로(503)의 HTTP 매핑이 검증되지 않습니다.
이 테스트는 401 경로와 성공 경로를 확인합니다. 그러나
CALENDAR_GATEWAY_CONTEXT_SECRET가 없거나 32바이트 미만일 때의 503calendar_context_unavailable응답은 어떤 테스트도 확인하지 않습니다.main.ts의 해당 분기가 401로 잘못 바뀌어도 감지되지 않습니다.시크릿을 제거한 상태의 요청이 503을 반환하고 리소스를 생성하지 않는지 확인하는 케이스를 추가하십시오.
코딩 가이드라인 근거: "Packages that enforce coverage gates must retain 100% statement, branch, function, and line coverage."
🤖 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/integration-calendar-service/src/calendar-sync.integration.test.ts` around lines 102 - 151, Extend the calendar synchronization HTTP boundary tests around the existing app setup to cover the unavailable-context path: remove or invalidate CALENDAR_GATEWAY_CONTEXT_SECRET, send a request with the required CSRF and workspace context headers, and assert a 503 response with the calendar_context_unavailable error while confirming provider.resources remains empty. Restore the environment variable and close the app in all cases, preserving the existing trusted and legacy-path assertions.Source: Coding guidelines
250-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value환경변수 설정을
beforeEach의vi.stubEnv와afterEach로 정리하세요.현재 두
it블록에서previousSecret을 저장·복원하는 코드가 반복됩니다. Vitest는vi.stubEnv의 원래 값을 추적하고vi.unstubAllEnvs()로 복원하므로,CALENDAR_GATEWAY_CONTEXT_SECRET설정을beforeEach에 넣고 테스트 종료 시vi.unstubAllEnvs()를 호출하도록 만들면 중복과 복원 누락 위험이 줄어듭니다.🤖 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/integration-calendar-service/src/calendar-sync.integration.test.ts` around lines 250 - 254, Refactor the affected tests to configure CALENDAR_GATEWAY_CONTEXT_SECRET in a shared beforeEach using vi.stubEnv, and restore environment state in afterEach with vi.unstubAllEnvs(). Remove the repeated previousSecret capture, conditional deletion, and manual restoration from both it blocks while preserving each test’s intended secret value.apps/integration-calendar-service/src/main.ts (2)
76-90: 🔒 Security & Privacy | 🔵 Trivial컨텍스트 검증 실패에 대한 관측 신호를 추가하십시오.
401과 503 응답은 본문이 최소한으로 유지되어 적절합니다. 그러나 실패가 기록되지 않으므로 서명 위조 시도와 시크릿 설정 오류를 운영에서 구분할 수 없습니다.
두 분기에 구조화 로그 또는 카운터를 추가하십시오. 워크스페이스 ID, 서명, 시크릿은 기록하지 마십시오. 실패 종류와 발생 횟수만 기록하면 충분합니다.
🤖 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/integration-calendar-service/src/main.ts` around lines 76 - 90, CalendarContextInvalidError 및 CalendarContextUnavailableError 처리 분기에 구조화 로그나 카운터를 추가해 각각의 검증 실패 유형과 발생 횟수를 기록하십시오. 로그에는 워크스페이스 ID, 서명, 시크릿을 포함하지 말고, 기존 401·503 응답 본문과 problem 반환 동작은 유지하십시오.
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift컨텍스트 시크릿을 부팅 시점에 검증하십시오.
핸들러가 요청마다
process.env.CALENDAR_GATEWAY_CONTEXT_SECRET를 직접 읽습니다. 시크릿이 없거나 32바이트 미만이면 첫 동기화 요청이 도착할 때까지 오류가 드러나지 않습니다. 잘못된 배포가 헬스체크를 통과합니다.부트스트랩에서 시크릿을 한 번 검증하고, 검증된 값을 DI로 주입하십시오. 그러면 설정 오류가 배포 시점에 드러납니다. 통합 테스트는 모듈 등록 시 시크릿을 전달하도록 조정하십시오.
코딩 가이드라인 근거: "Each service owns its migrations, runtime configuration, observability, tests, and shutdown behavior." 및 "Fail closed on malformed ownership, identifiers, signatures, digests, timestamps, pagination, or provider configuration."
🤖 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/integration-calendar-service/src/main.ts` around lines 70 - 75, 부팅 단계에서 CALENDAR_GATEWAY_CONTEXT_SECRET의 존재 여부와 최소 32바이트 길이를 검증하고, 검증된 값을 의존성 주입으로 핸들러에 전달하십시오. 요청 처리부의 requireTrustedCalendarWorkspaceContext 호출은 process.env를 직접 읽지 말고 주입된 시크릿을 사용해야 하며, 모듈 등록 시 해당 시크릿을 제공하도록 통합 테스트도 갱신하십시오.Source: Coding guidelines
🤖 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/integration-calendar-service/src/calendar-service-context.test.ts`:
- Around line 61-85: Update
apps/integration-calendar-service/src/calendar-service-context.test.ts lines
61-85 to assert CalendarContextInvalidError for each invalid context and
CalendarContextUnavailableError for the short-secret case. Add a case in
apps/integration-calendar-service/src/calendar-sync.integration.test.ts lines
102-151 verifying that a request without the secret returns HTTP 503 with
calendar_context_unavailable and does not create a resource.
---
Outside diff comments:
In `@apps/integration-calendar-service/src/calendar-sync.integration.test.ts`:
- Around line 92-99: Update the successful postSync test request so the legacy
x-workspace-id header contains a different workspace ID from the value supplied
by trustedWorkspaceHeaders(workspaceId), while preserving the signed context for
the intended workspace and all other request data.
---
Nitpick comments:
In `@apps/integration-calendar-service/src/calendar-service-context.test.ts`:
- Around line 8-27: Replace the dynamic import and swallowing catch in
contextModule with a static import of requireTrustedCalendarWorkspaceContext
from the calendar service context module. Remove the local CalendarContextModule
interface and wrapper type assertions, and update the test setup to use the
statically imported function so export presence and signature changes are
checked at compile time.
In `@apps/integration-calendar-service/src/calendar-service-context.ts`:
- Around line 84-95: Update UUID_V4_PATTERN validation in the calendar context
header check to accept only lowercase workspace IDs by removing its
case-insensitive flag. Preserve the existing lowercase normalization used by
expectedSignature, while ensuring uppercase UUID headers are rejected
immediately as invalid input.
- Around line 45-59: requireIssuedAt 검증만으로는 서명 유효 기간 내 컨텍스트 재사용을 막을 수 없으므로,
게이트웨이의 TLS 또는 단일 사용 nonce 중 어느 계층이 리플레이 방어를 담당하는지 문서화하십시오. 이 계층이 nonce를 담당한다면
Calendar 컨텍스트 서명 대상에 nonce를 포함하고, 짧은 TTL 캐시에 nonce를 기록해 중복 요청을 거부하도록 관련 검증 흐름과
requireIssuedAt를 갱신하십시오.
In `@apps/integration-calendar-service/src/calendar-sync.integration.test.ts`:
- Around line 102-151: Extend the calendar synchronization HTTP boundary tests
around the existing app setup to cover the unavailable-context path: remove or
invalidate CALENDAR_GATEWAY_CONTEXT_SECRET, send a request with the required
CSRF and workspace context headers, and assert a 503 response with the
calendar_context_unavailable error while confirming provider.resources remains
empty. Restore the environment variable and close the app in all cases,
preserving the existing trusted and legacy-path assertions.
- Around line 250-254: Refactor the affected tests to configure
CALENDAR_GATEWAY_CONTEXT_SECRET in a shared beforeEach using vi.stubEnv, and
restore environment state in afterEach with vi.unstubAllEnvs(). Remove the
repeated previousSecret capture, conditional deletion, and manual restoration
from both it blocks while preserving each test’s intended secret value.
In `@apps/integration-calendar-service/src/main.ts`:
- Around line 76-90: CalendarContextInvalidError 및
CalendarContextUnavailableError 처리 분기에 구조화 로그나 카운터를 추가해 각각의 검증 실패 유형과 발생 횟수를
기록하십시오. 로그에는 워크스페이스 ID, 서명, 시크릿을 포함하지 말고, 기존 401·503 응답 본문과 problem 반환 동작은
유지하십시오.
- Around line 70-75: 부팅 단계에서 CALENDAR_GATEWAY_CONTEXT_SECRET의 존재 여부와 최소 32바이트
길이를 검증하고, 검증된 값을 의존성 주입으로 핸들러에 전달하십시오. 요청 처리부의
requireTrustedCalendarWorkspaceContext 호출은 process.env를 직접 읽지 말고 주입된 시크릿을 사용해야
하며, 모듈 등록 시 해당 시크릿을 제공하도록 통합 테스트도 갱신하십시오.
🪄 Autofix
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: 08c3aa81-a623-46a7-81be-7b8471a31b57
📒 Files selected for processing (4)
apps/integration-calendar-service/src/calendar-service-context.test.tsapps/integration-calendar-service/src/calendar-service-context.tsapps/integration-calendar-service/src/calendar-sync.integration.test.tsapps/integration-calendar-service/src/main.ts
|
@coderabbitai review |
|
2 participants
Intent
Advance issue #129 with the narrowest security prerequisite: calendar synchronization must stop accepting a browser/client-selected workspace header as tenant authority and instead verify a short-lived signed server-derived workspace context.
Implemented on the current branch
CalendarSyncControllerreadsx-life-os-workspace-id, issued-at, and signature rather than trusting legacyx-workspace-idExact-head evidence
For head
e7d724007dd06203093f4b93d6b68d3ec42a0a59, CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, and CodeRabbit status are successful. The two current-head hard-coded HMAC-key findings were fixed and their review threads resolved. The PR is Ready for review.Security boundary
Provider credentials are not part of the signed context and are not exposed in errors, logs, or retained artifacts. The context authenticates only the server-derived workspace identity and freshness; downstream calendar operations still use the existing provider adapter contracts.
Remaining before merge
mainimmediately before any mergeImplemented on active PRuntil protected integrationThis is not the complete #129 credential lifecycle. Encrypted per-user token persistence, OAuth state/PKCE, refresh/revocation, calendar discovery/selection, and migration from the development-only process token remain follow-up slices.
Refs #129 and #21.
Summary by CodeRabbit
보안 강화
오류 처리
401, 컨텍스트를 사용할 수 없는 경우에는503을 반환합니다.