Fix IDOR in GET /api/users/{user_id} — email leaked to any authenticated user , Closes #25 - #40
Conversation
…d workspace membership
Any authenticated user could fetch any other user's email address and
verification status by guessing/observing their UUID, with no check
that the requester shares a workspace with the target.
- Add PublicUserResponse schema with reduced fields (id, username,
display_name, avatar_url)
- Add shares_workspace() to workspaces/repository.py
- Gate GET /api/users/{user_id} to return full UserResponse only for
self-lookups and shared-workspace members; PublicUserResponse
otherwise
Fixes Devlaner#25
Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesUser Profile Privacy
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant get_user
participant shares_workspace
participant Database
participant UserPresenter
Client->>get_user: GET /users/{user_id}
get_user->>shares_workspace: Check requester and target
shares_workspace->>Database: Query workspace memberships
Database-->>shares_workspace: Shared membership result
alt Shared workspace
shares_workspace-->>get_user: True
get_user->>UserPresenter: Build UserResponse
UserPresenter-->>Client: Full profile
else No shared workspace
shares_workspace-->>get_user: False
get_user->>UserPresenter: Build PublicUserResponse
UserPresenter-->>Client: Reduced profile
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
🤖 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 `@api/app/app/modules/users/presenter.py`:
- Around line 43-49: Update to_public_user_response and its display-name helper
display_name_of so public responses never derive display_name from user.email;
when first_name and last_name are absent, return a non-sensitive fallback such
as the username or a generic value, while preserving normal name formatting and
ensuring PublicUserResponse does not expose any email-derived data.
In `@api/app/app/modules/users/router.py`:
- Around line 43-56: Add a regression test for the get_user endpoint verifying
that a user without a shared workspace receives a PublicUserResponse when
retrieving another user, and that the response excludes email and
email_verified. Cover the cross-workspace case through the existing
authentication, database, and API test fixtures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f6dbcf36-a5c1-48ee-ad3f-4b5a5122ac4d
📒 Files selected for processing (4)
api/app/app/modules/users/presenter.pyapi/app/app/modules/users/router.pyapi/app/app/modules/users/schemas.pyapi/app/app/modules/workspaces/repository.py
|
@SankeerthNara fix the comments and tag me please |
nazarli-shabnam
left a comment
nazarli-shabnam
left a comment
There was a problem hiding this comment.
Nice fix overall, this closes the direct email-by-ID enumeration in #25. Two things I need addressed before merge though:
1. to_public_user_response still leaks email via display_name — api/app/app/modules/users/presenter.py:47 calls display_name_of(user), and that function (line 7-15) falls back to user.email.split("@", 1)[0] whenever first_name/last_name are blank. UserCreate.first_name/last_name aren't validated as non-empty, so any user who registers with first_name=""/last_name="" still has their email's local-part exposed in the "public" response to strangers — which is the exact class of leak #25 was filed for, just one hop removed. The public path should never derive from email; fall back to username instead (or something that isn't PII-derived).
2. No regression test, and there's no fixture to write one with — conftest.py only has a bare TestClient, no db session or user/workspace factories. For a security-sensitive endpoint like this I don't want to merge on manual curl verification alone — next refactor of shares_workspace or the response model silently reopens the hole with nothing to catch it. Please add a minimal db-backed fixture (session + a couple of user/workspace helpers) and a test asserting: no shared workspace → no email/email_verified/no email-derived display_name in the response; shared workspace or self → full profile.
Requesting changes for these two — happy to re-review once they're in.
Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
|
hey @nazarli-shabnam, i fixed the changes you pointed out. |
There was a problem hiding this comment.
Re-reviewed after c85d63c.
1. Email-leak-via-display_name — fixed. public_display_name_of (presenter.py) no longer touches user.email at all, falls back to username. Confirmed no other field in PublicUserResponse/to_public_user_response derives from email or the private names. Good.
2. Test fixture / regression coverage — still not addressed. conftest.py is unchanged, still just the bare TestClient, no db session or user/workspace factories, and no test was added for this endpoint. This was the other blocking point last round — still need it before merge, same as before: a minimal db-backed fixture plus a test pinning "no shared workspace → reduced profile, no email" and "self/shared workspace → full profile," so the next change to shares_workspace or the response model can't silently reopen #25.
Leaving this as requesting changes until the test lands - happy to approve as soon as it's in.
@SankeerthNara
…evlaner#25) Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
|
Added regression tests in tests/test_users.py covering: no-shared-workspace hides email, self-lookup and shared-workspace both return full profile, and a dedicated test for the display_name email-leak fix. Added minimal DB fixtures in conftest.py (transactional per-test session against a loomy_test Postgres DB, plus a get_current_user override for auth). All 42 tests pass, ruff clean, mypy clean (same 5 pre-existing unrelated errors as before this PR). |
|
hey @nazarli-shabnam , i think i fixed the requested changes. |
nazarli-shabnam
left a comment
nazarli-shabnam
left a comment
There was a problem hiding this comment.
Re-reviewed after d6e3f1d.
Regression tests — good coverage. test_users.py covers the four cases that matter: no shared workspace hides email, self-lookup and shared-workspace both return the full profile, and the display_name-from-email regression specifically. Thanks for adding these.
New blocker: the test suite now requires a live Postgres, and CI doesn't have one. conftest.py's db fixture connects to postgresql://postgres:postgres@localhost:15432/loomy_test, and client now depends on db. .github/workflows/api-ci.yml runs uv run pytest on a bare ubuntu-latest runner — no services: block, no loomy_test database ever created. This doesn't just affect the new tests: test_api_auth.py and test_ws_auth_handshake.py already use client, so as it stands this change breaks the entire suite in CI, not just adds coverage locally. Needs a Postgres service (and db creation/migration step) wired into api-ci.yml, or the fixture needs to fall back to something that doesn't need real infra (e.g. sqlite for these tests, or a testcontainers-style spin-up) — whichever this repo's pattern is elsewhere.
Also, unrelated nit while I was in here: double-checked the PR description's mypy claim ("5 pre-existing errors in app/core/redis.py and venv-bundled stubs, unrelated to this PR") — ran uv run mypy . and uv run ruff check . fresh against main, the PR's actual merge-base, and this PR's head. All three come back completely clean (0 errors) on both. Not blocking, just flagging so the description doesn't stay misleading if this lands.
Requesting changes again for the CI/Postgres gap — everything else here looks solid.
Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
nazarli-shabnam
left a comment
nazarli-shabnam
left a comment
There was a problem hiding this comment.
thanks for the contribution!
|
Are there any changes to make @nazarli-shabnam |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/api-ci.yml (1)
9-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a
permissions:block to restrict theGITHUB_TOKENscope.The
lint-and-testjob has no explicitpermissions:block, so the workflow uses the repository's default token permissions — which may include write access. Since this job only runs linting, migrations, and tests, it only needs read access to the repository contents.🔒 Proposed fix: add minimal permissions block
lint-and-test: runs-on: ubuntu-latest + permissions: + contents: read defaults: run: working-directory: api/app🤖 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 @.github/workflows/api-ci.yml around lines 9 - 13, Add a permissions block to the lint-and-test job in the workflow, granting only contents: read for the GITHUB_TOKEN. Keep the restriction scoped to the lint-and-test job and leave its existing run configuration unchanged.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.github/workflows/api-ci.yml:
- Around line 9-13: Add a permissions block to the lint-and-test job in the
workflow, granting only contents: read for the GITHUB_TOKEN. Keep the
restriction scoped to the lint-and-test job and leave its existing run
configuration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8d54acc-8903-4fce-a1ec-2704e0677671
📒 Files selected for processing (5)
.github/workflows/api-ci.ymlapi/app/app/modules/users/presenter.pyapi/app/tests/__init__.pyapi/app/tests/conftest.pyapi/app/tests/test_users.py
|
Sorry i pressed re-review by mistake |
|
@SankeerthNara fix the coderabbit comment and you are good to go |
Signed-off-by: SankeerthNara <sankeerthnara@gmail.com>
3 participants
Summary
Any authenticated user could fetch any other user's email address and verification status by guessing/observing their UUID on
GET /api/users/{user_id}, with no check that the requester shares a workspace with the target.Related issues
Closes #25
Type of change
Areas touched
api/app/)apps/frontend/)Database changes
N/A — no model changes, only response schema and query-layer additions.
Implementation notes
PublicUserResponseschema with reduced fields (id,username,display_name,avatar_url) — deliberately omitsemail/email_verified/ names.shares_workspace(db, user_id_a, user_id_b)toworkspaces/repository.py. Treats self-lookup as trivially "shared," and checksWorkspaceMemberfor any common workspace (owners are members too, viacreate(), so no separate owner check is needed).GET /api/users/{user_id}now returns fullUserResponseonly for self-lookups or shared-workspace members; everyone else getsPublicUserResponsevia aUnionresponse model.usersmodule didn't have DB-backed integration test fixtures yet (only unit-style tests exist elsewhere, e.g.test_jwt.py). Verified manually instead (see below). Happy to follow up with test scaffolding in a separate PR if useful.How to test
Start deps and apply migrations:
Create two users (A, B) via
POST /api/users, and log in as A viaPOST /api/auth/login.No shared workspace — confirm email is hidden:
Self-lookup — confirm full profile still returns:
Add B to a workspace owned by A via
POST /api/workspaces/{workspace_id}/members, then repeat step 3:# expect: full profile including email, now that they share a workspace
Checklist
main.router → service → repository) for backend changes.uv run mypy .passes for all code touched by this change (5 pre-existing errors remain inapp/core/redis.pyand venv-bundled stubs, unrelated to this PR).uv run ruff check .passes.uv run pytestpasses (37 passed).npm run lint,npm run format:check, andnpm run buildpass (for frontend changes).src/i18n/(for frontend changes)..envfiles are committed.Screenshots / recordings
N/A — backend-only API change.
Summary by CodeRabbit
GET /users/{user_id}to return full details only when viewing your own profile or when both users share a workspace; otherwise it returns the public profile.