feat(planning): expose trusted data-rights contributor transport - #194
Conversation
|
Warning Review limit reached
Next review available in: 4 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 경계를 추가했습니다. 요청 형식, HMAC 서명, 발급 시각, HTTP 바인딩과 멱등성 키를 검증합니다. 검증된 요청은 ChangesPlanning 데이터 권리 기여자 요청
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as Internal caller
participant Controller as PlanningDataRightsController
participant Boundary as parseTrustedPlanningDataRightsRequest
participant Runtime as PlanningRuntime.dataRightsContributor
Caller->>Controller: POST /internal/data-rights/contributor
Controller->>Boundary: Verify signed request
Boundary-->>Controller: Normalized request
Controller->>Runtime: handle(request)
Runtime-->>Controller: Contributor response
Controller-->>Caller: HTTP response
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
apps/planning-service/src/planning-data-rights-controller-authority.test.ts (1)
92-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win컨트롤러의 오류 변환 경로가 테스트되지 않았습니다.
contribute의catch블록은 어떤 테스트도 실행하지 않습니다. 그 결과 기여자 실패가 자격증명 없는 503으로 변환되는지 컨트롤러 수준에서 증명되지 않습니다. 저장소가 100% 브랜치 커버리지 게이트를 적용하므로 이 분기 누락은 게이트 실패로 이어집니다.
handle이 민감한 문자열을 담은 오류로 거부하는 케이스를 추가하세요. 상태가 503인지, 응답 본문에 민감한 문자열이 없는지 단정하세요.💚 추가할 테스트 예시
+ it('sanitizes contributor failures into a credential-free 503', async () => { + process.env.PLANNING_DATA_RIGHTS_CONTEXT_SECRET = SECRET; + const handle = vi + .fn() + .mockRejectedValue(new Error('postgres password and internal topology')); + const controller = controllerWith(handle); + const issuedAt = String(Math.floor(Date.now() / 1000)); + + await expect( + controller.contribute(issuedAt, signature(issuedAt), request), + ).rejects.toMatchObject({ status: 503 }); + expect(handle).toHaveBeenCalledTimes(1); + });🤖 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-data-rights-controller-authority.test.ts` around lines 92 - 101, Extend the controller tests around contribute with a rejected handle containing a sensitive string, exercising the catch-based error conversion path. Assert the rejection has status 503 and that the response body does not expose the sensitive string, while retaining the existing assertion that the handler is not called.Source: Coding guidelines
apps/planning-service/src/planning-data-rights-http-boundary.ts (3)
206-230: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value인증 전에 본문 검증이 먼저 실행됩니다.
normalizeRequest(body)가 서명 검증보다 먼저 실행됩니다. 그 결과 인증되지 않은 호출자가 400(본문 무효)과 401(권한 무효)을 구분할 수 있고, 스키마 오라클을 얻습니다. 또한 서명 없이도 파싱 비용이 발생합니다.내부 라우트이므로 영향은 제한적입니다. 그래도 검증기 설정 확인과 헤더 형식·신선도 검증을 본문 정규화보다 먼저 수행하면 노출면이 줄어듭니다. 서명 계산에는 정규화된 요청이 필요하므로, 헤더 검증 → 본문 정규화 → HMAC 비교 순서를 권장합니다.
참고: 이 함수는
async이지만await를 사용하지 않습니다. 동기 함수로 두고 호출부에서await를 유지해도 동작은 동일합니다.🤖 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-data-rights-http-boundary.ts` around lines 206 - 230, Reorder parseTrustedPlanningDataRightsRequest so secret, request binding, and header format/freshness checks complete before calling normalizeRequest(body), preserving the existing invalid/unavailable context responses. Then compute and compare the HMAC using the normalized request, ensuring unauthenticated callers cannot observe body-validation outcomes or incur normalization cost.
108-159: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUUID 정규화와 서명 대상이 어긋날 수 있습니다.
requireUuidV4는 값을 소문자로 변환합니다.requestDigest는 정규화된 소문자 값으로 HMAC을 계산합니다. 호출자가 대문자 UUID를 보내고 자신이 보낸 원문으로 서명하면 검증은 401로 실패합니다. 동작은 fail-closed이므로 안전하지만, 호출자에게는 원인을 알기 어려운 상호운용 함정입니다.호출자 계약을 명확히 하세요. 함수 docstring 또는 런북에 "식별자는 소문자 UUIDv4로 서명해야 한다"를 명시하거나, 대문자 입력을 400으로 거부하세요.
📝 계약 명시 예시
-/** Requires and canonicalizes one opaque UUIDv4 product identity. */ +/** + * Requires and canonicalizes one opaque UUIDv4 product identity. + * + * The canonical form is lowercase. Callers must sign the lowercase form, + * because `requestDigest` binds the canonicalized value. + */ function requireUuidV4(value: unknown): string {🤖 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-data-rights-http-boundary.ts` around lines 108 - 159, Clarify the signing contract around requireUuidV4 and requestDigest by documenting that all UUID identifiers must be lowercase before signing, using the function docstring or the relevant runbook. Keep the existing lowercase canonicalization and fail-closed verification behavior unchanged.Source: Coding guidelines
253-261: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win오류를 완전히 버리면 진단이 불가능합니다.
void error는 실패 원인을 폐기합니다. 응답을 자격증명 없이 유지하는 목적은 타당합니다. 그러나 서버 측 관측 신호도 함께 사라집니다. 데이터 권리 작업이 실패해도 운영자는 원인을 알 수 없습니다.경계가 있는 구조적 로그를 남기세요. 원문 메시지, 스택 트레이스, 프롬프트, 자격증명은 제외하고 오류 클래스명과 요청 상관 식별자만 기록하세요. 이는 "각 서비스는 자체 관측성을 소유한다"는 지침과 "공개 문제·로그·아티팩트는 자격증명이 없고 경계가 있어야 한다"는 지침을 동시에 만족합니다.
또한 모든 실패를 503으로 매핑하면 영구적 오류와 일시적 오류가 구분되지 않습니다. 재시도 정책을 정의하는 호출자에게는 이 구분이 필요합니다.
🤖 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-data-rights-http-boundary.ts` around lines 253 - 261, Update toPlanningDataRightsHttpException to emit bounded structured server-side logs containing only the error class name and request correlation identifier, never raw messages, stacks, prompts, or credentials. Extend its inputs or use the existing request context to obtain that correlation identifier, and classify failures so transient errors retain 503 while permanent errors map to the appropriate non-retryable HTTP status and problem code instead of forcing every failure to 503.</codeգSource: Coding guidelines
apps/planning-service/src/planning-data-rights-http-boundary.test.ts (1)
106-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win실패 케이스 표에 경계 조건이 빠졌습니다.
현재 표는 잘못된 경로, 잘못된 메서드, 61초 경과, 검증기 미설정만 다룹니다. 다음 분기는 검증되지 않습니다.
- 미래 스큐:
issuedAt = NOW_SECONDS + 6거부,+5허용.- 수명 경계:
issuedAt = NOW_SECONDS - 60허용.- 서명 형식: 43자가 아닌 값, base64url 알파벳 외 문자.
- 비정규 base64url: 마지막 문자의 미사용 비트가 0이 아닌 값. 이 값은
actual.toString('base64url') !== headers.signature분기를 검증합니다.- 본문 형식: 배열 본문,
contractVersion불일치, 알 수 없는operation, UUIDv4가 아닌 식별자.이 분기들은 보안 제어의 핵심입니다. 저장소가 100% 브랜치 커버리지 게이트를 적용하므로, 누락 시 게이트도 실패합니다.
💚 표에 추가할 케이스 예시
{ name: 'stale evidence', secret: SECRET, binding: { method: 'POST', path: CONTRIBUTOR_PATH }, issuedAt: String(NOW_SECONDS - 61), }, + { + name: 'future evidence beyond allowed skew', + secret: SECRET, + binding: { method: 'POST', path: CONTRIBUTOR_PATH }, + issuedAt: String(NOW_SECONDS + 6), + },🤖 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-data-rights-http-boundary.test.ts` around lines 106 - 142, Expand the parameterized “fails closed for $name” cases around rejectedStatus and parseTrustedPlanningDataRightsRequest to cover future skew (+6 rejected, +5 accepted), the 60-second age boundary, malformed signature lengths and characters, non-canonical base64url signatures, and invalid bodies including arrays, mismatched contractVersion, unknown operations, and non-UUIDv4 identifiers. Add the corresponding accepted boundary assertions and preserve the expected 401/503 status behavior.Source: Coding guidelines
apps/planning-service/src/main.ts (1)
352-352: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win검증기 비밀값을 요청마다 읽습니다.
process.env를 핸들러 안에서 읽습니다. 설정이 없으면 요청 시점에 503으로 실패합니다. 동작은 fail-closed이므로 안전합니다.다만 부팅 시점 검증이 더 낫습니다. 시작할 때 값의 존재와 최소 길이를 확인하고 실패하면 프로세스를 종료하세요. 그러면 잘못 설정된 인스턴스가 트래픽을 받지 않습니다. 또한 런타임 설정을 주입 가능한 의존성으로 만들면 테스트가 전역
process.env변형에 의존하지 않습니다.🤖 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 352, Move PLANNING_DATA_RIGHTS_CONTEXT_SECRET lookup and validation out of the request handler into application startup, requiring it to be present and meet the minimum length before accepting traffic; terminate the process when invalid. Pass the validated secret into the handler through an injectable dependency so request processing no longer reads process.env and tests do not need to mutate global environment state.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 @.env.example:
- Around line 31-32: .env.example의 컨텍스트 시크릿 변수 순서를 dotenv-linter의 알파벳 정렬 규칙에 맞게
수정하세요. PLANNING_DATA_RIGHTS_CONTEXT_SECRET을 PLANNING_GATEWAY_CONTEXT_SECRET보다 앞에
배치하고, 두 변수의 자리표시자 값은 변경하지 마세요.
In `@apps/planning-service/src/main.ts`:
- Around line 344-357: Update the contribute method to inject the HTTP request
via `@Req`() and pass its actual method and route path into
parseTrustedPlanningDataRightsRequest instead of the hard-coded POST and path
values, preserving the existing headers, body, and context-secret handling.
In `@apps/planning-service/src/planning-data-rights-controller-authority.test.ts`:
- Around line 61-77: Update the mock response in the controller.contribute test
to match the request’s operation, which is 'export'. Use the export response
shape with schemaVersion, recordCount, sha256, and data, while preserving the
existing request, signature, and handle assertions.
In `@apps/planning-service/src/planning-data-rights-http-boundary.test.ts`:
- Around line 95-103: Update both rejection assertions in
apps/planning-service/src/planning-data-rights-http-boundary.test.ts:95-103 and
147-155 to use the existing rejectedStatus helper. Assert status 401 for the
tampered idempotencyKey case at lines 95-103, and status 400 for the
undeclared-field schema validation case at lines 147-155, replacing the generic
HttpException instance checks.
---
Nitpick comments:
In `@apps/planning-service/src/main.ts`:
- Line 352: Move PLANNING_DATA_RIGHTS_CONTEXT_SECRET lookup and validation out
of the request handler into application startup, requiring it to be present and
meet the minimum length before accepting traffic; terminate the process when
invalid. Pass the validated secret into the handler through an injectable
dependency so request processing no longer reads process.env and tests do not
need to mutate global environment state.
In `@apps/planning-service/src/planning-data-rights-controller-authority.test.ts`:
- Around line 92-101: Extend the controller tests around contribute with a
rejected handle containing a sensitive string, exercising the catch-based error
conversion path. Assert the rejection has status 503 and that the response body
does not expose the sensitive string, while retaining the existing assertion
that the handler is not called.
In `@apps/planning-service/src/planning-data-rights-http-boundary.test.ts`:
- Around line 106-142: Expand the parameterized “fails closed for $name” cases
around rejectedStatus and parseTrustedPlanningDataRightsRequest to cover future
skew (+6 rejected, +5 accepted), the 60-second age boundary, malformed signature
lengths and characters, non-canonical base64url signatures, and invalid bodies
including arrays, mismatched contractVersion, unknown operations, and non-UUIDv4
identifiers. Add the corresponding accepted boundary assertions and preserve the
expected 401/503 status behavior.
In `@apps/planning-service/src/planning-data-rights-http-boundary.ts`:
- Around line 206-230: Reorder parseTrustedPlanningDataRightsRequest so secret,
request binding, and header format/freshness checks complete before calling
normalizeRequest(body), preserving the existing invalid/unavailable context
responses. Then compute and compare the HMAC using the normalized request,
ensuring unauthenticated callers cannot observe body-validation outcomes or
incur normalization cost.
- Around line 108-159: Clarify the signing contract around requireUuidV4 and
requestDigest by documenting that all UUID identifiers must be lowercase before
signing, using the function docstring or the relevant runbook. Keep the existing
lowercase canonicalization and fail-closed verification behavior unchanged.
- Around line 253-261: Update toPlanningDataRightsHttpException to emit bounded
structured server-side logs containing only the error class name and request
correlation identifier, never raw messages, stacks, prompts, or credentials.
Extend its inputs or use the existing request context to obtain that correlation
identifier, and classify failures so transient errors retain 503 while permanent
errors map to the appropriate non-retryable HTTP status and problem code instead
of forcing every failure to 503.</codeգ
🪄 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: f67332d1-4ccb-4602-af95-b6719d1e7088
📒 Files selected for processing (5)
.env.exampleapps/planning-service/src/main.tsapps/planning-service/src/planning-data-rights-controller-authority.test.tsapps/planning-service/src/planning-data-rights-http-boundary.test.tsapps/planning-service/src/planning-data-rights-http-boundary.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/planning-service/src/planning-data-rights-controller-authority.test.ts`:
- Around line 15-18: Align all contribute invocations in the affected tests with
the three-argument contract (issuedAt, signature, body) by removing HTTP_REQUEST
unless the implementation is updated to accept it. Update main.ts so the actual
request method and originalUrl flow into the parser if HTTP binding is part of
the intended HMAC behavior; otherwise remove HTTP_REQUEST from every related
test and rewrite the GET binding test to match the fixed POST binding.
🪄 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: 9fab874b-289e-4e7a-ac40-b32a1908f11c
📒 Files selected for processing (1)
apps/planning-service/src/planning-data-rights-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/planning-service/src/main.ts`:
- Around line 61-65: Add an explanatory JSDoc contract above the
RequestBindingSource interface, documenting that it represents HTTP binding
input used for signature verification and clarifying the source and purpose of
the optional method and originalUrl fields. Keep the interface shape 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: 14f59871-adee-4dc9-ba79-7e3c80d8f8a4
📒 Files selected for processing (3)
.env.exampleapps/planning-service/src/main.tsapps/planning-service/src/planning-data-rights-http-boundary.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/planning-service/src/planning-data-rights-http-boundary.test.ts
1 participant
Buyer/privacy outcome
Advance #55 by exposing the already-protected Planning-owned
life-os.data-rights-contributor.v1implementation through a service-authenticated internal HTTP boundary. Identity can invoke Planning-owned export/preflight/erase/verification without direct Planning SQL authority.Test-first sequence
The branch first added the HTTP authority contract while
planning-data-rights-http-boundarydid not exist, then implemented the smallest request-bound verifier and controller wiring. Historical predecessor evidence does not transfer; this exact head must prove GREEN.Authority contract
POST /v1/internal/data-rights/contributorresource;Scope / maturity
This is one contributor transport slice, not complete end-to-end data-rights orchestration. Identity registration/client orchestration, remaining service contributors, artifact delivery, lifecycle/audit, recovery and complete buyer acceptance remain #55 work.
Merge gate
Keep Draft until the unchanged exact head passes CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, current automated review, package typecheck/test/build and live-base compatibility with zero actionable findings. No stale review/check evidence or administrative bypass.
Refs #55.
Summary by CodeRabbit
새 기능
오류 처리
설정