fix(review): require signed workspace authority on clean current main - #185
Merged
seonghobae merged 28 commits intoAug 11, 2026
Merged
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 서비스가 요청 method와 path에 바인딩된 HMAC workspace context를 검증하도록 변경되었습니다. Secret 설정 검증과 Changes서명된 workspace context 인증
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewClient
participant ReviewController
participant requireTrustedWorkspaceContext
participant ReviewService
ReviewClient->>ReviewController: 서명된 workspace context와 요청 전송
ReviewController->>requireTrustedWorkspaceContext: 헤더, secret, method/path 검증
requireTrustedWorkspaceContext-->>ReviewController: 검증된 workspace ID 반환
ReviewController->>ReviewService: workspace ID와 도메인 요청 전달
ReviewService-->>ReviewController: 결과 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (4)
apps/review-service/src/http-boundary.ts (1)
105-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value검증기 시계 오류를 401로 분류하지 마십시오.
nowSeconds가 안전한 정수가 아니거나 음수인 경우는 서버 측 시계 결함입니다. 요청자의 자격 증명 문제가 아닙니다. 현재 코드는 이 경우에도 401invalid_gateway_context를 반환합니다.requireReviewGatewayContextSecret의 503 처리와 일관되게, 시계 결함은unavailableGatewayContext()로 분류하십시오. 이 변경은http-boundary.test.ts의non-integer verifier clock,negative verifier clock사례 기대값도 함께 수정해야 합니다.♻️ 시계 검증을 분리하는 예시
const verifiedSecret = requireReviewGatewayContextSecret(secret); + if (!Number.isSafeInteger(nowSeconds) || nowSeconds < 0) { + return unavailableGatewayContext(); + } if ( typeof headers.workspaceId !== 'string' || typeof headers.issuedAt !== 'string' || typeof headers.signature !== 'string' || !UNIX_SECONDS_PATTERN.test(headers.issuedAt) || - !BASE64URL_SHA256_PATTERN.test(headers.signature) || - !Number.isSafeInteger(nowSeconds) || - nowSeconds < 0 + !BASE64URL_SHA256_PATTERN.test(headers.signature) ) { return invalidGatewayContext(); }🤖 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.ts` around lines 105 - 115, Separate the verifier-clock validation from the credential/header validation in the request context flow. When nowSeconds is not a safe integer or is negative, return unavailableGatewayContext() instead of invalidGatewayContext(), consistent with requireReviewGatewayContextSecret; update the non-integer verifier clock and negative verifier clock expectations in http-boundary.test.ts accordingly.apps/review-service/src/http-boundary.test.ts (1)
134-344: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win다른 workspace로 서명된 컨텍스트 사례를 추가하십시오.
현재 실패 사례는 위조 서명, 만료, 형식 오류를 다룹니다. 그러나 유효한 secret으로 다른 workspace ID를 서명한 뒤 헤더의 workspace ID만 교체하는 교차 테넌트 치환 사례는 없습니다.
signature헬퍼는 이미workspaceId인자를 받으므로 사례 추가 비용이 낮습니다. 이 사례는 서명이 workspace ID에 결합되어 있음을 증명합니다.💚 교차 테넌트 치환 사례 추가 예시
{ name: 'signature bound to another workspace', headers: { workspaceId: WORKSPACE_ID, issuedAt: String(NOW_SECONDS), signature: signature( String(NOW_SECONDS), '018f47b2-c1d2-4a30-8c17-221fb579c043', ), }, secret: SECRET, nowSeconds: NOW_SECONDS, status: 401, code: 'invalid_gateway_context', },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 134 - 344, Extend the “fails closed for $name” table with a cross-workspace substitution case: generate the signature using a different valid workspace ID via the existing signature helper, then send that signature with WORKSPACE_ID in the headers. Keep SECRET and NOW_SECONDS valid, and assert the request returns 401 with invalid_gateway_context.Source: Coding guidelines
apps/review-service/src/review-controller-authority.test.ts (1)
128-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win소스 문자열 검사 대신 동작 검증을 사용하십시오.
이 테스트는
main.ts의 텍스트에서 특정 문자열이 없는지만 확인합니다. 이 방식은 회귀를 신뢰성 있게 잡지 못합니다. 헬퍼 이름이 바뀌거나 주석에 동일한 문자열이 남으면 결과가 잘못됩니다. 또한 legacy 헤더를 다시 도입하면서 이름만 바꾸면 통과합니다. 같은 파일의 lines 149-194는 이미 동작 수준에서 거부를 증명합니다. 텍스트 검사를 제거하고, 대신 서명 없이 workspace 헤더만 전달할 때 모든 라우트가 401로 거부하는지 확인하는 사례를 추가하십시오.💚 동작 기반 대체 예시
- it('rejects legacy browser-selectable workspace authority', () => { - expect(controllerSource).not.toContain("`@Headers`('x-workspace-id')"); - expect(controllerSource).not.toContain('requireWorkspaceHeader('); - }); + it('rejects a browser-selected workspace header without signed context', async () => { + process.env.REVIEW_GATEWAY_CONTEXT_SECRET = CONTEXT_SECRET; + const service = serviceSpies(); + const controller = controllerWith(service); + const headers: RouteHeaders = { + workspaceId: WORKSPACE_ID, + issuedAt: undefined, + signature: undefined, + }; + + for (const route of ROUTES) { + vi.clearAllMocks(); + expect(await rejectedStatus(route.invoke(controller, headers))).toBe(401); + expect(service[route.serviceMethod]).not.toHaveBeenCalled(); + } + });이 대체를 적용하면 lines 2-3의
readFileSync,resolveimport와 line 9의controllerSource도 제거할 수 있습니다.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/review-controller-authority.test.ts` around lines 128 - 131, Remove the source-text assertions and the associated readFileSync, resolve imports and controllerSource setup. Replace the legacy-authority test with an integration-style case that sends only the workspace header, without a signature, to every relevant route and verifies each responds with 401, reusing the existing request setup and behavior-level patterns in the tests around lines 149-194.Source: Coding guidelines
apps/review-service/src/main.ts (1)
54-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win컨텍스트 검증 블록의 중복을 제거하십시오.
네 개 라우트가 동일한 4행 블록을 반복합니다. 각 라우트가
process.env.REVIEW_GATEWAY_CONTEXT_SECRET을 직접 읽습니다. 이 구조에서는 secret 출처나 검증 방식을 바꿀 때 네 곳을 모두 수정해야 하며, 한 곳을 놓치면 인증이 누락됩니다. 검증을 private 헬퍼로 추출하거나, NestJS guard 또는 주입된 설정 객체로 옮기십시오.♻️ private 헬퍼로 추출하는 예시
+ /** Returns the workspace ID only after the signed gateway context verifies. */ + private trustedWorkspaceId(headers: { + workspaceId: string | undefined; + issuedAt: string | undefined; + signature: string | undefined; + }): string { + return requireTrustedWorkspaceContext( + headers, + process.env.REVIEW_GATEWAY_CONTEXT_SECRET, + ); + } + /** Records a completed daily planning ritual. */ `@Post`('reviews/daily-planning/completions') async completeDailyPlanning( `@Headers`('x-life-os-workspace-id') workspaceId: string | undefined, `@Headers`('x-life-os-context-issued-at') issuedAt: string | undefined, `@Headers`('x-life-os-context-signature') signature: string | undefined, `@Body`() body: unknown, ): Promise<ReviewCompletionRecord> { - const trustedWorkspaceId = requireTrustedWorkspaceContext( - { workspaceId, issuedAt, signature }, - process.env.REVIEW_GATEWAY_CONTEXT_SECRET, - ); + const trustedWorkspaceId = this.trustedWorkspaceId({ + workspaceId, + issuedAt, + signature, + }); return await this.complete(trustedWorkspaceId, 'daily-planning', body); }🤖 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/main.ts` around lines 54 - 119, 중복된 workspace 컨텍스트 검증을 private 헬퍼로 추출하고, completeDailyPlanning, completeDailyShutdown, completeWeeklyReview, listCompletions가 해당 헬퍼를 사용하도록 변경하십시오. 헬퍼에서 헤더 값과 REVIEW_GATEWAY_CONTEXT_SECRET을 한 곳에서 전달해 requireTrustedWorkspaceContext를 호출하고 검증된 workspace ID를 반환하도록 하여, 각 라우트가 secret을 직접 읽지 않게 하십시오.
🤖 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 99-146: Verify whether the gateway minting flow and request
validation bind the trusted workspace signature to the HTTP method and path and
enforce nonce or one-time replay protection. If not, update
requireTrustedWorkspaceContext and the corresponding signing/verification
helpers to include method, path, and a nonce or equivalent one-time identifier
in the signed payload, rejecting reused contexts while preserving existing
workspace, timestamp, and signature validation.
In `@apps/review-service/src/main.ts`:
- Around line 152-161: The Review service boundary change requires coordinated
documentation and deployment updates. Update the architecture and design
documentation, agent guidance, implementation plans, operating runbooks,
capability evidence, and the Unreleased CHANGELOG to describe the required
signed headers and REVIEW_GATEWAY_CONTEXT_SECRET; update deployment
configuration and environment templates to provision the secret, and document
rollout ordering so the gateway issues the new headers before Review traffic is
sent.
---
Nitpick comments:
In `@apps/review-service/src/http-boundary.test.ts`:
- Around line 134-344: Extend the “fails closed for $name” table with a
cross-workspace substitution case: generate the signature using a different
valid workspace ID via the existing signature helper, then send that signature
with WORKSPACE_ID in the headers. Keep SECRET and NOW_SECONDS valid, and assert
the request returns 401 with invalid_gateway_context.
In `@apps/review-service/src/http-boundary.ts`:
- Around line 105-115: Separate the verifier-clock validation from the
credential/header validation in the request context flow. When nowSeconds is not
a safe integer or is negative, return unavailableGatewayContext() instead of
invalidGatewayContext(), consistent with requireReviewGatewayContextSecret;
update the non-integer verifier clock and negative verifier clock expectations
in http-boundary.test.ts accordingly.
In `@apps/review-service/src/main.ts`:
- Around line 54-119: 중복된 workspace 컨텍스트 검증을 private 헬퍼로 추출하고,
completeDailyPlanning, completeDailyShutdown, completeWeeklyReview,
listCompletions가 해당 헬퍼를 사용하도록 변경하십시오. 헬퍼에서 헤더 값과 REVIEW_GATEWAY_CONTEXT_SECRET을
한 곳에서 전달해 requireTrustedWorkspaceContext를 호출하고 검증된 workspace ID를 반환하도록 하여, 각
라우트가 secret을 직접 읽지 않게 하십시오.
In `@apps/review-service/src/review-controller-authority.test.ts`:
- Around line 128-131: Remove the source-text assertions and the associated
readFileSync, resolve imports and controllerSource setup. Replace the
legacy-authority test with an integration-style case that sends only the
workspace header, without a signature, to every relevant route and verifies each
responds with 401, reusing the existing request setup and behavior-level
patterns in the tests around lines 149-194.
🪄 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: c4fe40af-0087-4afa-9115-bd1a6d84df6c
📒 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.
🧹 Nitpick comments (2)
apps/review-service/src/http-boundary.test.ts (2)
94-124: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick wincompletion 경로 간 재사용 테스트를 추가하십시오.
현재 재사용 거부 테스트는 GET history 서명을 POST completion에 사용하는 경우만 다룹니다. 세 개의 POST completion 경로는 method와 secret이 동일하므로, 경로만 다른 서명의 재사용이 가장 중요한 경계입니다.
daily-planning서명을weekly-reviewbinding으로 검증하는 케이스를 추가하십시오.💚 추가 테스트 예시
+ it('rejects replaying one completion signature on another completion path', () => { + const issuedAt = String(NOW_SECONDS); + expectTrustedContextRejection( + { + workspaceId: WORKSPACE_ID, + issuedAt, + signature: signature(issuedAt, DAILY_PLANNING_BINDING), + }, + SECRET, + { method: 'POST', path: '/v1/reviews/weekly-review/completions' }, + NOW_SECONDS, + 401, + 'invalid_gateway_context', + ); + });🤖 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 94 - 124, Extend the completion-binding tests around requireTrustedWorkspaceContext to cover cross-path reuse: create a signature with DAILY_PLANNING_BINDING, validate it using the weekly-review binding, and assert the same 401 invalid_gateway_context rejection as the existing history-signature case.
36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win서명 헬퍼가 세 개의 테스트 파일에 중복되어 있습니다.
life-os.review-context.v1페이로드 형식이http-boundary.test.ts,main.test.ts:36-47,review-controller-authority.test.ts:36-47에 동일하게 복제되어 있습니다. 컨텍스트 버전이나 필드 순서가 바뀌면 세 곳을 모두 수정해야 하며, 일부만 갱신하면 테스트가 잘못된 형식을 계속 통과시킵니다. 공용 테스트 헬퍼 모듈로 추출하십시오.🤖 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 36 - 47, Extract the duplicated signature helper and its shared payload construction into a common test utility module, then update signature usages in http-boundary.test.ts, main.test.ts, and review-controller-authority.test.ts to import and reuse it. Preserve the existing HMAC inputs, field order, defaults, and return format through the shared helper.
🤖 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.
Nitpick comments:
In `@apps/review-service/src/http-boundary.test.ts`:
- Around line 94-124: Extend the completion-binding tests around
requireTrustedWorkspaceContext to cover cross-path reuse: create a signature
with DAILY_PLANNING_BINDING, validate it using the weekly-review binding, and
assert the same 401 invalid_gateway_context rejection as the existing
history-signature case.
- Around line 36-47: Extract the duplicated signature helper and its shared
payload construction into a common test utility module, then update signature
usages in http-boundary.test.ts, main.test.ts, and
review-controller-authority.test.ts to import and reuse it. Preserve the
existing HMAC inputs, field order, defaults, and return format through the
shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 559e0ba2-c432-43ae-8607-159602b225ab
📒 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
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/review-service/src/review-controller-authority.test.ts
- apps/review-service/src/main.test.ts
- apps/review-service/src/main.ts
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/review-service/package.json`:
- Line 8: Update the package.json lint script so validation covers every file
formatted by its Prettier command, including package.json, migrations/README.md,
src/**/*.ts, and the plan document. Replace the narrow git diff path list with
matching whole-target validation, preferably using Prettier check semantics,
while preserving the existing TypeScript check.
🪄 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: 2ad2e48b-9645-46e6-960f-7c6c02ecae00
📒 Files selected for processing (1)
apps/review-service/package.json
Contributor
Author
|
@coderabbitai review |
|
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
Reconstruct the exact Review signed-workspace-authority change from current protected main after #178 acquired GitHub Actions-authored merge head
d153a7009a6e8a5f55fcfcdf8e9ef7fc79a8567a. Every pull-request workflow for that exact head completedaction_requiredwith zero jobs, so the head cannot supply valid CI/security evidence.This successor starts directly from protected main
3e3c8b02a0a757e16cb7dee3ada48edf865e0577and applies exactly the five Review file blobs from #178. No prior check, review, approval, or mergeability evidence transfers.Preserved scope
apps/review-service/src/http-boundary.tsapps/review-service/src/http-boundary.test.tsapps/review-service/src/main.tsapps/review-service/src/main.test.tsapps/review-service/src/review-controller-authority.test.tsAll workspace-scoped Review completion/history routes require short-lived signed
life-os.workspace.v1context before tenant identity reaches domain or persistence code. Bare browser-selected workspace headers are not authority. Invalid deployment secret configuration remains fail-closed at startup/readiness and invalid request context remains fail-closed at the HTTP boundary.Preservation proof
The branch is one commit ahead of live main and its main-relative diff is exactly the same five files and line statistics as #178. Source blobs were copied directly from #178 exact head; the reconstruction intentionally excludes its GitHub Actions-authored merge ancestry.
Merge gate
Require this unchanged exact head to pass Review tests/typecheck/build, CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, current CodeRabbit/review findings, and current-live-base compatibility under live repository policy. No predecessor evidence transfers and no administrative bypass is acceptable.
Supersedes #178 only after this successor is proven to run normally.
Summary by CodeRabbit
보안 강화
운영 개선
버그 수정