🛡️ Sentinel: [CRITICAL] Fix Missing Authorization Checks#107
Open
daggerstuff wants to merge 1 commit intostagingfrom
Open
🛡️ Sentinel: [CRITICAL] Fix Missing Authorization Checks#107daggerstuff wants to merge 1 commit intostagingfrom
daggerstuff wants to merge 1 commit intostagingfrom
Conversation
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Contributor
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds missing READ-scope authorization checks to critical dataset API endpoints and documents the security learning in the Sentinel log. Sequence diagram for READ scope authorization on dataset endpointssequenceDiagram
actor User
participant API as DatasetAPI
participant Auth as AuthDependency
participant DB as Database
User->>API: HTTP GET /datasets
API->>Auth: get_current_active_user_or_api_key()
Auth-->>API: current_auth_entity(scopes)
API->>API: check PermissionLevel.READ in scopes
alt has READ scope
API->>DB: list tables
DB-->>API: datasets
API-->>User: 200 OK with datasets
else missing READ scope
API-->>User: 403 Forbidden (insufficient permissions)
end
Flow diagram for READ permission checks on dataset endpointsflowchart TD
A[Incoming request to dataset endpoint
list_datasets / get_dataset_metadata / query_dataset] --> B[Resolve current_auth_entity via get_current_active_user_or_api_key]
B --> C{Does current_auth_entity.scopes
contain PermissionLevel.READ?}
C -->|Yes| D[Execute endpoint logic
Connect to database and perform operation]
D --> E[Return 200 OK with result]
C -->|No| F[Raise HTTPException 403
Insufficient permissions]
F --> G[Return 403 Forbidden to client]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAuthorization control checks are introduced through a sentinel documentation entry and implemented in three dataset API endpoints. The endpoints ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Endpoint as FastAPI Endpoint
participant Auth as Scope Verifier
participant DB as Database
participant Response
Client->>Endpoint: Request (GET /datasets)
Endpoint->>Auth: Check PermissionLevel.READ<br/>in current_auth_entity["scopes"]
alt Scope Present
Auth->>DB: Scope verified, execute query
DB->>Auth: Return data
Auth->>Response: 200 OK with data
else Scope Missing
Auth->>Response: 403 Forbidden<br/>"Insufficient permissions"
end
Response->>Client: HTTP Response
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The authorization checks assume
current_auth_entityalways has a"scopes"key and that it’s iterable; consider using a safer access pattern (e.g.,.get("scopes", [])) or a typed object to avoid runtime errors when the auth shape changes. - The
PermissionLevel.READchecks are duplicated across three endpoints; consider extracting a small helper (e.g.,enforce_scope(current_auth_entity, PermissionLevel.READ, action_description)) or a dependency to centralize scope enforcement and keep the handlers focused on their core logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The authorization checks assume `current_auth_entity` always has a `"scopes"` key and that it’s iterable; consider using a safer access pattern (e.g., `.get("scopes", [])`) or a typed object to avoid runtime errors when the auth shape changes.
- The `PermissionLevel.READ` checks are duplicated across three endpoints; consider extracting a small helper (e.g., `enforce_scope(current_auth_entity, PermissionLevel.READ, action_description)`) or a dependency to centralize scope enforcement and keep the handlers focused on their core logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 the current code and only fix it if needed.
Inline comments:
In `@api/dataset_api.py`:
- Around line 139-140: The permission checks directly index
current_auth_entity["scopes"] which can be missing for token-auth users; change
each check in list-datasets, get_dataset_metadata, and query_dataset to first
read scopes = current_auth_entity.get("scopes") or [] and then test
PermissionLevel.READ in scopes (and similarly for other PermissionLevel checks)
so missing scopes default to an empty list and return a proper 403 instead of
causing a 500.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f8e0f54-9d6a-4b28-af79-3921d3b6cc69
📒 Files selected for processing (2)
.Jules/sentinel.mdapi/dataset_api.py
Comment on lines
+139
to
+140
| if PermissionLevel.READ not in current_auth_entity["scopes"]: | ||
| raise HTTPException(status_code=403, detail="Insufficient permissions to list datasets") |
There was a problem hiding this comment.
READ checks depend on a broken user-scope source and can fail for JWT users
These checks assume current_auth_entity["scopes"] is always valid, but Line 105 builds scopes from user.permissions. Based on the auth model (security/api_authentication.py:106-121), permissions are role-derived, so this can throw for token-auth users and produce 500s instead of clean auth decisions.
🔧 Proposed fix
async def get_current_active_user_or_api_key(
request: Request, api_key: Optional[str] = Depends(api_key_header)
):
@@
user = getattr(request.state, "authenticated_user", None)
if user:
+ scopes = auth_system.role_permissions.get(user.role, [])
return {
"username": user.username,
- "scopes": user.permissions,
+ "scopes": scopes,
"auth_type": "user_token",
}- if PermissionLevel.READ not in current_auth_entity["scopes"]:
+ scopes = current_auth_entity.get("scopes") or []
+ if PermissionLevel.READ not in scopes:
raise HTTPException(status_code=403, detail="Insufficient permissions to list datasets")Apply the same scopes = ...get("scopes") or [] guard to the checks in get_dataset_metadata and query_dataset.
Also applies to: 202-204, 267-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/dataset_api.py` around lines 139 - 140, The permission checks directly
index current_auth_entity["scopes"] which can be missing for token-auth users;
change each check in list-datasets, get_dataset_metadata, and query_dataset to
first read scopes = current_auth_entity.get("scopes") or [] and then test
PermissionLevel.READ in scopes (and similarly for other PermissionLevel checks)
so missing scopes default to an empty list and return a proper 403 instead of
causing a 500.
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.
🚨 Severity: CRITICAL\n💡 Vulnerability: Missing authorization checks on dataset API endpoints allowed any authenticated user to read datasets regardless of their permissions.\n🔧 Fix: Added PermissionLevel.READ checks to list_datasets, get_dataset_metadata, and query_dataset.\n✅ Verification: Review tests and confirm 403 Forbidden is returned for users without READ scopes.
PR created automatically by Jules for task 3826732354342097834 started by @daggerstuff
Summary by Sourcery
Enforce read-scope authorization on dataset API endpoints to prevent unauthorized data access.
Bug Fixes:
Documentation:
Summary by cubic
Added missing authorization checks to dataset API endpoints to require
PermissionLevel.READ, blocking access for users without the correct scope. Unauthorized requests now return 403 with clear error messages.PermissionLevel.READinlist_datasets,get_dataset_metadata, andquery_dataset..Jules/sentinel.mdto always verify scopes when usingget_current_active_user_or_api_key.Written for commit fd63503. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation