fix(review): require signed workspace authority - #165
Closed
seonghobae wants to merge 33 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReview Service가 단순 Changes서명된 workspace context 인증
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ReviewController
participant requireTrustedWorkspaceContext
participant ReviewService
Client->>ReviewController: 서명된 workspace context와 리뷰 요청
ReviewController->>requireTrustedWorkspaceContext: context 검증
requireTrustedWorkspaceContext-->>ReviewController: 검증된 workspace ID
ReviewController->>ReviewService: workspace ID와 리뷰 작업 전달
ReviewService-->>ReviewController: 처리 결과 반환
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
![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 (1)
apps/review-service/src/http-boundary.test.ts (1)
49-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win허용된 시간 경계도 테스트하십시오.
현재 테스트는
NOW_SECONDS - 61및NOW_SECONDS + 6만 거부합니다.MAXIMUM_CONTEXT_AGE_SECONDS의 정확히 60초 경계와MAXIMUM_FUTURE_SKEW_SECONDS의 정확히 5초 경계가 허용되는지 검증하십시오.경계값 테스트 예시
+ it.each([ + String(NOW_SECONDS - 60), + String(NOW_SECONDS + 5), + ])('accepts a context at an allowed time boundary', (issuedAt) => { + expect( + requireTrustedWorkspaceContext( + { workspaceId: WORKSPACE_ID, issuedAt, signature: signature(issuedAt) }, + SECRET, + NOW_SECONDS, + ), + ).toBe(WORKSPACE_ID); + });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/review-service/src/http-boundary.test.ts` around lines 49 - 89, Extend the parameterized cases in the HTTP boundary test to cover the exact allowed limits: issuedAt at NOW_SECONDS - 60 and NOW_SECONDS + 5 must be accepted with a successful response and valid gateway-context result. Keep the existing rejection cases for -61 and +6, and reuse the normal SECRET and signature setup so the tests validate the real boundary behavior.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/review-service/src/http-boundary.ts`:
- Around line 48-54: Add explanatory production-contract docstrings at
apps/review-service/src/http-boundary.ts:48-54 for invalidGatewayContext’s 401
status and safe error contract, at
apps/review-service/src/http-boundary.ts:56-62 for unavailableGatewayContext’s
HTTP status and configuration-failure meaning, and at
apps/review-service/src/http-boundary.ts:64-72 for workspaceContextDigest’s
canonical payload and digest format; also document at
apps/review-service/src/main.ts:111-117 that complete accepts only workspace IDs
that passed requireTrustedWorkspaceContext.
---
Nitpick comments:
In `@apps/review-service/src/http-boundary.test.ts`:
- Around line 49-89: Extend the parameterized cases in the HTTP boundary test to
cover the exact allowed limits: issuedAt at NOW_SECONDS - 60 and NOW_SECONDS + 5
must be accepted with a successful response and valid gateway-context result.
Keep the existing rejection cases for -61 and +6, and reuse the normal SECRET
and signature setup so the tests validate the real boundary behavior.
🪄 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: dd4ae884-8179-4d8a-b3e4-2971c43445a7
📒 Files selected for processing (5)
apps/review-service/src/http-boundary.test.tsapps/review-service/src/http-boundary.tsapps/review-service/src/main.test.tsapps/review-service/src/main.tsapps/review-service/src/review-controller-authority.test.ts
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 `@apps/review-service/src/review-controller-authority.test.ts`:
- Around line 152-158: Update the RouteCase definitions and success-path loop in
the review-controller authority tests to include realistic request bodies,
complete expected service arguments, and expected return values for each route.
Assert the exact arguments passed through route.invoke—including complete’s
ritualKind/body and list’s limit—and verify the returned value, while preserving
the existing call-count assertion.
- Around line 175-180: Extend the invalidContexts cases in the review-controller
authority tests to cover contexts missing issuedAt and signature, asserting each
returns 401 and does not invoke the domain service. Keep the existing
missing-workspaceId, expiration, tampering, malformed, and unconfigured-secret
cases unchanged.
🪄 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: 7f29f20c-66e0-404b-9da0-3a67806bd050
📒 Files selected for processing (2)
apps/review-service/src/http-boundary.test.tsapps/review-service/src/review-controller-authority.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/review-service/src/http-boundary.test.ts
This was referenced Aug 10, 2026
Contributor
Author
This was referenced Aug 10, 2026
Contributor
Author
|
Superseded by clean current-main successor #178. The successor preserves the exact five Review authority paths from this branch, starts from current protected main, and requires fresh exact-head CI/security/review evidence; no predecessor checks or approvals transfer. |
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
2 participants
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
Remove the remaining Review API trust in a browser-selectable
x-workspace-id. All workspace-scoped guided-review completion/history routes now require the short-lived signedlife-os.workspace.v1context before tenant identity reaches domain or persistence code.Test-first sequence
The first commits add RED contracts before production code:
@Headers('x-workspace-id')andrequireWorkspaceHeader, and requires all four workspace-scoped route families to bind the signed workspace/issued-at/signature headers.Implementation
requireTrustedWorkspaceContextvalidates the existing UUIDv4 workspace invariant, canonical Unix seconds, 43-character base64url SHA-256 HMAC, minimum 32-byte verifier secret, 60-second maximum age, 5-second future skew, and constant-time digest comparison;x-life-os-workspace-id,x-life-os-context-issued-at, andx-life-os-context-signaturewithREVIEW_GATEWAY_CONTEXT_SECRET;ReviewServiceand public errors remain bounded and credential-free.Scope
This is a service-boundary hardening slice. It does not add gateway routing, cross-service database access, user-level review authorization beyond the existing workspace boundary, or change review ritual/scoring semantics. The public gateway must mint this context only after authenticated workspace resolution when Review is composed into the hosted path.
Verification
Merge only after the unchanged exact head passes Review tests/typecheck/build, repository CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, current review findings, and current-live-base compatibility without administrative bypass.
Summary by CodeRabbit