Fix/backend p0 audit - #6602
Conversation
Patches oss.src.services.user_service.create_reset_password_link instead of supertokens_python... source module. The service imports the function by name, so patching the source module does not intercept the local binding. All 4 IDOR tests pass. Full access test suite: 12/12 passing.
Keeps both: - Main's new organization owner password-reset guard - Our IDOR fix: PermissionError/NoResultFound handling
The merge added new DB calls (get_project_by_id, get_organization_owner) to the router and changed the import structure. Updated mock patches: - Fixed create_reset_password_link patch target to match post-merge import - Added mocks for db_manager.get_project_by_id - Added mocks for db_manager.get_organization_owner - Verified all 4 IDOR tests pass without real network calls
… reset Adds organization membership check in generate_user_password_reset_link using db_manager.get_user_org_and_workspace_id BEFORE get_user_with_id. Raises PermissionError when target user belongs to a different org, which the router catches and returns 403. Fixes test_cross_org_reset_returns_403 to mock org mismatch path.
Adds org membership check before get_user_with_id in generate_user_password_reset_link. Updates tests to mock get_user_org_and_workspace_id for cross-org/same-org paths.
Ensures get_user_org_and_workspace_id is called before get_user_with_id. Raises PermissionError on org mismatch. Updates tests to mock org check correctly.
…t tests The router now reads request.state.organization_id for the cross-org check. All test request mocks must include this attribute.
The test asserts 'permission' appears in the 403 detail text. The PermissionError message and the router's hardcoded detail both now include the word 'permission', matching the convention used by every other permission-denied response in user_profile.py. No logic, status-code, or exception-type changes — wording only.
get_user_org_and_workspace_id() returns a dict with an organization_ids list, not a 2-tuple. The previous tuple-unpacking silently bound target_org_id to the literal key name id, causing every same-organization password reset to be incorrectly rejected with 403. Fixed to check membership against organization_ids, and updated test mocks to match the real return shape.
|
@shafeeq27edu-ai is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe changes add organization-based authorization to password-reset links, standardize missing-user responses, replace assertion validation with explicit exceptions, correct API key not-found handling, and load demo IDs from centralized configuration. ChangesPassword reset authorization
Runtime error handling and configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Password resets now enforce organization boundaries and return consistent denial responses, while related validation and missing-resource handling are explicit. The covered behavior is ready to merge with no identified current-head risk. Sequence Diagram(s)sequenceDiagram
participant Caller
participant UserProfileRouter
participant UserService
participant DBManager
Caller->>UserProfileRouter: request password-reset link
UserProfileRouter->>UserService: pass target user and caller organization
UserService->>DBManager: load target-user organization IDs
DBManager-->>UserService: return organization IDs
UserService-->>UserProfileRouter: return link or authorization error
UserProfileRouter-->>Caller: return reset link or 403 response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 1f540dad-4451-4539-aaf1-5a431d097f27
📒 Files selected for processing (8)
api/oss/src/core/access/permissions/service.pyapi/oss/src/routers/api_key_router.pyapi/oss/src/routers/user_profile.pyapi/oss/src/services/commoners.pyapi/oss/src/services/db_manager.pyapi/oss/src/services/user_service.pyapi/oss/src/utils/exceptions.pyapi/oss/tests/pytest/unit/access/test_reset_password_idor.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
thanks for the pr. i found one additional behavior gap that needs a change before qa. the existing exact-head security finding also remains open.
…umeration leak - Resolve user_profile.py merge conflict: keep status_code=404 (fix/backend-p0-audit), discard 400 (main) - Fix latent bug: get_user_with_id raises NoResultFound, never returns None. Wrap in try/except so a stale/deleted user_id returns 404 instead of crashing 500 - Fix user-enumeration leak: collapse PermissionError and NoResultFound handlers in reset_user_password to a single 403 response — callers can no longer distinguish 'user in another org' from 'user does not exist' - Update test 3 to assert 403 (was 404) with permission-denied body - Add test 5 for user_profile() missing-user → 404 via NoResultFound
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/oss/tests/pytest/unit/access/test_reset_password_idor.py (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant section comments.
Keep the module docstring that explains the security regression. Remove structural headings such as “Helpers”, “Fixtures”, and test-number banners. The declarations already show this structure.
Also applies to: 33-35, 65-67, 90-92, 138-140, 214-216, 261-263, 282-284
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: f2ff2d13-e253-498c-ad08-5532b609aa80
📒 Files selected for processing (6)
api/oss/src/core/access/permissions/service.pyapi/oss/src/routers/api_key_router.pyapi/oss/src/routers/user_profile.pyapi/oss/src/services/commoners.pyapi/oss/src/services/user_service.pyapi/oss/tests/pytest/unit/access/test_reset_password_idor.py
🚧 Files skipped from review as they are similar to previous changes (4)
- api/oss/src/services/commoners.py
- api/oss/src/core/access/permissions/service.py
- api/oss/src/services/user_service.py
- api/oss/src/routers/api_key_router.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
thanks for the update. the two earlier findings are fixed, but this follow-up still has one runtime failure that needs a change before qa.
…ec regression test
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 9bbc5101-ed19-4056-b6d1-6fb917f52308
📒 Files selected for processing (2)
api/oss/src/services/user_service.pyapi/oss/tests/pytest/unit/access/test_reset_password_idor.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
@mmabrouk — CI's run-api-unit-tests check is failing on this PR, but the 5 failures are all in sessions/ (test_execution_watchdog.py, test_interaction_continuation_admission.py, test_records_mapping_upsert.py), none of which I've touched in this PR's commits. Traced the root cause: a DAO change (.scalar_one_or_none() added to the sequenced-append path in dao.py, from a main merge) that the test fakes — specifically _FakeResult in test_records_mapping_upsert.py — weren't updated to implement. Same class of mock/signature drift as the user_uid issue you caught earlier, just in code outside this PR's scope. I can write the fix (add scalar_one_or_none to the fake, matching the real DAO signature) if that's useful, but wanted to check first since it's unrelated to the P0 audit this PR is meant to cover — didn't want to expand scope into sessions/ code without your call. Let me know if you'd rather this land here or in a separate PR. |
2 participants
Summary
What changed & Why it was needed:
assertstatements inuser_profile.py,permissions/service.py, anddb_manager.pywith explicitHTTPException,ValueError, andTypeErrorexceptions.assertstatements are stripped out in optimized Python environments (-O), bypassing critical authorization and input validation checks.intercept_exceptionsdecorator inexceptions.pyto usekwargs.get("request")instead ofkwargs.pop("request"). Thepop()method was destructively mutating the request dictionary, breaking downstream processes that relied on the request object.delete_api_keyendpoint inapi_key_router.pyto correctly catchsqlalchemy.exc.NoResultFoundinstead ofKeyError. This ensures the endpoint returns a proper 404 Not Found instead of a 500 Internal Server Error when deleting non-existent keys.os.getenv("AGENTA_DEMOS")incommoners.pywith the type-safeenv.agenta.extras.demosobject to align with repository standards and prevent runtime configuration issues.Testing
Verified locally
apitest suite locally usinguv run --no-sync pytestto ensure these fixes did not break existing catalog or entrypoint logic.ruff checkandruff formatto verify Python linting and code style compliance.Added or updated tests
N/A
QA follow-up
N/A
Demo
AFTER FIX :
Checklist
Contributor Resources