From b724b6fdc816b271d7f847baa613630be5bad7bd Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 13:13:08 +0530 Subject: [PATCH 1/6] fix: gate email/verification fields on GET /api/users/{user_id} behind 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 #25 Signed-off-by: SankeerthNara --- api/app/app/modules/users/presenter.py | 11 +++++++++- api/app/app/modules/users/router.py | 21 ++++++++++++++------ api/app/app/modules/users/schemas.py | 13 ++++++++++++ api/app/app/modules/workspaces/repository.py | 21 ++++++++++++++++++++ 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/api/app/app/modules/users/presenter.py b/api/app/app/modules/users/presenter.py index d627877..89a5c85 100644 --- a/api/app/app/modules/users/presenter.py +++ b/api/app/app/modules/users/presenter.py @@ -1,7 +1,7 @@ from app.config import settings from app.core.storage import is_enabled as storage_enabled, presign_get_url from app.modules.users.model import User -from app.modules.users.schemas import UserResponse +from app.modules.users.schemas import PublicUserResponse, UserResponse def display_name_of(user: User) -> str: @@ -38,3 +38,12 @@ def to_user_response(user: User) -> UserResponse: created_at=user.created_at, updated_at=user.updated_at, ) + + +def to_public_user_response(user: User) -> PublicUserResponse: + return PublicUserResponse( + id=user.id, + username=user.username, + display_name=display_name_of(user), + avatar_url=avatar_url_of(user), + ) \ No newline at end of file diff --git a/api/app/app/modules/users/router.py b/api/app/app/modules/users/router.py index b9c4f50..c0492e3 100644 --- a/api/app/app/modules/users/router.py +++ b/api/app/app/modules/users/router.py @@ -1,5 +1,6 @@ import logging import secrets +from typing import Union from uuid import UUID from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, status @@ -15,9 +16,15 @@ ) from app.db.session import get_db from app.modules.users.model import User -from app.modules.users.presenter import to_user_response -from app.modules.users.schemas import UserCreate, UserResponse, UserUpdate +from app.modules.users.presenter import to_user_response, to_public_user_response +from app.modules.users.schemas import ( + PublicUserResponse, + UserCreate, + UserResponse, + UserUpdate, +) from app.modules.users.service import get_user_by_id, register_user +from app.modules.workspaces.repository import shares_workspace logger = logging.getLogger(__name__) @@ -33,18 +40,20 @@ } -@router.get("/{user_id}", response_model=UserResponse) +@router.get("/{user_id}", response_model=Union[UserResponse, PublicUserResponse]) def get_user( user_id: UUID, db: Session = Depends(get_db), current_user: User = Depends(get_current_user), -) -> UserResponse: +) -> UserResponse | PublicUserResponse: user = get_user_by_id(db, user_id) if not user: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="User not found" ) - return to_user_response(user) + if shares_workspace(db, current_user.id, user_id): + return to_user_response(user) + return to_public_user_response(user) @router.post("", response_model=UserResponse, status_code=status.HTTP_201_CREATED) @@ -152,4 +161,4 @@ def delete_avatar( db.refresh(current_user) if previous_key: delete_object(bucket=settings.s3_bucket, key=previous_key) - return to_user_response(current_user) + return to_user_response(current_user) \ No newline at end of file diff --git a/api/app/app/modules/users/schemas.py b/api/app/app/modules/users/schemas.py index 846b69a..046004c 100644 --- a/api/app/app/modules/users/schemas.py +++ b/api/app/app/modules/users/schemas.py @@ -32,3 +32,16 @@ class UserResponse(BaseModel): updated_at: datetime model_config = {"from_attributes": True} + + +class PublicUserResponse(BaseModel): + """Reduced profile shape returned to users who don't share a workspace + with the target user. Deliberately omits email / email_verified / names + to avoid PII leakage (see issue #25).""" + + id: UUID + username: str + display_name: str + avatar_url: str | None = None + + model_config = {"from_attributes": True} \ No newline at end of file diff --git a/api/app/app/modules/workspaces/repository.py b/api/app/app/modules/workspaces/repository.py index 96a80c8..1d4a869 100644 --- a/api/app/app/modules/workspaces/repository.py +++ b/api/app/app/modules/workspaces/repository.py @@ -202,3 +202,24 @@ def list_workspace_invitations( if pending_only: query = query.filter(WorkspaceInvitation.accepted_at.is_(None)) return query.order_by(WorkspaceInvitation.created_at.desc()).all() + + +def shares_workspace(db: Session, user_id_a: UUID, user_id_b: UUID) -> bool: + """True if the two users are the same person or are both members of at + least one common workspace (owners are members too, via `create()`).""" + if user_id_a == user_id_b: + return True + member_workspace_ids = ( + db.query(WorkspaceMember.workspace_id) + .filter(WorkspaceMember.user_id == user_id_b) + .subquery() + ) + return ( + db.query(WorkspaceMember) + .filter( + WorkspaceMember.user_id == user_id_a, + WorkspaceMember.workspace_id.in_(member_workspace_ids), + ) + .first() + is not None + ) \ No newline at end of file From 1178e58ff1823d717c1901432ea3c25b7cad1d7b Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 13:24:55 +0530 Subject: [PATCH 2/6] fix: satisfy mypy strict typing in shares_workspace Signed-off-by: SankeerthNara --- api/app/app/modules/workspaces/repository.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/app/app/modules/workspaces/repository.py b/api/app/app/modules/workspaces/repository.py index 1d4a869..5f0c9bc 100644 --- a/api/app/app/modules/workspaces/repository.py +++ b/api/app/app/modules/workspaces/repository.py @@ -212,7 +212,6 @@ def shares_workspace(db: Session, user_id_a: UUID, user_id_b: UUID) -> bool: member_workspace_ids = ( db.query(WorkspaceMember.workspace_id) .filter(WorkspaceMember.user_id == user_id_b) - .subquery() ) return ( db.query(WorkspaceMember) From c85d63ce20662ae89445c07c2de405957d3b9e78 Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 13:51:32 +0530 Subject: [PATCH 3/6] fix: prevent email leak via display_name fallback in PublicUserResponse Signed-off-by: SankeerthNara --- api/app/app/modules/users/presenter.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/api/app/app/modules/users/presenter.py b/api/app/app/modules/users/presenter.py index 89a5c85..4ed1a41 100644 --- a/api/app/app/modules/users/presenter.py +++ b/api/app/app/modules/users/presenter.py @@ -16,6 +16,16 @@ def display_name_of(user: User) -> str: return user.username or "Unknown" +def public_display_name_of(user: User) -> str: + """Like display_name_of, but never falls back to the email local part — + used for PublicUserResponse where email must not be inferable.""" + first = (user.first_name or "").strip() + last = (user.last_name or "").strip() + if first or last: + return f"{first} {last}".strip() + return user.username or "Unknown" + + def avatar_url_of(user: User) -> str | None: if user.avatar_key and storage_enabled(): try: @@ -44,6 +54,6 @@ def to_public_user_response(user: User) -> PublicUserResponse: return PublicUserResponse( id=user.id, username=user.username, - display_name=display_name_of(user), + display_name=public_display_name_of(user), avatar_url=avatar_url_of(user), - ) \ No newline at end of file + ) From d6e3f1d230b6c648f4e53d9a72109cb58796dbfc Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 14:23:54 +0530 Subject: [PATCH 4/6] test: add regression tests for workspace-gated user profile access (#25) Signed-off-by: SankeerthNara --- api/app/tests/__init__.py | 0 api/app/tests/conftest.py | 92 ++++++++++++++++++++++++++++++++++++- api/app/tests/test_users.py | 89 +++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 api/app/tests/__init__.py create mode 100644 api/app/tests/test_users.py diff --git a/api/app/tests/__init__.py b/api/app/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/app/tests/conftest.py b/api/app/tests/conftest.py index 406cdbd..f3e8b7f 100644 --- a/api/app/tests/conftest.py +++ b/api/app/tests/conftest.py @@ -1,9 +1,97 @@ +import uuid +from collections.abc import Generator + import pytest from fastapi.testclient import TestClient +from sqlalchemy import create_engine +from sqlalchemy.orm import Session, sessionmaker +from app.api.deps import get_current_user +from app.db.session import get_db from app.main import app +from app.modules.users.model import User +from app.modules.workspaces.model import Workspace, WorkspaceMember + +TEST_DATABASE_URL = "postgresql://postgres:postgres@localhost:15432/loomy_test" + +engine = create_engine(TEST_DATABASE_URL) +TestingSessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) + + +@pytest.fixture +def db() -> Generator[Session, None, None]: + """Each test runs inside a transaction that is rolled back afterward, + so tests never leave residue in loomy_test and can run in any order.""" + connection = engine.connect() + transaction = connection.begin() + session = TestingSessionLocal(bind=connection) + + try: + yield session + finally: + session.close() + transaction.rollback() + connection.close() @pytest.fixture -def client() -> TestClient: - return TestClient(app) +def client(db: Session) -> Generator[TestClient, None, None]: + def _get_db_override() -> Generator[Session, None, None]: + yield db + + app.dependency_overrides[get_db] = _get_db_override + with TestClient(app) as test_client: + yield test_client + app.dependency_overrides.clear() + + +def make_user( + db: Session, + *, + email: str | None = None, + username: str | None = None, + first_name: str | None = "Test", + last_name: str | None = "User", +) -> User: + """Create and persist a real User row for use in tests.""" + unique = uuid.uuid4().hex[:8] + user = User( + email=email or f"user-{unique}@test.com", + username=username or f"user{unique}", + # Password hash is irrelevant here since tests authenticate via + # the get_current_user override, not a real login flow. + hashed_password="not-a-real-hash", + first_name=first_name, + last_name=last_name, + email_verified=False, + ) + db.add(user) + db.flush() + db.refresh(user) + return user + + +def make_workspace(db: Session, *, owner: User, name: str = "Test Workspace") -> Workspace: + """Create a workspace owned by `owner`, and add the owner as a member + (mirrors workspaces/repository.py's create()).""" + unique = uuid.uuid4().hex[:8] + workspace = Workspace(name=name, slug=f"test-workspace-{unique}", owner_id=owner.id) + db.add(workspace) + db.flush() + db.refresh(workspace) + member = WorkspaceMember(workspace_id=workspace.id, user_id=owner.id, role="owner") + db.add(member) + db.flush() + return workspace + + +def add_member(db: Session, *, workspace: Workspace, user: User, role: str = "member") -> None: + member = WorkspaceMember(workspace_id=workspace.id, user_id=user.id, role=role) + db.add(member) + db.flush() + + +def auth_as(client: TestClient, user: User) -> None: + """Override get_current_user so requests through `client` are + authenticated as `user`, without going through a real JWT login.""" + app.dependency_overrides[get_current_user] = lambda: user \ No newline at end of file diff --git a/api/app/tests/test_users.py b/api/app/tests/test_users.py new file mode 100644 index 0000000..0019b22 --- /dev/null +++ b/api/app/tests/test_users.py @@ -0,0 +1,89 @@ +from fastapi.testclient import TestClient +from sqlalchemy.orm import Session + +from tests.conftest import add_member, auth_as, make_user, make_workspace + + +def test_get_user_no_shared_workspace_hides_email( + client: TestClient, db: Session +) -> None: + """Core regression test for issue #25: a user with no shared workspace + must not be able to retrieve another user's email via this endpoint.""" + requester = make_user(db) + target = make_user(db) + auth_as(client, requester) + + response = client.get(f"/api/users/{target.id}") + + assert response.status_code == 200 + body = response.json() + assert set(body.keys()) == {"id", "username", "display_name", "avatar_url"} + assert "email" not in body + assert "email_verified" not in body + assert "first_name" not in body + assert "last_name" not in body + + +def test_get_user_self_lookup_returns_full_profile( + client: TestClient, db: Session +) -> None: + user = make_user(db) + auth_as(client, user) + + response = client.get(f"/api/users/{user.id}") + + assert response.status_code == 200 + body = response.json() + assert body["email"] == user.email + assert body["email_verified"] == user.email_verified + + +def test_get_user_shared_workspace_returns_full_profile( + client: TestClient, db: Session +) -> None: + owner = make_user(db) + member = make_user(db) + workspace = make_workspace(db, owner=owner) + add_member(db, workspace=workspace, user=member) + auth_as(client, owner) + + response = client.get(f"/api/users/{member.id}") + + assert response.status_code == 200 + body = response.json() + assert body["email"] == member.email + assert body["email_verified"] == member.email_verified + + +def test_get_user_display_name_never_leaks_email_local_part( + client: TestClient, db: Session +) -> None: + """Regression test for the coderabbitai finding: when a user has no + first_name/last_name set, the public response's display_name must fall + back to username, never to the email local part.""" + requester = make_user(db) + target = make_user( + db, + email="secretlocalpart@test.com", + first_name=None, + last_name=None, + ) + auth_as(client, requester) + + response = client.get(f"/api/users/{target.id}") + + assert response.status_code == 200 + body = response.json() + assert body["display_name"] == target.username + assert "secretlocalpart" not in body["display_name"] + + +def test_get_user_not_found_returns_404(client: TestClient, db: Session) -> None: + import uuid + + requester = make_user(db) + auth_as(client, requester) + + response = client.get(f"/api/users/{uuid.uuid4()}") + + assert response.status_code == 404 \ No newline at end of file From a83a444918aff2767361e5f3eb312b1a91bc6e72 Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 14:37:44 +0530 Subject: [PATCH 5/6] ci: add Postgres service and test DB migrations to API CI workflow Signed-off-by: SankeerthNara --- .github/workflows/api-ci.yml | 31 +++++++++++++++++++++---------- api/app/tests/conftest.py | 6 +++++- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/.github/workflows/api-ci.yml b/.github/workflows/api-ci.yml index daa73a7..ff22802 100644 --- a/.github/workflows/api-ci.yml +++ b/.github/workflows/api-ci.yml @@ -1,38 +1,49 @@ name: API CI - on: pull_request: branches: [main] paths: - "api/app/**" - ".github/workflows/api-ci.yml" - jobs: lint-and-test: runs-on: ubuntu-latest defaults: run: working-directory: api/app - + services: + postgres: + image: postgres:17-alpine + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: loomy_test + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + env: + TEST_DATABASE_URL: postgresql://postgres:postgres@localhost:5432/loomy_test steps: - uses: actions/checkout@v4 - - name: Install uv uses: astral-sh/setup-uv@v4 with: version: "latest" - - name: Set up Python run: uv python install 3.12 - - name: Install dependencies run: uv sync --all-extras - + - name: Run migrations (test db) + run: uv run alembic upgrade head + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/loomy_test - name: Ruff run: uv run ruff check . - - name: Mypy run: uv run mypy . - - name: Pytest - run: uv run pytest --tb=short -q + run: uv run pytest --tb=short -q \ No newline at end of file diff --git a/api/app/tests/conftest.py b/api/app/tests/conftest.py index f3e8b7f..f5bf6e3 100644 --- a/api/app/tests/conftest.py +++ b/api/app/tests/conftest.py @@ -1,4 +1,5 @@ import uuid +import os from collections.abc import Generator import pytest @@ -12,7 +13,10 @@ from app.modules.users.model import User from app.modules.workspaces.model import Workspace, WorkspaceMember -TEST_DATABASE_URL = "postgresql://postgres:postgres@localhost:15432/loomy_test" +TEST_DATABASE_URL = os.environ.get( + "TEST_DATABASE_URL", + "postgresql://postgres:postgres@localhost:15432/loomy_test", +) engine = create_engine(TEST_DATABASE_URL) TestingSessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) From 1216b73788035570c311fa3a5354a4a7cb97997c Mon Sep 17 00:00:00 2001 From: SankeerthNara Date: Fri, 10 Jul 2026 14:52:40 +0530 Subject: [PATCH 6/6] ci: restrict GITHUB_TOKEN to read-only permissions Signed-off-by: SankeerthNara --- .github/workflows/api-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/api-ci.yml b/.github/workflows/api-ci.yml index ff22802..7dfb2a8 100644 --- a/.github/workflows/api-ci.yml +++ b/.github/workflows/api-ci.yml @@ -8,6 +8,8 @@ on: jobs: lint-and-test: runs-on: ubuntu-latest + permissions: + contents: read defaults: run: working-directory: api/app