fix(review): require signed workspace authority on current main - #178
Closed
seonghobae wants to merge 43 commits into
Closed
fix(review): require signed workspace authority on current main#178seonghobae wants to merge 43 commits into
seonghobae wants to merge 43 commits into
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:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughReview 서비스가 클라이언트 workspace 헤더 대신 HMAC 서명 컨텍스트를 검증한다. 완료 기록 및 조회 라우트는 검증된 workspace ID를 사용한다. 검증 실패는 401 또는 503으로 반환된다. ChangesReview workspace 권한
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ReviewController
participant requireTrustedWorkspaceContext
participant ReviewService
Client->>ReviewController: 서명된 workspace 컨텍스트 전송
ReviewController->>requireTrustedWorkspaceContext: 헤더와 gateway secret 검증
requireTrustedWorkspaceContext-->>ReviewController: 검증된 workspace ID 반환
ReviewController->>ReviewService: 검증된 workspace ID로 요청 처리
ReviewService-->>ReviewController: 처리 결과 반환
ReviewController-->>Client: HTTP 응답 반환
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 |
![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 (7)
apps/review-service/src/main.ts (2)
52-55: 🩺 Stability & Availability | 🔵 Trivial부팅 시점에
REVIEW_GATEWAY_CONTEXT_SECRET검증을 추가하는 것을 권장합니다.현재는 요청마다 secret을 읽고, 미설정 시 요청 단위로 503을 반환합니다. 동작은 fail-closed입니다. 다만 잘못 배포된 인스턴스는 트래픽을 받은 뒤에야 문제를 드러냅니다. 부팅 시 secret 존재와 최소 길이를 검사하고 readiness 프로브에 반영하면 배포 단계에서 즉시 감지할 수 있습니다.
🤖 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 52 - 55, Validate REVIEW_GATEWAY_CONTEXT_SECRET during application startup, including its presence and minimum required length, before serving requests. Integrate this validation with the readiness probe so an invalid configuration keeps the instance unready and exposes the deployment error immediately, while preserving the existing requireTrustedWorkspaceContext request validation.
47-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff네 개 라우트의 컨텍스트 검증 블록이 동일하게 반복됩니다.
각 라우트가 헤더 데코레이터 3개와
requireTrustedWorkspaceContext호출을 그대로 복제합니다. NestJS 커스텀 파라미터 데코레이터 또는 가드로 추출하면 중복이 사라지고, 새 라우트에서 검증 누락이 발생할 위험도 줄어듭니다.주의:
apps/review-service/src/review-controller-authority.test.ts의 Line 126-144는main.ts소스 문자열에서 데코레이터와 호출이 각각 4회 나타나는지 검사합니다. 추출 리팩터링을 진행하면 해당 테스트도 함께 갱신해야 합니다.🤖 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 47 - 109, Extract the repeated workspace-context header handling and requireTrustedWorkspaceContext call from the four route methods in the review controller into a shared NestJS custom parameter decorator or guard, then update each route to consume the trusted workspace ID through that abstraction while preserving existing validation and error behavior. Update review-controller-authority.test.ts to assert the refactored source structure instead of expecting four duplicated decorator and call occurrences.apps/review-service/src/http-boundary.ts (1)
66-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value서명 payload가 정규화된 workspace ID를 사용한다는 점을 문서화하십시오.
workspaceContextDigest는requireReviewWorkspaceId가 소문자로 정규화한 값을 서명 대상으로 사용합니다. 헤더 원본 값은 사용하지 않습니다. 게이트웨이가 헤더 원본 문자열로 서명하면 대소문자가 다를 때 검증이 실패합니다. 이 계약을 docstring에 명시하십시오.📝 제안 diff
-/** Computes the SHA-256 HMAC over the canonical `life-os.workspace.v1` workspace-and-time payload. */ +/** + * Computes the SHA-256 HMAC over the canonical `life-os.workspace.v1` payload. + * The workspace ID must already be normalized to lowercase UUIDv4 form, + * so the gateway must sign the normalized identifier, not the raw header value. + */ function workspaceContextDigest(🤖 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 66 - 75, Update the docstring for workspaceContextDigest to state that the signed payload uses the lowercase-normalized workspace ID returned by requireReviewWorkspaceId, not the original header value, and that signing must use this normalized value consistently for verification.apps/review-service/src/review-controller-authority.test.ts (1)
126-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win소스 문자열 단언은 리팩터링에 취약합니다.
Line 129-140은
main.ts의 텍스트에서 데코레이터와 호출 횟수를 셉니다. 이 단언은 동작이 아니라 소스 형태를 검증합니다. 라우트 검증을 가드나 커스텀 파라미터 데코레이터로 추출하면 동작이 그대로여도 테스트가 실패합니다.Line 146-204의 동작 기반 테스트는 이미 네 라우트 모두에서 검증 통과와 실패를 증명합니다. Line 127-128의 회귀 방지 단언(
x-workspace-id,requireWorkspaceHeader미존재)만 남기고 횟수 단언은 제거하는 방안을 검토하십시오.테스트가 모의 호출 횟수가 아니라 실제 도메인 결과와 실패 동작을 증명해야 한다는 코딩 가이드라인에 따릅니다.
🤖 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 126 - 144, In the test case “rejects browser-selectable workspace authority on every review route,” remove the source-text assertions that count decorator occurrences, requireTrustedWorkspaceContext calls, and REVIEW_GATEWAY_CONTEXT_SECRET references. Preserve only the regression assertions rejecting x-workspace-id and requireWorkspaceHeader, relying on the existing behavior-based tests for all four routes.Source: Coding guidelines
apps/review-service/src/main.test.ts (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value테스트 secret 생성 방식을 다른 테스트와 통일하는 것을 고려하십시오.
apps/review-service/src/http-boundary.test.ts와apps/review-service/src/review-controller-authority.test.ts는randomBytes(32).toString('base64url')을 사용합니다. 이 파일만 고정 문자열을 조합합니다. 동일한 방식을 사용하면 최소 길이 요건 충족이 명확해집니다.🤖 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.test.ts` around lines 14 - 21, Update the test secret setup around GATEWAY_SECRET in main.test.ts to generate the value using randomBytes(32).toString('base64url'), matching http-boundary.test.ts and review-controller-authority.test.ts, and remove the fixed string composition while preserving previousGatewaySecret handling.apps/review-service/src/http-boundary.test.ts (2)
123-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value실패 단언을
expect(...).toThrow형태로 바꾸는 것을 고려하십시오.현재 sentinel
throw new Error('expected trusted context rejection')은 같은try블록 안에 있습니다. 검증이 통과하면 이 Error가 아래catch로 잡히고toBeInstanceOf(HttpException)단언이 실패합니다. 결과는 올바르지만 실패 메시지가 원인을 정확히 보여주지 않습니다.🤖 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 123 - 134, Update the stale/future/forged/unverifiable context test around requireTrustedWorkspaceContext to use an expect(...).toThrow-style assertion instead of the sentinel throw and surrounding try/catch. Preserve validation of the thrown HttpException through response(error) so the expected status and code remain asserted with an accurate failure message.
82-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win실패 케이스 표에 헤더 누락과 서명 형식 오류를 추가하십시오.
현재 표는 만료, 미래, 위조, 짧은 secret만 다룹니다.
requireTrustedWorkspaceContext의 다음 분기는 이 파일에서 실행되지 않습니다.
typeof headers.workspaceId !== 'string'등 헤더 누락 분기!BASE64URL_SHA256_PATTERN.test(headers.signature)서명 길이·문자 오류 분기!UNIX_SECONDS_PATTERN.test(headers.issuedAt)비숫자issuedAt분기!Number.isSafeInteger(nowSeconds) || nowSeconds < 0분기또한
issuedAtSeconds의!Number.isSafeInteger(...)검사는UNIX_SECONDS_PATTERN이 최대 13자리를 허용하므로 도달할 수 없습니다. 이 분기는 100% 분기 커버리지 게이트를 충족할 수 없습니다. 해당 검사를 제거하거나, 패턴을 넓혀 도달 가능하게 하십시오.커버리지 게이트를 적용하는 패키지는 100% statement/branch/function/line 커버리지를 유지해야 한다는 코딩 가이드라인에 따릅니다.
🧪 표에 추가할 케이스 예시
{ headers: { workspaceId: WORKSPACE_ID, issuedAt: String(NOW_SECONDS), signature: signature(String(NOW_SECONDS)), }, secret: 'too-short', status: 503, code: 'gateway_context_unavailable', }, + { + headers: { + workspaceId: undefined, + issuedAt: String(NOW_SECONDS), + signature: signature(String(NOW_SECONDS)), + }, + secret: SECRET, + status: 401, + code: 'invalid_gateway_context', + }, + { + headers: { + workspaceId: WORKSPACE_ID, + issuedAt: 'not-a-timestamp', + signature: signature(String(NOW_SECONDS)), + }, + secret: SECRET, + status: 401, + code: 'invalid_gateway_context', + }, + { + headers: { + workspaceId: WORKSPACE_ID, + issuedAt: String(NOW_SECONDS), + signature: 'A'.repeat(42), + }, + secret: SECRET, + status: 401, + code: '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 82 - 134, Update the failure-case table exercising requireTrustedWorkspaceContext to cover missing or non-string workspaceId, malformed signature characters/length, nonnumeric issuedAt, and invalid nowSeconds values, while asserting the appropriate rejection responses. Also make the issuedAtSeconds safe-integer branch reachable by widening UNIX_SECONDS_PATTERN, or remove that redundant check if the existing pattern is intentionally bounded; preserve 100% statement, branch, function, and line coverage.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/review-controller-authority.test.ts`:
- Around line 168-173: Update the tampered signature construction in the review
authorization test to decode fresh.signature as base64url, deterministically
modify one decoded byte, and re-encode it as base64url. Preserve the intent of
producing a same-length invalid HMAC while avoiding changes limited to the final
character’s ignored padding bits.
---
Nitpick comments:
In `@apps/review-service/src/http-boundary.test.ts`:
- Around line 123-134: Update the stale/future/forged/unverifiable context test
around requireTrustedWorkspaceContext to use an expect(...).toThrow-style
assertion instead of the sentinel throw and surrounding try/catch. Preserve
validation of the thrown HttpException through response(error) so the expected
status and code remain asserted with an accurate failure message.
- Around line 82-134: Update the failure-case table exercising
requireTrustedWorkspaceContext to cover missing or non-string workspaceId,
malformed signature characters/length, nonnumeric issuedAt, and invalid
nowSeconds values, while asserting the appropriate rejection responses. Also
make the issuedAtSeconds safe-integer branch reachable by widening
UNIX_SECONDS_PATTERN, or remove that redundant check if the existing pattern is
intentionally bounded; preserve 100% statement, branch, function, and line
coverage.
In `@apps/review-service/src/http-boundary.ts`:
- Around line 66-75: Update the docstring for workspaceContextDigest to state
that the signed payload uses the lowercase-normalized workspace ID returned by
requireReviewWorkspaceId, not the original header value, and that signing must
use this normalized value consistently for verification.
In `@apps/review-service/src/main.test.ts`:
- Around line 14-21: Update the test secret setup around GATEWAY_SECRET in
main.test.ts to generate the value using randomBytes(32).toString('base64url'),
matching http-boundary.test.ts and review-controller-authority.test.ts, and
remove the fixed string composition while preserving previousGatewaySecret
handling.
In `@apps/review-service/src/main.ts`:
- Around line 52-55: Validate REVIEW_GATEWAY_CONTEXT_SECRET during application
startup, including its presence and minimum required length, before serving
requests. Integrate this validation with the readiness probe so an invalid
configuration keeps the instance unready and exposes the deployment error
immediately, while preserving the existing requireTrustedWorkspaceContext
request validation.
- Around line 47-109: Extract the repeated workspace-context header handling and
requireTrustedWorkspaceContext call from the four route methods in the review
controller into a shared NestJS custom parameter decorator or guard, then update
each route to consume the trusted workspace ID through that abstraction while
preserving existing validation and error behavior. Update
review-controller-authority.test.ts to assert the refactored source structure
instead of expecting four duplicated decorator and call occurrences.
In `@apps/review-service/src/review-controller-authority.test.ts`:
- Around line 126-144: In the test case “rejects browser-selectable workspace
authority on every review route,” remove the source-text assertions that count
decorator occurrences, requireTrustedWorkspaceContext calls, and
REVIEW_GATEWAY_CONTEXT_SECRET references. Preserve only the regression
assertions rejecting x-workspace-id and requireWorkspaceHeader, relying on the
existing behavior-based tests for all four routes.
🪄 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: f29fd6a2-36a0-4633-9735-d938aa7c52f5
📒 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: 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/src/main.test.ts`:
- Around line 117-140: Update the unsafe-secret assertions in the test “keeps
startup and readiness fail-closed for unsafe gateway secrets” to verify that
both missing and too-short REVIEW_GATEWAY_CONTEXT_SECRET failures throw an
HttpException with status 503 and the gateway_context_unavailable error.
Preserve the existing valid-secret readiness assertions.
🪄 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: b153e3b6-0213-4bb9-920e-8fd08635d476
📒 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 (1)
- apps/review-service/src/http-boundary.test.ts
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 Review signed-workspace-authority hardening from current protected main after PR #165's branch acquired a GitHub Actions-authored merge commit whose exact head produced only
action_requiredworkflow runs with zero jobs. This replacement avoids carrying that control-plane state forward while preserving the Review semantic changes exactly.All workspace-scoped guided-review completion/history routes require the short-lived signed
life-os.workspace.v1context before tenant identity reaches domain or persistence code. A bare client-selectedx-workspace-idis not authority.Preservation proof
The replacement starts exactly from protected main
be05311f8cf0e2681edf931203f83a66f08a17f7and changes exactly the five Review paths owned by #165: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.tsThe copied source blobs match #165's repaired exact-head Review blobs; no prior check, review, or approval evidence transfers. Current-main Habit and Planning signed-authority changes remain inherited from protected main rather than duplicated here.
Test-first/security contract
Merge gate
Require the unchanged exact replacement head to pass Review tests/typecheck/build, CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, current review findings, CodeRabbit when configured, and current-live-base compatibility under live repository policy. No predecessor evidence transfers.
Supersedes #165 after unique-work preservation.
Summary by CodeRabbit
보안 강화
서비스 안정성
테스트