fix(planning): bind signed workspace authority to request - #188
Conversation
|
Warning Review limit reached
Next review available in: 40 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 (1)
📝 WalkthroughWalkthroughPlanning 컨텍스트 서명이 HTTP 메서드와 경로에 바인딩되도록 변경되었습니다. 웹 클라이언트, Gateway, Planning 라우트가 동일한 요청 바인딩을 사용합니다. 잘못된 바인딩과 서명 재사용을 거부하는 테스트가 추가되었습니다. ChangesPlanning 요청 바인딩
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebClient
participant Gateway
participant PlanningRoute
participant ContextVerifier
WebClient->>Gateway: Planning 요청 전달
Gateway->>Gateway: 메서드·경로를 포함한 v2 HMAC 생성
Gateway->>PlanningRoute: 바인딩된 HMAC 헤더와 요청 전달
PlanningRoute->>ContextVerifier: 요청 바인딩과 workspace context 검증
ContextVerifier-->>PlanningRoute: 검증된 workspace context 또는 401
PlanningRoute-->>WebClient: Planning 응답 반환
Possibly related PRs
🚥 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}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?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: 2
🧹 Nitpick comments (3)
apps/planning-service/src/request-bound-workspace-context.test.ts (1)
23-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
expectInvalid가 자신의 sentinel 오류를 다시 잡습니다.
operation()이 성공하면 26행이Error('Expected request-bound context to be rejected')를 던집니다. 이 오류는 바로 아래 27행의catch가 잡습니다. 테스트는 여전히 실패합니다. 그러나 실패 메시지는 "HttpException이 아님"으로 표시됩니다. 의도한 메시지는 사라집니다. 회귀가 발생하면 원인 파악이 느려집니다.
expect(...).toThrow()대신 sentinel을try블록 밖으로 옮겨 주세요.💚 제안 수정
function expectInvalid(operation: () => unknown): void { + let succeeded = false; try { operation(); - throw new Error('Expected request-bound context to be rejected'); + succeeded = true; } catch (error) { + if (succeeded) throw error; expect(error).toBeInstanceOf(HttpException); expect((error as HttpException).getStatus()).toBe(401); expect((error as HttpException).getResponse()).toEqual({ type: 'about:blank', title: 'Trusted gateway context is invalid', status: 401, code: 'invalid_gateway_context', }); + return; } + throw new Error('Expected request-bound context to be rejected'); }🤖 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/planning-service/src/request-bound-workspace-context.test.ts` around lines 23 - 37, Update expectInvalid so the sentinel Error is thrown outside the try/catch, preventing it from being caught and reported as an HttpException assertion failure. Keep operation() inside the try block and preserve the existing status and response assertions for errors it actually throws.apps/web/app/planning-search-client.ts (1)
165-177: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
requirePlanningBinding이method를 검증하지 않고, 경로 규칙이 서버 허용목록보다 느슨합니다.
requirePlanningBinding은path만 검사합니다.method는 타입 수준에서만 제한됩니다.apps/web/app/today-sync-client.ts344-347행은request.method as 'GET' | 'PUT'로 단언해서 값을 전달합니다. 현재는 상위 코드가 메서드를 먼저 검사하므로 안전합니다. 그러나 런타임 검증이 없으므로 이후 호출자가 추가되면 임의 문자열이 서명 입력에 들어갈 수 있습니다.또한 이 검증기는
/v1/접두사만 요구합니다.apps/planning-service/src/http-boundary.ts72-127행은 정확한 라우트 조합만 허용합니다. 클라이언트가 서버 허용목록 밖의 바인딩을 서명하면 요청은 401로 실패합니다. 보안 위험은 없습니다. 다만 신규 라우트 추가 시 두 목록이 조용히 어긋날 수 있습니다.♻️ 메서드 런타임 검증 추가 제안
function requirePlanningBinding( binding: PlanningContextRequestBinding, ): PlanningContextRequestBinding { if ( + (binding.method !== 'GET' && + binding.method !== 'POST' && + binding.method !== 'PUT') || !binding.path.startsWith('/v1/') || binding.path.length > 256 || /[\u0000-\u001f\u007f?#]/u.test(binding.path) ) { throw new Error('Planning request binding is invalid'); } return binding; }As per coding guidelines: "Treat every external response, stored JSON value, environment value, model output, and connector result as untrusted until bounded and validated."
🤖 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/web/app/planning-search-client.ts` around lines 165 - 177, Update requirePlanningBinding to validate binding.method at runtime, accepting only the methods supported by the Planning client rather than relying on the TypeScript type. Replace the broad /v1/ prefix check with validation against the exact server-supported method/path combinations, keeping the existing length and control-character restrictions. Reject any unrecognized method or route with the existing invalid-binding error.Source: Coding guidelines
apps/planning-service/src/main.ts (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win라우트 데코레이터와 서명 경로가 각각 따로 선언되어 어긋날 수 있습니다.
각 핸들러는
@Get('today/:date')같은 데코레이터와'/v1/today/${date}'같은 문자열 리터럴을 별도로 유지합니다. 두 값은 컴파일러가 연결하지 않습니다. 라우트 경로를 나중에 변경하면 서명 바인딩이 조용히 어긋납니다. 그 결과는 컴파일 오류가 아니라 런타임 401입니다.라우트별 경로 상수를 한 곳에 정의하고, 데코레이터와 바인딩이 같은 상수를 사용하도록 정리해 주세요. 반복되는
process.env.PLANNING_GATEWAY_CONTEXT_SECRET인자도 같은 헬퍼로 모을 수 있습니다.Also applies to: 138-138, 172-172
🤖 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/planning-service/src/main.ts` at line 113, Centralize each route path in shared route constants and update the corresponding controller decorators and signature-binding entries in main.ts to reuse those constants, including the routes near the GET /v1/search entries and the additional affected bindings. Extract the repeated process.env.PLANNING_GATEWAY_CONTEXT_SECRET argument into a shared helper used by all route bindings, preserving existing authorization behavior.
🤖 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/planning-service/src/http-boundary.test.ts`:
- Around line 11-14: Extend the tests around requirePlanningRequestBinding to
cover rejection of unsupported methods, non-string paths, paths longer than 256
characters, paths containing control characters, ?, or #, and nested-path
identifiers that are not UUIDv4 values. Follow the existing patterns in
planning-controller-authority.test.ts and assert each invalid binding is
rejected.
In `@apps/planning-service/src/main.ts`:
- Line 205: Gateway Today의 today-composition 요청 서명 로직에서 life-os.workspace.v1
바인딩을 life-os.planning-context.v2로 변경하세요. /v1/today/${safeDate} 요청의 HTTP 메서드와 경로가
Planning service 검증값 및 Web Today와 동일하게 서명되도록 업데이트하고, 관련 서명 설정만 수정하세요.
---
Nitpick comments:
In `@apps/planning-service/src/main.ts`:
- Line 113: Centralize each route path in shared route constants and update the
corresponding controller decorators and signature-binding entries in main.ts to
reuse those constants, including the routes near the GET /v1/search entries and
the additional affected bindings. Extract the repeated
process.env.PLANNING_GATEWAY_CONTEXT_SECRET argument into a shared helper used
by all route bindings, preserving existing authorization behavior.
In `@apps/planning-service/src/request-bound-workspace-context.test.ts`:
- Around line 23-37: Update expectInvalid so the sentinel Error is thrown
outside the try/catch, preventing it from being caught and reported as an
HttpException assertion failure. Keep operation() inside the try block and
preserve the existing status and response assertions for errors it actually
throws.
In `@apps/web/app/planning-search-client.ts`:
- Around line 165-177: Update requirePlanningBinding to validate binding.method
at runtime, accepting only the methods supported by the Planning client rather
than relying on the TypeScript type. Replace the broad /v1/ prefix check with
validation against the exact server-supported method/path combinations, keeping
the existing length and control-character restrictions. Reject any unrecognized
method or route with the existing invalid-binding error.
🪄 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: f90f1872-bc80-414e-aede-afe154f71c3a
📒 Files selected for processing (7)
apps/planning-service/src/http-boundary.test.tsapps/planning-service/src/http-boundary.tsapps/planning-service/src/main.tsapps/planning-service/src/planning-controller-authority.test.tsapps/planning-service/src/request-bound-workspace-context.test.tsapps/web/app/planning-search-client.tsapps/web/app/today-sync-client.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/today-composition.ts (1)
370-375: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
workspaceContextHeaders의 서명 계약을 문서화하십시오.이 함수는
planningBinding유무에 따라 legacy workspace v1 서명 또는 request-bound Planning v2 서명을 생성합니다. 새 기여자가 이 fallback 계약과 반환 헤더를 구현 코드 없이 이해할 수 있도록 TSDoc을 추가하십시오.제안 수정
+/** + * Creates trusted workspace headers for an upstream service request. + * + * Uses the Planning v2 request-bound signature when `planningBinding` exists. + * Uses the workspace v1 signature for services that do not use Planning bindings. + */ function workspaceContextHeaders(As per coding guidelines, "Production declarations must include explanatory docstrings sufficient for a new contributor to understand the contract without reconstructing the implementation."
🤖 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/gateway/src/today-composition.ts` around lines 370 - 375, 함수 workspaceContextHeaders의 선언부에 TSDoc을 추가하여 planningBinding이 없으면 legacy workspace v1 서명을, 있으면 요청의 method와 path에 바인딩된 Planning v2 서명을 생성한다는 fallback 계약을 설명하십시오. 또한 반환값이 서명 검증에 필요한 workspace 컨텍스트 헤더를 담은 읽기 전용 레코드임을 문서화하고, 각 매개변수와 선택적 planningBinding의 의미를 명시하십시오.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.
Outside diff comments:
In `@apps/gateway/src/today-composition.ts`:
- Around line 370-375: 함수 workspaceContextHeaders의 선언부에 TSDoc을 추가하여
planningBinding이 없으면 legacy workspace v1 서명을, 있으면 요청의 method와 path에 바인딩된
Planning v2 서명을 생성한다는 fallback 계약을 설명하십시오. 또한 반환값이 서명 검증에 필요한 workspace 컨텍스트 헤더를
담은 읽기 전용 레코드임을 문서화하고, 각 매개변수와 선택적 planningBinding의 의미를 명시하십시오.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: beec6947-a344-4cb9-9364-c8908a9b010c
📒 Files selected for processing (4)
apps/gateway/src/today-composition.test.tsapps/gateway/src/today-composition.tsapps/planning-service/src/http-boundary.test.tsapps/planning-service/src/request-bound-workspace-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/planning-service/src/request-bound-workspace-context.test.ts
2 participants
Security outcome
Planning's short-lived signed workspace context currently authenticates only workspace + timestamp, so a captured fresh context can be replayed across Planning HTTP methods and resource paths that share the same secret. This branch closes that authority-confusion boundary by binding signatures to the exact Planning method and resource path.
Test-first evidence
The first commit adds focused RED coverage proving that a Planning search signature must not authorize
POST /v1/goalsor a different resource path. The implementation will then migrate Planning verification plus every in-repository signer to a versioned request-bound context without accepting the legacy context on migrated public routes.Scope
No cross-service persistence, browser-selected workspace authority, gate weakening, or unrelated repository changes.
Summary by CodeRabbit