fix(planning): require signed workspace authority on all routes [superseded by #168] - #160
Closed
seonghobae wants to merge 7 commits into
Closed
fix(planning): require signed workspace authority on all routes [superseded by #168]#160seonghobae wants to merge 7 commits into
seonghobae wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPlanning 목표·프로젝트·작업 라우트가 클라이언트 지정 ChangesPlanning workspace 권한 강화
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlanningController
participant requireTrustedWorkspaceContext
participant PlanningService
Client->>PlanningController: 서명된 workspace 컨텍스트 헤더 전송
PlanningController->>requireTrustedWorkspaceContext: 컨텍스트 검증 요청
requireTrustedWorkspaceContext-->>PlanningController: 검증된 workspace ID 반환
PlanningController->>PlanningService: workspace ID와 요청 데이터 전달
PlanningService-->>PlanningController: 리소스 결과 반환
PlanningController-->>Client: HTTP 응답 반환
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
![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
🤖 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/planning-controller-authority.test.ts`:
- Around line 20-32: Extend the tests around the PlanningController HTTP
boundary instead of only counting source tokens. Exercise every workspace-scoped
route with malformed, stale, future, forged, and unconfigured signed contexts,
asserting rejection status and response bodies while confirming no unverified
workspace ID reaches the service. Cover the six mutating Goal/Project/Task
routes to verify the signed workspace context overrides workspace identifiers
supplied in the request body or path.
🪄 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: dcb2c2be-e850-4d03-8534-b0db3e355617
📒 Files selected for processing (4)
CHANGELOG.mdapps/planning-service/src/main.tsapps/planning-service/src/planning-controller-authority.test.tsdocs/research/2026-08-10-planning-workspace-authority.md
Comment on lines
+20
to
+32
| describe('PlanningController workspace authority contract', () => { | ||
| it('never accepts a bare client-selected workspace header', () => { | ||
| expect(count(LEGACY_WORKSPACE_HEADER)).toBe(0); | ||
| expect(CONTROLLER_SOURCE).not.toContain('function requireWorkspaceId'); | ||
| }); | ||
|
|
||
| it('binds every workspace-scoped planning route to the signed workspace context', () => { | ||
| // search + Today GET/PUT + six Goal/Project/Task routes. | ||
| expect(count(TRUSTED_WORKSPACE_HEADER)).toBe(9); | ||
| expect(count(TRUSTED_ISSUED_AT_HEADER)).toBe(9); | ||
| expect(count(TRUSTED_SIGNATURE_HEADER)).toBe(9); | ||
| expect(CONTROLLER_SOURCE.match(/requireTrustedWorkspaceContext\(/gu)).toHaveLength(9); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
HTTP 경계 실패 동작을 검증하는 테스트를 추가하십시오.
이 테스트는 main.ts의 토큰 수만 검사합니다. 이 테스트는 malformed, stale, future, forged, unconfigured context가 각 라우트에서 거부되는지 증명하지 않습니다. 이 테스트는 검증된 workspace ID만 서비스 호출에 전달되는지도 증명하지 않습니다.
실제 컨트롤러 또는 HTTP 경계 테스트를 추가하십시오. 각 실패 유형의 credential-free 상태와 응답 본문을 검증하십시오. 여섯 개 변경 라우트에서 서명된 context가 body 또는 path ID보다 workspace ownership에 우선하는지도 검증하십시오.
As per coding guidelines, “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/planning-service/src/planning-controller-authority.test.ts` around lines
20 - 32, Extend the tests around the PlanningController HTTP boundary instead of
only counting source tokens. Exercise every workspace-scoped route with
malformed, stale, future, forged, and unconfigured signed contexts, asserting
rejection status and response bodies while confirming no unverified workspace ID
reaches the service. Cover the six mutating Goal/Project/Task routes to verify
the signed workspace context overrides workspace identifiers supplied in the
request body or path.
Source: Coding guidelines
seonghobae
changed the title
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security outcome
Close the remaining client-selected planning tenant boundary identified in #158. Goal, project and task create/list routes now derive workspace ownership from the same short-lived signed
life-os.workspace.v1context already used by planning search and durable Today.Test-first contract
A new controller-source contract was committed first and fails on protected main because six routes still bind
@Headers('x-workspace-id')andrequireWorkspaceId(). It requires:x-workspace-iddecorators;requireWorkspaceIdhelper;x-life-os-workspace-id, issued-at, signature and callrequireTrustedWorkspaceContext.Existing
http-boundary.test.tsindependently exercises fresh, stale, future, malformed, forged and unconfigured signed context plus sanitized HTTP failure mapping.Implementation
BadRequestExceptionand the legacy bare-workspace helper;Standards / doctoring
docs/research/2026-08-10-planning-workspace-authority.mdrecords the final NIST SP 800-53 Rev. 5 basis and explicitly avoids a conformance claim.CHANGELOG.mdrecords the security boundary change.Merge gate
Merge only after the unchanged exact head passes planning tests/typecheck/build, configured exact owned-code coverage/docstrings, CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, CodeRabbit/current review findings, current live-base compatibility and actual repository policy without administrative bypass.
Closes #158.
Summary by CodeRabbit
보안 강화
새 기능
버그 수정
문서화