fix(security): enforce production HMAC signing key readiness - #313
fix(security): enforce production HMAC signing key readiness#313seonghobae wants to merge 8 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughProduction readiness 검증이 tenant-claims와 artifact-token HMAC 비밀을 모두 확인하도록 변경되었습니다. UTF-8 최소 길이, NUL·유니코드 공백 정규화, 키 분리 조건과 관련 테스트가 추가되었습니다. ChangesProduction HMAC readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 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 review Please review exact current source head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='bcf9989eaebac435802425708cdc2a48f243d295'
base='f3cc09a9838f0f88c81a2ceae22138fab80a2edb'
printf '%s\n' '== Exact commits =='
git show -s --format='base %H%n%P%n%an <%ae>%n%s' "$base"
git show -s --format='head %H%n%P%n%an <%ae>%n%s' "$head"
printf '%s\n' '== Changed files and diff =='
git diff --name-status "$base" "$head"
git diff --find-renames --find-copies --check "$base" "$head"
git diff --find-renames --find-copies --unified=80 "$base" "$head"
printf '%s\n' '== Candidate source and test files =='
fd -t f -i 'ProductionAuthReadinessConfig' . || true
printf '%s\n' '== Candidate file AST outlines =='
fd -t f -i 'ProductionAuthReadinessConfig' . -x ast-grep outline {} --items all --view expanded || true
printf '%s\n' '== Tenant-claims configuration, guards, and test references =='
rg -n -i -C 8 \
'(clearfolio\.tenant-claims\.hmac-secret|tenant.?claims|hmac.?secret|ProductionAuthReadinessConfig)' \
--glob '!**/target/**' --glob '!**/.git/**' .
printf '%s\n' '== Production profile configuration files =='
fd -t f -i 'application*' . -x sh -c '
case "$1" in
*.yml|*.yaml|*.properties)
if rg -q -i "(spring\.config\.activate\.on-profile|spring\.profiles|production|tenant-claims|hmac-secret)" "$1"; then
echo "--- $1"
cat -n "$1"
fi
;;
esac
' sh {} || trueLength of output: 143632 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/clearfolio'
pr='313'
head='bcf9989eaebac435802425708cdc2a48f243d295'
base='f3cc09a9838f0f88c81a2ceae22138fab80a2edb'
printf '%s\n' '== Commit ancestry and changed paths =='
git merge-base --is-ancestor "$base" "$head" && echo 'base is an ancestor of head'
git diff --name-only "$base" "$head"
git diff --check "$base" "$head"
printf '%s\n' '== Fixture UTF-8 byte counts =='
python3 - <<'PY'
for value in ("short-hmac-key!", "0123456789abcdef", "production-secret"):
print(f"{value!r}: chars={len(value)}, utf8_bytes={len(value.encode('utf-8'))}")
PY
printf '%s\n' '== Exact PR metadata, reviews, and commits =='
gh pr view "$pr" --repo "$repo" --json \
number,state,isDraft,mergeable,headRefOid,baseRefOid,reviewDecision,reviews,statusCheckRollup,commits \
--jq '{
number, state, isDraft, mergeable, headRefOid, baseRefOid, reviewDecision,
reviews: [.reviews[] | {author: .author.login, state, commit: .commit.oid}],
checks: [.statusCheckRollup[] | {name, status, conclusion}],
commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}]
}'
printf '%s\n' '== Pull-request review threads at the requested head =='
gh api graphql \
-F owner='ContextualWisdomLab' \
-F name='clearfolio' \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes {
author { login }
commit { oid }
body
path
line
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false)
| {comments: [.comments.nodes[] | select(.commit.oid == "bcf9989eaebac435802425708cdc2a48f243d295")
| {author: .author.login, path, line, body}]})'
printf '%s\n' '== Build and JavaDoc configuration =='
fd -t f -g 'pom.xml' -g 'build.gradle' -g 'build.gradle.kts' . -x sh -c '
echo "--- $1"
rg -n -i -C 4 "(maven-javadoc|javadoc|doclint|maven-checkstyle|spotless|maven-enforcer)" "$1" || true
' sh {}Length of output: 6086
I verified the exact requested head and base. The implementation uses UTF-8 bytes in
|
seonghobae
changed the title
|
@coderabbitai review Please review exact current source head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='4724bf33637bec6385551280d59b0e14bcb296ab'
base='f3cc09a9838f0f88c81a2ceae22138fab80a2edb'
repo='ContextualWisdomLab/clearfolio'
printf '%s\n' '== Exact commits and ancestry =='
git show -s --format='base: %H%nparents: %P%nsubject: %s' "$base"
git show -s --format='head: %H%nparents: %P%nsubject: %s' "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base is an ancestor of head'
printf '%s\n' '== Changed paths and diff =='
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
git diff --find-renames --unified=100 "$base" "$head"
printf '%s\n' '== Candidate source and test files =='
fd -t f -i 'ProductionAuthReadinessConfig' . -E target
fd -t f -i 'ArtifactLinkService' . -E target
printf '%s\n' '== Java outlines =='
fd -t f -i 'ProductionAuthReadinessConfig' . -E target -x ast-grep outline {} --items all --view expanded
fd -t f -i 'ArtifactLinkService' . -E target -x ast-grep outline {} --items all --view expanded
printf '%s\n' '== Relevant configuration and signing references =='
rg -n -i -C 8 \
'(clearfolio\.tenant-claims\.hmac-secret|clearfolio\.artifact-token\.secret|ProductionAuthReadinessConfig|ArtifactLinkService|HmacSHA|random.*key|SecureRandom)' \
--glob '!**/target/**' --glob '!**/.git/**' .
printf '%s\n' '== Exact focused files at requested head =='
for path in $(git diff --name-only "$base" "$head"); do
case "$path" in
*.java)
echo "--- $path"
git show "$head:$path" | cat -n
;;
esac
done
printf '%s\n' '== Exact PR and related PR changed paths =='
for pr in 313 270 268; do
echo "--- PR #$pr"
gh pr view "$pr" --repo "$repo" --json number,state,isDraft,headRefOid,baseRefOid,files,statusCheckRollup \
--jq '{number,state,isDraft,headRefOid,baseRefOid,files:[.files[].path],checks:[.statusCheckRollup[]|{name,status,conclusion}]}' || true
doneLength of output: 233156
I verified the requested source head and protected base. The production-only guard rejects missing or blank values and rejects secrets under 16 UTF-8 bytes. It blocks the production use of the process-local artifact-token fallback. The diff does not change the claim payload, artifact-token payload, or
|
|
@coderabbitai review Please review exact current source head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/clearfolio'
pr='313'
head='c1455ad0f69ba0ff1d3bb890e83e0dba5b9ad9d2'
base='f3cc09a9838f0f88c81a2ceae22138fab80a2edb'
printf '%s\n' '== Exact commits and ancestry =='
git show -s --format='base %H%nparents %P%nsubject %s' "$base"
git show -s --format='head %H%nparents %P%nsubject %s' "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
printf '%s\n' '== Changed paths and whitespace check =='
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
printf '%s\n' '== Changed Java outlines =='
git diff --name-only "$base" "$head" -- '*.java' | while IFS= read -r path; do
printf '\n--- %s\n' "$path"
ast-grep outline "$path" --items all --view expanded
done
printf '%s\n' '== Exact changed Java source =='
git diff --name-only "$base" "$head" -- '*.java' | while IFS= read -r path; do
printf '\n--- %s\n' "$path"
git show "$head:$path" | cat -n
done
printf '%s\n' '== Relevant implementation and call sites =='
rg -n -C 8 \
'(ProductionAuthReadinessConfig|ArtifactLinkService|clearfolio\.tenant-claims\.hmac-secret|clearfolio\.artifact-token\.secret|HmacSHA|SecureRandom|MessageDigest\.isEqual|getBytes\(StandardCharsets\.UTF_8\))' \
--glob '!**/target/**' --glob '!**/.git/**' .
printf '%s\n' '== Relevant file outlines =='
fd -t f -E target -E .git \
'ProductionAuthReadinessConfig\.java|ProductionAuthReadinessConfigTest\.java|ArtifactLinkService\.java' . \
-x ast-grep outline {} --items all --view expanded
printf '%s\n' '== PR state, checks, and changed-path overlap =='
gh pr view "$pr" --repo "$repo" \
--json state,isDraft,mergeable,headRefOid,baseRefOid,reviewDecision,statusCheckRollup \
--jq '{state,isDraft,mergeable,headRefOid,baseRefOid,reviewDecision,checks:[.statusCheckRollup[]|{name,status,conclusion}]}'
for related_pr in 270 268; do
printf '\n--- PR #%s paths\n' "$related_pr"
gh pr view "$related_pr" --repo "$repo" --json files \
--jq '.files[].path' || true
done
printf '%s\n' '== Unresolved review threads on exact requested head =='
gh api graphql \
-F owner='ContextualWisdomLab' -F name='clearfolio' -F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes { author { login } commit { oid } body path line }
}
}
}
}
}
}' \
--jq --arg head "$head" '
.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false)
| .comments.nodes[]
| select(.commit.oid == $head)
| {author: .author.login, path, line, body})'Length of output: 203491 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/clearfolio'
pr='313'
head='c1455ad0f69ba0ff1d3bb890e83e0dba5b9ad9d2'
base='f3cc09a9838f0f88c81a2ceae22138fab80a2edb'
printf '%s\n' '== Exact changed paths =='
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
printf '%s\n' '== Focused test source at exact head =='
git show "$head:src/test/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfigTest.java" | cat -n
printf '%s\n' '== Exact-head unresolved review threads =='
gh api graphql \
-F owner='ContextualWisdomLab' \
-F name='clearfolio' \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes {
author { login }
commit { oid }
body
path
line
}
}
}
}
}
}
}' \
| jq --arg head "$head" '
[.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| .comments.nodes[]
| select(.commit.oid == $head)
| {author: .author.login, path, line, body}]'
printf '%s\n' '== UTF-8 fixture counts =='
python3 - <<'PY'
values = {
"tenant_short": "short-hmac-key!",
"tenant_minimum": "0123456789abcdef",
"artifact_short": "short-art-key!",
"artifact_minimum": "0123456789abcdef",
"non_ascii_example": "éééééééé",
}
for name, value in values.items():
print(f"{name}: code_points={len(value)}, utf8_bytes={len(value.encode('utf-8'))}")
PYLength of output: 7246
I verified the exact requested head and base. The diff changes only The artifact-token development fallback remains outside the production profile guard. The claim payload, artifact-token payload, and You are interacting with an AI system. |
|
@opencode-agent @cwl-noema-review Please review exact current source head |
d026524 to
2e1dc91
Compare
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/config/ProductionAuthReadinessConfig.java`:
- Around line 52-54: Update ProductionAuthReadinessConfig, TenantAccessService,
and ArtifactLinkService to resolve tenant-claims and artifact-token secrets
through the same KV or credential registry used by the application, removing
their Spring `@Value` environment-placeholder injection. Keep environment
variables limited to bootstrapping that shared secret store, and ensure all
three components consume the resolved values consistently.
In
`@src/test/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfigTest.java`:
- Around line 102-118: Update
productionProfileStartsAtMinimumSignedTenantClaimsSecretLength and
productionProfileStartsAtMinimumArtifactTokenSecretLength to use distinct
non-ASCII secrets that are exactly 16 UTF-8 bytes, replacing the current ASCII
boundary fixtures while keeping the startup-success assertions unchanged.
- Around line 41-79: Extend ProductionAuthReadinessConfigTest with coverage for
both secret types: add a tenant-claims secret containing NUL characters but at
least 16 UTF-8 bytes and assert the “without NUL or surrounding whitespace”
failure, and add whitespace-only tenant and artifact-token secrets whose raw
values are at least 16 bytes while asserting the missing-secret validation
errors. Keep the existing productionRunner and direct-constructor testing
patterns.
🪄 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: daf91af9-801a-42e7-bc34-4f6ce5ec8738
📒 Files selected for processing (2)
src/main/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfig.javasrc/test/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfigTest.java
| public ProductionAuthReadinessConfig( | ||
| @Value("${clearfolio.tenant-claims.hmac-secret:}") String tenantClaimsSecret) { | ||
| if (!StringUtils.hasText(tenantClaimsSecret)) { | ||
| @Value("${clearfolio.tenant-claims.hmac-secret:}") String tenantClaimsSecret, | ||
| @Value("${clearfolio.artifact-token.secret:}") String artifactTokenSecret) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
비밀 조회를 Spring 환경 placeholder에서 분리하세요.
Line 53과 Line 54는 HMAC 비밀을 @Value Spring 환경 placeholder에서 읽습니다. 이는 production 비밀의 허용된 소스가 아닙니다. ProductionAuthReadinessConfig, TenantAccessService, ArtifactLinkService가 동일한 KV 또는 credential registry에서 해석한 비밀을 사용하도록 마이그레이션하세요. 환경 변수는 해당 저장소의 bootstrap에만 사용하세요.
As per coding guidelines, 새 비밀과 기존 artifact-token 및 tenant-claims 비밀은 Spring 환경 placeholder 대신 KV 또는 credential registry에서 읽어야 합니다.
🤖 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
`@src/main/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfig.java`
around lines 52 - 54, Update ProductionAuthReadinessConfig, TenantAccessService,
and ArtifactLinkService to resolve tenant-claims and artifact-token secrets
through the same KV or credential registry used by the application, removing
their Spring `@Value` environment-placeholder injection. Keep environment
variables limited to bootstrapping that shared secret store, and ensure all
three components consume the resolved values consistently.
Source: Coding guidelines
1 participant
Objective
Fail closed when production uses missing, ephemeral, undersized, purpose-reused, or misleadingly normalized HMAC signing material for tenant claims and artifact links.
Fresh exact state — 2026-08-10
610f081cc56cfcaa372d674a56f4b21c91123c8f;main:55d7ae8647208e301f282350f076eeddaba61d11after protected merge of fix(security): harden audit pseudonymization and refresh Netty evidence #270;31387412055: success;31387411478: success;31387411422: success;31387412079: success;The former divergent branch and every predecessor run are historical. #270's
mvn verify, zero-missed owned production coverage, strict public-Javadoc and privacy/security substrate are now protected-main behavior.Test-first security evidence
This bounded slice establishes regressions for:
strip()normalization used byTenantAccessService;Clean-base coverage RED → GREEN
Initial clean head
2e1dc916a0b4f5392439da147abf190a18cf3e3cexecuted 505 Maven tests with zero failures, errors, or skips, but exact CI31387048868failed the uncompromised JaCoCo gate:ProductionAuthReadinessConfig.javaretained one missed line and one missed branch in the explicit-null normalization path.Current head
610f081cc56cfcaa372d674a56f4b21c91123c8fadds the focused null-secret regression without changing production behavior or weakening coverage. Exact-head CI, Security Scan, SAST, and fuzz are all GREEN.Claim boundary
The 16-byte floor is a minimum-strength guard, not proof of entropy, independent generation, KMS custody, rotation, or complete key-management compliance. NIST SP 800-224 remains draft guidance; final key-management authority and operational controls remain distinct. Issue #319 owns migration from direct environment/config secret authority to a provider-neutral credential registry and must not be conflated with this readiness slice. Issue #314 separately owns OIDC/JWT federation.
Scope
Only these files change:
src/main/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfig.javasrc/test/java/com/clearfolio/viewer/config/ProductionAuthReadinessConfigTest.javaThe slice does not change tenant-claim payload/signature semantics, artifact-token payloads, HMAC algorithm, endpoint authorization, or secret transport.
Merge gate
Move to Ready for ordinary independent review. Merge only if this unchanged exact head continues to satisfy every live required check, zero valid unresolved findings, the counted write-authorized independent approval required by repository policy, and expected-head-safe protection. Do not treat this key-readiness slice as completion of issue #319's credential-source migration.
Summary by CodeRabbit
보안 및 안정성 개선
테스트