Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
506 changes: 506 additions & 0 deletions docs/research_page_redesign.md

Large diffs are not rendered by default.

31 changes: 31 additions & 0 deletions src/paperbot/api/routes/research.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from fastapi import APIRouter, BackgroundTasks, HTTPException, Query
from pydantic import BaseModel, Field
from sqlalchemy.exc import IntegrityError

from paperbot.context_engine import ContextEngine, ContextEngineConfig
from paperbot.context_engine.track_router import TrackRouter
Expand Down Expand Up @@ -78,6 +79,14 @@ class TrackCreateRequest(BaseModel):
activate: bool = True


class TrackUpdateRequest(BaseModel):
name: Optional[str] = Field(None, min_length=1, max_length=128)
description: Optional[str] = None
keywords: Optional[List[str]] = None
venues: Optional[List[str]] = None
methods: Optional[List[str]] = None


class TrackResponse(BaseModel):
track: Dict[str, Any]

Expand Down Expand Up @@ -124,6 +133,28 @@ def get_active_track(user_id: str = "default"):
return TrackResponse(track=track)


@router.patch("/research/tracks/{track_id}", response_model=TrackResponse)
def update_track(
track_id: int,
req: TrackUpdateRequest,
background_tasks: BackgroundTasks,
user_id: str = "default",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The API endpoint accepts a user_id parameter (defaulting to "default") which is used to identify the user for data access and modification. Since this parameter is taken from the request (query string) and not from a secure session or token, any user can impersonate another user by providing their user_id in the request. This allows unauthorized access to and modification of research tracks belonging to other users.

To remediate this, remove user_id as a request parameter. Instead, implement a secure authentication mechanism and derive the authenticated user's ID from the secure session or token on the backend.

):
update_data = req.model_dump(exclude_unset=True, exclude_none=True)

if not update_data:
raise HTTPException(status_code=400, detail="No fields to update")

try:
track = _research_store.update_track(user_id=user_id, track_id=track_id, **update_data)
except IntegrityError:
raise HTTPException(status_code=409, detail="Track name already exists") from None
if not track:
raise HTTPException(status_code=404, detail="Track not found")
_schedule_embedding_precompute(background_tasks, user_id=user_id, track_ids=[track_id])
return TrackResponse(track=track)
Comment on lines +136 to +155

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Missing tests for new PATCH endpoint and store method.

The new update_track endpoint and SqlAlchemyResearchStore.update_track introduce behavior changes but no corresponding tests are included. As per coding guidelines, "If behavior changes, add or update tests".

🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/research.py` around lines 135 - 161, Add tests
covering the new PATCH endpoint function update_track and the store method
SqlAlchemyResearchStore.update_track: write unit tests for (1) successful
partial update (verify returned TrackResponse contains updated fields and that
_schedule_embedding_precompute/background task is triggered with the correct
track_id), (2) empty update payload returns HTTP 400, and (3) non-existent track
returns HTTP 404; for the store method add tests for updating individual fields
and for returning None when track_id not found. In tests, call the router path
that hits update_track and mock/spy the _research_store.update_track and
_schedule_embedding_precompute (or inject a test BackgroundTasks) to assert they
are invoked with expected arguments; also include input fixtures for
TrackUpdateRequest variations to cover name, description, keywords, venues, and
methods updates.



@router.post("/research/tracks/{track_id}/activate", response_model=TrackResponse)
def activate_track(track_id: int, background_tasks: BackgroundTasks, user_id: str = "default"):
track = _research_store.activate_track(user_id=user_id, track_id=track_id)
Expand Down
42 changes: 42 additions & 0 deletions src/paperbot/infrastructure/stores/research_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,48 @@ def activate_track(self, *, user_id: str, track_id: int) -> Optional[Dict[str, A
session.refresh(row)
return self._track_to_dict(row)

def update_track(
self,
*,
user_id: str,
track_id: int,
name: Optional[str] = None,
description: Optional[str] = None,
keywords: Optional[List[str]] = None,
venues: Optional[List[str]] = None,
methods: Optional[List[str]] = None,
) -> Optional[Dict[str, Any]]:
now = _utcnow()
with self._provider.session() as session:
row = session.execute(
select(ResearchTrackModel).where(
ResearchTrackModel.user_id == user_id, ResearchTrackModel.id == track_id
)
).scalar_one_or_none()
if row is None:
return None

if name is not None:
row.name = name.strip()
if description is not None:
row.description = description.strip()
if keywords is not None:
row.keywords_json = _dump_list(keywords)
if venues is not None:
row.venues_json = _dump_list(venues)
if methods is not None:
row.methods_json = _dump_list(methods)

row.updated_at = now
session.add(row)
try:
session.commit()
except IntegrityError:
session.rollback()
raise
session.refresh(row)
return self._track_to_dict(row)
Comment on lines +182 to +222

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

IntegrityError not handled when renaming to a duplicate name.

The research_tracks table has a UniqueConstraint("user_id", "name"). If a user updates a track's name to one that already exists, the session.commit() on line 216 will raise an unhandled IntegrityError, resulting in a 500 response. The existing create_track method handles this case explicitly.

Suggested fix
             row.updated_at = now
             session.add(row)
-            session.commit()
-            session.refresh(row)
-            return self._track_to_dict(row)
+            try:
+                session.commit()
+            except IntegrityError:
+                session.rollback()
+                return None  # or raise a more specific error
+            session.refresh(row)
+            return self._track_to_dict(row)

Note: Returning None would map to a 404 at the API layer, which isn't ideal for a uniqueness conflict. You may want to raise a distinct exception that the route can catch and return as a 409 Conflict instead.

🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 182 - 218,
The update_track method can raise an unhandled sqlalchemy.exc.IntegrityError
when changing a track name to one that already exists; modify update_track to
catch IntegrityError around session.commit(), call session.rollback() on error,
and then raise the same kind of domain-level exception used in create_track (or
a new DuplicateTrackNameError) so the API layer can map it to a 409 Conflict
instead of returning a 500; reference the update_track function, the
session.commit() call, and the create_track handling as the model to follow.


def archive_track(self, *, user_id: str, track_id: int, archived: bool = True) -> bool:
now = _utcnow()
with self._provider.session() as session:
Expand Down
184 changes: 184 additions & 0 deletions tests/integration/test_research_track_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
from __future__ import annotations

import pytest
from fastapi.testclient import TestClient

from paperbot.api.main import app
from paperbot.infrastructure.stores.research_store import SqlAlchemyResearchStore


@pytest.fixture
def client_with_store(tmp_path, monkeypatch):
"""Create test client with isolated database."""
db_url = f"sqlite:///{tmp_path / 'test.db'}"
store = SqlAlchemyResearchStore(db_url=db_url, auto_create_schema=True)

# Monkeypatch the global store in the routes module
import paperbot.api.routes.research as research_module

monkeypatch.setattr(research_module, "_research_store", store)

return TestClient(app), store


def test_patch_track_success(client_with_store):
"""Test PATCH returns 200 with updated track."""
client, store = client_with_store

track = store.create_track(user_id="test", name="Original", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test", json={"name": "Updated"}
)

assert response.status_code == 200
assert response.json()["track"]["name"] == "Updated"


def test_patch_track_not_found(client_with_store):
"""Test PATCH returns 404 for non-existent track."""
client, _ = client_with_store

response = client.patch(
"/api/research/tracks/99999?user_id=test", json={"name": "Test"}
)

assert response.status_code == 404
assert "not found" in response.json()["detail"].lower()


def test_patch_track_duplicate_name(client_with_store):
"""Test PATCH returns 409 when name conflicts."""
client, store = client_with_store

store.create_track(user_id="test", name="Track A", activate=False)
track_b = store.create_track(user_id="test", name="Track B", activate=False)

response = client.patch(
f"/api/research/tracks/{track_b['id']}?user_id=test", json={"name": "Track A"}
)

assert response.status_code == 409
assert "already exists" in response.json()["detail"].lower()


def test_patch_track_no_fields(client_with_store):
"""Test PATCH returns 400 when no fields provided."""
client, store = client_with_store

track = store.create_track(user_id="test", name="Test", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test", json={}
)

assert response.status_code == 400
assert "no fields" in response.json()["detail"].lower()


def test_patch_track_partial_update(client_with_store):
"""Test PATCH updates only specified fields."""
client, store = client_with_store

track = store.create_track(
user_id="test",
name="Original Name",
description="Original Description",
keywords=["keyword1", "keyword2"],
activate=False,
)

# Update only name
response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test",
json={"name": "New Name"},
)

assert response.status_code == 200
result = response.json()["track"]
assert result["name"] == "New Name"
# Other fields should remain unchanged
assert result["description"] == "Original Description"
assert result["keywords"] == ["keyword1", "keyword2"]


def test_patch_track_update_description(client_with_store):
"""Test PATCH can update description."""
client, store = client_with_store

track = store.create_track(user_id="test", name="Test Track", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test",
json={"description": "New description"},
)

assert response.status_code == 200
assert response.json()["track"]["description"] == "New description"


def test_patch_track_update_keywords(client_with_store):
"""Test PATCH can update keywords list."""
client, store = client_with_store

track = store.create_track(user_id="test", name="Test Track", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test",
json={"keywords": ["ml", "nlp", "transformers"]},
)

assert response.status_code == 200
assert response.json()["track"]["keywords"] == ["ml", "nlp", "transformers"]


def test_patch_track_update_multiple_fields(client_with_store):
"""Test PATCH can update multiple fields at once."""
client, store = client_with_store

track = store.create_track(user_id="test", name="Original", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=test",
json={
"name": "Updated Name",
"description": "Updated Description",
"keywords": ["new", "keywords"],
},
)

assert response.status_code == 200
result = response.json()["track"]
assert result["name"] == "Updated Name"
assert result["description"] == "Updated Description"
assert result["keywords"] == ["new", "keywords"]


def test_patch_track_wrong_user(client_with_store):
"""Test PATCH returns 404 for track owned by different user."""
client, store = client_with_store

track = store.create_track(user_id="user1", name="Test Track", activate=False)

response = client.patch(
f"/api/research/tracks/{track['id']}?user_id=user2",
json={"name": "New Name"},
)

assert response.status_code == 404


def test_patch_track_default_user(client_with_store):
"""Test PATCH uses default user_id when not specified."""
client, store = client_with_store

track = store.create_track(user_id="default", name="Test Track", activate=False)

# Call without user_id parameter (should default to "default")
response = client.patch(
f"/api/research/tracks/{track['id']}",
json={"name": "Updated Name"},
)

assert response.status_code == 200
assert response.json()["track"]["name"] == "Updated Name"
Loading
Loading