Security: Source group CRUD endpoints lack authentication/authorization checks - #127
Conversation
The source_groups router exposes list/get/create/update operations with only a DB dependency and no `get_current_user` or role-based authorization dependency. This allows unauthenticated or unauthorized callers to read and modify source group configuration, which may include sensitive ingestion settings. Affected files: source_groups.py Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
|
🚀 nice! was on our todo 🙏🏽 |
There was a problem hiding this comment.
Pull request overview
Adds authentication/authorization enforcement to the source_groups API router to prevent unauthenticated access to source group configuration and to introduce basic audit logging for configuration changes.
Changes:
- Require authentication for
GET /source_groupsandGET /source_groups/{group_id}. - Add a write-access dependency for
POST/PUTendpoints. - Emit audit logs on source group create/update.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def require_source_group_write_access(current_user: Any = Depends(get_current_user)): | ||
| role = getattr(current_user, "role", None) | ||
| roles = getattr(current_user, "roles", None) | ||
|
|
||
| if isinstance(current_user, dict): | ||
| role = role or current_user.get("role") | ||
| roles = roles or current_user.get("roles") | ||
|
|
||
| if role in {"admin", "editor"}: | ||
| return current_user | ||
|
|
||
| if isinstance(roles, (list, set, tuple)) and any(r in {"admin", "editor"} for r in roles): | ||
| return current_user | ||
|
|
||
| raise HTTPException(status_code=403, detail="Insufficient permissions") |
There was a problem hiding this comment.
require_source_group_write_access checks for roles {"admin", "editor"}, but the codebase’s defined roles are ["admin", "manager", "triage", "analyst"] (see core/api_routers/auth.py VALID_ROLES). As written, no user can ever have the "editor" role and non-admin roles will be blocked from writes even if they have permissions via role_permissions. Consider reusing the existing core.auth.require_permission(page, "write") mechanism (and adding a page entry for source groups if needed) or updating the role list/permission model consistently across the app.
| db.add(db_sg) | ||
| db.commit() | ||
| db.refresh(db_sg) | ||
| logger.info("source_group_created", extra={"source_group_id": str(db_sg.id), "actor": str(getattr(current_user, "id", None) or current_user.get("id") if isinstance(current_user, dict) else None)}) |
There was a problem hiding this comment.
The audit log actor field is always None because get_current_user() returns a dict with user_id, not id. Use current_user.get("user_id") (or username) and consider extracting it into a local variable for readability.
| db_sg.config = sg.config | ||
| db.commit() | ||
| db.refresh(db_sg) | ||
| logger.info("source_group_updated", extra={"source_group_id": str(db_sg.id), "actor": str(getattr(current_user, "id", None) or current_user.get("id") if isinstance(current_user, dict) else None)}) |
There was a problem hiding this comment.
Same issue as above: get_current_user() provides user_id, not id, so this audit log records actor=None. Use current_user.get("user_id") (or username) to make the audit trail reliable.
| @router.get("/", response_model=List[SourceGroupRead]) | ||
| def list_source_groups(db: Session = Depends(get_db)): | ||
| def list_source_groups( | ||
| db: Session = Depends(get_db), | ||
| current_user: Any = Depends(get_current_user) | ||
| ): |
There was a problem hiding this comment.
Auth/authorization behavior changed for these endpoints (now requires authentication, and writes require elevated access), but there are no API router tests for source_groups. Add tests similar to core/tests/test_api_routers/test_jobs.py that cover: unauthenticated requests => 401, authenticated but insufficient role => 403 on POST/PUT, and permitted role => success.
Jovonni
left a comment
Jovonni
left a comment
There was a problem hiding this comment.
Thanks for catching this — the missing auth on this router is a real gap and I want it closed. A few changes before we merge so it lines up with how the rest of the API does authz:
-
Reuse
require_permissioninstead of a bespoke check. Every other write router (models,cases,settings,data) gates onDepends(require_permission("<page>", "write"))fromcore/auth.py, which honors therole_permissionstable and the admin bypass. The customrequire_source_group_write_accessintroduces aneditorrole that isn't part of our RBAC model, so it'd behave differently from the rest of the app. Suggest: reads →require_permission("data", "read"), writes →require_permission("data", "write"). That drops the whole helper. -
Dead code:
get_current_useralways returns aUsermodel, never a dict, so theisinstance(current_user, dict)branches (and the inline actor extraction in the log lines) never execute. Once you switch torequire_permissionthe audit-log lines simplify a lot —current_user.idis enough. -
Not blocking, just noting: this router still has no
DELETE, and separatelyrequire_permissioncurrently fails open on exception (core/auth.py). I'll track the fail-open hardening on its own — out of scope here.
Swap to require_permission and I'll get this in right after the batch of feature work I've got in flight. Appreciate the contribution.
Problem
The source_groups router exposes list/get/create/update operations with only a DB dependency and no
get_current_useror role-based authorization dependency. This allows unauthenticated or unauthorized callers to read and modify source group configuration, which may include sensitive ingestion settings.Severity:
highFile:
core/api_routers/source_groups.pySolution
Add authentication (
Depends(get_current_user)) and enforce role-based access control (e.g., admin/editor) for create/update/delete. Restrict list/get access as appropriate and add audit logging for configuration changes.Changes
core/api_routers/source_groups.py(modified)Testing