Repository navigation
🛡️ Sentinel: [CRITICAL] Fix authorization bypass in admin API - #582
seonghobae wants to merge 24 commits into
Conversation
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints in `AdminController` did not require any authentication or authorization checks. 🎯 Impact: Any user could access administrative resources, leading to potential data exposure or loss (e.g., deleting conversion jobs, viewing all tenant jobs). 🔧 Fix: Added `ADMIN_READ` and `ADMIN_WRITE` permissions to `TenantPermissions.java`. Injected `TenantAccessService` into `AdminController` and added `tenantAccessService.require(headers, ...)` to all endpoints to enforce proper authorization. ✅ Verification: `mvn clean test` and `mvn checkstyle:check` executed. Verified via test cases `AdminControllerTest.java` returning `403 FORBIDDEN` on unauthorized access.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough관리자 엔드포인트에 Changes관리자 테넌트 접근 제어
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: High Sequence Diagram(s)sequenceDiagram
participant 관리자
participant AdminController
participant TenantAccessService
participant DocumentConversionService
관리자->>AdminController: 관리자 요청과 HttpHeaders 전송
AdminController->>TenantAccessService: 권한 및 테넌트 컨텍스트 검증
TenantAccessService-->>AdminController: TenantContext 반환
AdminController->>DocumentConversionService: 테넌트 작업 조회 또는 변경
DocumentConversionService-->>AdminController: 작업 결과 반환
AdminController-->>관리자: HTTP 응답 반환
Possibly related PRs
Merge Risk: ⚪ Minimal · up to Admin job listing, deletion, and retry now enforce tenant permissions and ownership, including the retry race response contract. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints in `AdminController` did not require any authentication or authorization checks. 🎯 Impact: Any user could access administrative resources, leading to potential data exposure or loss (e.g., deleting conversion jobs, viewing all tenant jobs). 🔧 Fix: Added `ADMIN_READ` and `ADMIN_WRITE` permissions to `TenantPermissions.java`. Injected `TenantAccessService` into `AdminController` and added `tenantAccessService.require(headers, ...)` to all endpoints to enforce proper authorization. ✅ Verification: `mvn clean test` and `mvn checkstyle:check` executed. Verified via test cases `AdminControllerTest.java` returning `403 FORBIDDEN` on unauthorized access.
|
현재 exact head 현재 production 새 RED는 다음을 고정합니다.
@jules 이 exact RED를 기준으로 최소 causal fix를 ordinary-forward로 적용해 주세요. |
요청하신 대로 권한 확인과 동시에 Tenant Isolation을 강제하도록 모든 Admin 엔드포인트 수정 및 테스트를 강화하여 반영했습니다.
추가적인 수정사항이 필요하면 알려주세요. |
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints retrieved and managed conversion jobs globally without isolating them to the requested tenant's context, leading to unauthorized cross-tenant data access and operations. 🎯 Impact: An authenticated admin could read metadata of or manipulate conversion jobs that belonged to other isolated tenants within the system, violating multi-tenancy guarantees. 🔧 Fix: Updated `AdminController` to restrict list operations via `belongsToTenant()` and enforce strict ownership on state changes via `tenantAccessService.requireSameTenant()` and `conversionService.deleteJob(jobId, tenantContext)`. ✅ Verification: Expanded test suite in `AdminControllerTest` to explicitly cover `belongsToTenant()` filters and 404 responses for out-of-scope actions. Successfully verified `mvn clean test`, `mvn checkstyle:check`, and 100% JaCoCo coverage.
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints retrieved and managed conversion jobs globally without isolating them to the requested tenant's context, leading to unauthorized cross-tenant data access and operations. 🎯 Impact: An authenticated admin could read metadata of or manipulate conversion jobs that belonged to other isolated tenants within the system, violating multi-tenancy guarantees. 🔧 Fix: Updated `AdminController` to restrict list operations via `belongsToTenant()` and enforce strict ownership on state changes via `tenantAccessService.requireSameTenant()` and `conversionService.deleteJob(jobId, tenantContext)`. ✅ Verification: Expanded test suite in `AdminControllerTest` to explicitly cover `belongsToTenant()` filters and 404 responses for out-of-scope actions. Successfully verified `mvn clean test`, `mvn checkstyle:check`, and 100% JaCoCo coverage.
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints retrieved and managed conversion jobs globally without isolating them to the requested tenant's context, leading to unauthorized cross-tenant data access and operations. 🎯 Impact: An authenticated admin could read metadata of or manipulate conversion jobs that belonged to other isolated tenants within the system, violating multi-tenancy guarantees. 🔧 Fix: Updated `AdminController` to restrict list operations via `belongsToTenant()` and enforce strict ownership on state changes via `tenantAccessService.requireSameTenant()` and `conversionService.deleteJob(jobId, tenantContext)`. ✅ Verification: Expanded test suite in `AdminControllerTest` to explicitly cover `belongsToTenant()` filters and 404 responses for out-of-scope actions. Successfully verified `mvn clean test`, `mvn checkstyle:check`, and 100% JaCoCo coverage.
|
Current-head RED→GREEN evidence for the retry disappearance boundary:
SAST is still running, CodeQL is still queued, and there is no qualifying current-head independent review yet. Keep this PR Draft until those exact-head gates are satisfied; do not reuse verdicts from older generations. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== HEAD ==='
git rev-parse HEAD
git status --short
git diff --stat origin/main...HEAD || true
echo '=== Candidate files ==='
fd -a 'AdminController.java|DocumentConversionService.java|TenantAccessService.java|ConversionJob.java' . || true
echo '=== Java outlines ==='
fd -a -e java . | xargs -r ast-grep outline --items all --type class,method --match 'AdminController|retryDeadLettered|deleteJob|requireSameTenant|belongsToTenant|getAllJobs' 2>/dev/null || true
echo '=== Relevant definitions and call sites ==='
rg -n -C 5 --type java \
'class\s+AdminController\b|TenantAccessService\.require\s*\(|requireSameTenant\s*\(|getAllJobs\s*\(|deleteJob\s*\(|retryDeadLettered\s*\(|belongsToTenant\s*\(|RetryDeadLetterResult' \
. -g '!**/target/**' || true
echo '=== Changed Java and test files ==='
git diff --name-only origin/main...HEAD | sed -n '1,200p'
git diff --check origin/main...HEADLength of output: 149879 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Repository instructions ==='
if [ -f AGENTS.md ]; then
cat -n AGENTS.md
fi
echo '=== Complete PR diff ==='
git diff --find-renames --find-copies origin/main...HEAD -- \
src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java \
src/main/java/com/clearfolio/viewer/controller/AdminController.java \
src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java \
src/test/java/com/clearfolio/viewer/controller/AdminControllerRetryRaceTest.java \
.jules/sentinel.md
echo '=== AdminController with line numbers ==='
cat -n src/main/java/com/clearfolio/viewer/controller/AdminController.java
echo '=== Admin controller tests with line numbers ==='
cat -n src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java
cat -n src/test/java/com/clearfolio/viewer/controller/AdminControllerRetryRaceTest.java
echo '=== Permissions and authorization parsing ==='
cat -n src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java
sed -n '1,160p' src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java | cat -n
echo '=== Unrelated artifacts or patch files at HEAD ==='
fd -a 'patch_tenant_permissions.sh|search.txt|replace.txt' . || trueLength of output: 69495
I found no code findings in the reviewed change. The supplied Maven, Security Scan, and fuzz evidence is consistent with the repository gate requirements. SAST and CodeQL remain pending. Keep PR ✅ Action performedReview finished.
|
🚨 Severity: CRITICAL 💡 Vulnerability: Admin API endpoints retrieved and managed conversion jobs globally without isolating them to the requested tenant's context, leading to unauthorized cross-tenant data access and operations. 🎯 Impact: An authenticated admin could read metadata of or manipulate conversion jobs that belonged to other isolated tenants within the system, violating multi-tenancy guarantees. 🔧 Fix: Updated `AdminController` to restrict list operations via `belongsToTenant()` and enforce strict ownership on state changes via `tenantAccessService.requireSameTenant()` and `conversionService.deleteJob(jobId, tenantContext)`. ✅ Verification: Expanded test suite in `AdminControllerTest` to explicitly cover `belongsToTenant()` filters and 404 responses for out-of-scope actions. Successfully verified `mvn clean test`, `mvn checkstyle:check`, and 100% JaCoCo coverage.
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='78b5efa0fec57994eee2cee4b98ac2b6605bc534'
echo '=== Exact head ==='
actual_head="$(git rev-parse HEAD)"
printf 'expected=%s\nactual=%s\n' "$expected_head" "$actual_head"
test "$actual_head" = "$expected_head"
echo '=== Worktree and PR delta ==='
git status --short
git diff --check origin/main...HEAD
git diff --name-status origin/main...HEAD
echo '=== Repository instructions ==='
fd -a -I '^AGENTS\.md$|^CONTRIBUTING\.md$' . -x sh -c 'echo "--- $1"; cat -n "$1"' sh {} \; 2>/dev/null || true
echo '=== Relevant source and test map ==='
fd -a -e java . | xargs -r ast-grep outline --items all --type class,method \
--match 'AdminController|TenantAccessService|DocumentConversionService|ConversionJob|AdminControllerTest|AdminControllerRetryRaceTest' \
2>/dev/null || true
echo '=== Changed authorization-related diff ==='
git diff --find-renames origin/main...HEAD -- \
src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java \
src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java \
src/main/java/com/clearfolio/viewer/controller/AdminController.java \
src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java \
src/main/java/com/clearfolio/viewer/model/ConversionJob.java \
src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java \
src/test/java/com/clearfolio/viewer/controller/AdminControllerRetryRaceTest.java
echo '=== Admin route implementations ==='
admin_file="$(fd -a -e java '^AdminController\.java$' src | head -n1)"
test -n "$admin_file"
cat -n "$admin_file"
echo '=== Authorization and service contracts ==='
rg -n -C 8 --type java \
'TenantContext\s+require\s*\(|void\s+requireSameTenant\s*\(|boolean\s+deleteJob\s*\(|RetryDeadLetterResult\s+retryDeadLettered\s*\(|Optional<ConversionJob>\s+getJob\s*\(|boolean\s+belongsToTenant\s*\(' \
src/main/java -g '!**/target/**'
echo '=== Admin route test assertions and interaction order ==='
rg -n -C 6 --type java \
'AdminController|ADMIN_READ|ADMIN_WRITE|belongsToTenant|requireSameTenant|deleteJob|retryDeadLettered|NOT_FOUND|notFound|status\(\)\.isNotFound|never\(\)|InOrder' \
src/test/java/com/clearfolio/viewer/controller -g '!**/target/**'
echo '=== Current PR metadata and checks ==='
gh pr view 582 --json number,headRefOid,isDraft,reviewDecision,statusCheckRollup \
--jq '{number,headRefOid,isDraft,reviewDecision,checks:[.statusCheckRollup[]|{name,conclusion,status}]}'Length of output: 133814
I found one issue.
The other reviewed paths preserve the verified
|
seonghobae
added
bug
ChatGPT Codex Connector
요약
AdminController가 원래 인증·권한 검사를 하지 않았고, 단순ADMIN_READ/ADMIN_WRITE추가만으로는 검증된TenantContext를 버려 cross-tenant listing/delete/retry가 가능했습니다.TenantAccessService.require(...)가 반환한 tenant context를 aggregate ownership 판단까지 유지합니다. 목록은ConversionJob.belongsToTenant(...)로 tenant 범위를 제한하고, 삭제는 기존 tenant-awaredeleteJob(jobId, tenantContext)를 사용하며, retry는getJob→requireSameTenant→retryDeadLettered순서로 수행합니다. foreign/missing aggregate는 404로 숨깁니다.patch_tenant_permissions.sh,search.txt,replace.txt)는 제거했습니다.검증 계약
AGENTS.md에 따라mvn -B --no-transfer-progress verify, zero skipped tests, JaCoCo 100% line/branch, public Javadoc, security/fuzz/SAST/CodeQL/current-head review입니다.현재 PR은 Draft입니다. exact-head hosted acceptance가 모두 GREEN이 되기 전에는 완료로 보지 않습니다.
PR created automatically by Jules for task 13895687966802332716 started by @seonghobae
Summary by CodeRabbit
보안 개선
404 Not Found로 처리됩니다.테스트