fix(security): authenticate artifact tokens before object lookup on current main - #431
fix(security): authenticate artifact tokens before object lookup on current main#431seonghobae wants to merge 6 commits into
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughPDF 조회 전에 읽기 토큰을 사전 검증하도록 변경했습니다. 토큰 오류는 문서 변환 및 아티팩트 조회 전에 반환합니다. 테넌트와 체크섬 검증은 최종 검증 단계에서 유지합니다. Changes아티팩트 토큰 접근 제어
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to This PR changes artifact-token authentication while retaining secret injection through a Spring environment placeholder, conflicting with the required credential-registry handling and leaving a security configuration risk unresolved; merge should wait for migration or explicit owner acceptance. Sequence Diagram(s)sequenceDiagram
participant Client
participant ArtifactController
participant ArtifactLinkService
participant DocumentConversionService
participant ArtifactStore
Client->>ArtifactController: PDF 요청 및 토큰 전달
ArtifactController->>ArtifactLinkService: preauthorizeReadToken(docId, token)
ArtifactLinkService-->>ArtifactController: 토큰 클레임 또는 토큰 오류
ArtifactController->>DocumentConversionService: 문서 및 작업 조회
ArtifactController->>ArtifactStore: PDF 아티팩트 조회
ArtifactController->>ArtifactLinkService: verifyReadToken(docId, token, 작업, 체크섬)
ArtifactLinkService-->>ArtifactController: 최종 검증 결과
ArtifactController-->>Client: PDF 또는 오류 응답
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: 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 `@src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java`:
- Around line 186-199: ArtifactLinkService still sources the artifact-token
secret through a Spring `@Value` environment placeholder. Replace that injection
with the project’s credential registry or KV-based secret provider, remove the
placeholder dependency, and update the service’s token-authentication
initialization to use the registry-provided value while preserving existing
artifact-token and tenant-claims validation 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: 4fafca6a-6dae-45c3-a606-bf078e53f378
📒 Files selected for processing (5)
src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.javasrc/main/java/com/clearfolio/viewer/controller/ArtifactController.javasrc/test/java/com/clearfolio/viewer/controller/ArtifactControllerPostLookupBindingTest.javasrc/test/java/com/clearfolio/viewer/controller/ArtifactControllerTest.javasrc/test/java/com/clearfolio/viewer/controller/ArtifactControllerTokenPreconditionTest.java
|
Exact-current-head read-only review request for @opencode-agent review |
Rate Limit Exceeded
|
|
@opencode-agent review Review exact head Review read-only; preserve independent reviewer identity. Do not mutate the branch, synthesize author approval, weaken protection, or merge. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='2f16af873714e62870d059324f9fe0bdead29874'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
if [ "$actual" != "$expected" ]; then
echo 'ERROR: repository HEAD is not the requested commit.' >&2
exit 2
fi
for f in \
src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java \
src/main/java/com/clearfolio/viewer/controller/ArtifactController.java \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerTokenPreconditionTest.java \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerPostLookupBindingTest.java \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerTest.java
do
test -f "$f"
printf '\n===== OUTLINE: %s =====\n' "$f"
ast-grep outline "$f" --view expanded || true
done
printf '\n===== AUTHORIZATION, LOOKUP, AND BINDING CALL SITES =====\n'
rg -n -C 8 --type java \
'(preauthorizeReadToken|verifyReadToken|conversionService|artifactStore|findBy|load|tenant|checksum)' \
src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java \
src/main/java/com/clearfolio/viewer/controller/ArtifactController.java
printf '\n===== FOCUSED TEST ASSERTIONS =====\n'
rg -n -C 6 --type java \
'(UNAUTHORIZED|FORBIDDEN|never\(|verify\(|preauthorizeReadToken|verifyReadToken|checksum|tenant|artifact)' \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerTokenPreconditionTest.java \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerPostLookupBindingTest.java \
src/test/java/com/clearfolio/viewer/controller/ArtifactControllerTest.javaLength of output: 142735 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '===== ArtifactController.getPdf ====='
sed -n '120,215p' src/main/java/com/clearfolio/viewer/controller/ArtifactController.java
printf '%s\n' '===== Token resolution and parser ====='
sed -n '330,390p' src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java
printf '%s\n' '===== Preauthorization and post-lookup binding ====='
sed -n '180,250p' src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java
printf '%s\n' '===== Invalid-credential regression methods ====='
sed -n '20,95p' src/test/java/com/clearfolio/viewer/controller/ArtifactControllerTokenPreconditionTest.java
printf '%s\n' '===== All production artifact read route declarations ====='
rg -n -C 4 --type java \
'@(GetMapping|RequestMapping).*artifacts|/artifacts/|Mono<ResponseEntity<byte\[\]>>|ResponseEntity<byte\[\]>' \
src/main/java
printf '%s\n' '===== All production conversion-job and artifact-store reads ====='
rg -n -C 5 --type java \
'\.(getJob|getPdf)\s*\(' \
src/main/javaLength of output: 42203 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '===== ConversionController.downloadArtifact ====='
sed -n '205,305p' src/main/java/com/clearfolio/viewer/controller/ConversionController.java
printf '%s\n' '===== Download route tests and signed-token call sites ====='
rg -n -C 8 --type java \
'(downloadArtifact|/api/v1/convert/jobs/.*/download|verifyReadToken|resolveToken|preauthorizeReadToken)' \
src/test/java src/main/java/com/clearfolio/viewer/controller/ConversionController.javaLength of output: 90860
I found one blocking issue.
Move I did not approve or merge the pull request.
|
|
Exact-current-head independent review request for The five-file artifact-read preauthorization slice is mergeable; CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; the only inline finding is resolved and tracked to the separate credential-registry architecture in #319/#344. Invalid or absent credentials are rejected before conversion-job or artifact-store lookup, while post-lookup tenant/checksum binding remains mandatory. @opencode-agent review Review this exact head read-only. Do not mutate the branch, synthesize author approval, weaken protection, or merge. Protected |
Rate Limit Exceeded
|
|
Exact-head review/update request for The artifact preauthorization slice has terminal-success CI, Security Scan, SAST Semgrep, and fuzz, is mergeable, and has zero unresolved threads. Its construction base predates current protected @coderabbitai review |
Rate Limit Exceeded
|
1 participant
Objective
Authenticate public artifact-read tokens before conversion-job or artifact-store lookup so token validity cannot become a document-existence oracle.
Exact current-head evidence
961f44a19c4ced87f8062b3ab9842a9a08d05235;cd5a8a965dfefbe81c131d60748d7311dda858bd;2f16af873714e62870d059324f9fe0bdead29874;Contract
Missing credentials, unsupported Authorization schemes, non-empty invalid query tokens, and invalid Bearer tokens return
401before any conversion-job or artifact-store interaction. The preauthorization phase verifies syntax, signature, lifetime, read scope, document binding, issued-link authority, and revocation state without object lookup. Full tenant and artifact-checksum binding remains mandatory after lookup and before bytes are returned.Credential-source dependency
The existing Spring-placeholder secret source is unchanged from protected
main; its provider-neutral KV/credential-registry migration is explicitly owned by #319 with rotation, restart/replica, least-privilege, no-secret-telemetry, and standalone/composed adapter acceptance. This PR does not claim that the current source is commercially final and must not duplicate or race #319's architecture.Scope boundary
Artifact-read preauthorization and focused regressions only. Parser hardening, checksum/tenant binding, range semantics, read audit, and signed-link behavior are preserved.
Merge gate
Keep this exact head unchanged. Auto-merge may act only after live checks remain successful, zero valid unresolved findings remain, and a qualifying independent non-author approval is attached to this exact head. Automated evidence and predecessor reviews do not transfer.