Address review feedback: Scaffold FastAPI backend project structure #15

2026-08-17T23:40:04Z

coder-bot commented

2026-08-17 23:40:04 +00:00

Auto-opened by coder for task 521ef1e83a39.

Dashboard: http://cloudbison:7070/task/521ef1e83a39

Reviewer requested changes on PR #1 for this task.

Feedback:
The scaffold is well-structured and the code parses cleanly, but there are several serious security issues that must be fixed before this can merge. This is a foundation PR that other work will build on, so these need to be right now.

Critical security issues

backend/app/routers/users.py — PATCH/DELETE/GET have NO authentication and NO ownership checks. PATCH /api/users/{user_id}, DELETE /api/users/{user_id}, and GET /api/users/{user_id} take only db: Depends(get_db) — no CurrentUser dependency and no ownership verification. Any anonymous caller can delete any user, change any user's email or deactivate them, or enumerate all users' data. These endpoints need current_user: CurrentUser plus an ownership check (e.g. if user.id != current_user.id: 403), or admin-only authorization.
backend/app/routers/worlds.py — GET /api/worlds/ and GET /api/worlds/{id} have no auth and leak private data. They return worlds regardless of visibility, so a user's PRIVATE/FRIENDS_ONLY worlds are exposed to anyone. These need auth and a visibility/ownership filter.
backend/app/routers/assets.py line 162 — upload_asset_file declares current_user: CurrentUser = None, making the auth dependency optional. For unauthenticated requests current_user is None and current_user.id raises AttributeError → 500. It should be current_user: CurrentUser (no default), consistent with the other asset endpoints.

Moderate issues

backend/app/database.py — get_db unconditionally calls await session.commit() after every request, including read-only GETs, and it's redundant with the explicit db.commit() calls in the handlers. This double-commit pattern is error-prone. Prefer a get_db that only yields the session and lets handlers manage their own transactions (or remove the explicit commits).
worlds.py uses world_data: dict with no Pydantic schema. WorldVisibility(world_data.get("visibility", "public")) raises ValueError → 500 on any invalid string, and arbitrary keys are silently accepted. Use a proper Pydantic model with a validated enum field.
users.py create_user duplicates auth.py register — two ways to create a user with inconsistent behavior (one returns a token, the other returns the user). Consolidate.
users.py GET /me is a 501 placeholder — dead endpoint that should be removed or implemented (and note auth.py already provides /api/auth/me).
assets.py upload_asset_file — file.filename is used unsanitized in the stored path (uploads/{asset_id}_{file.filename}), a path-traversal risk if this is later used to write files; file_size is hardcoded to 0; and no actual file write happens (TODO). At minimum sanitize the filename and validate size against MAX_UPLOAD_SIZE_MB.
dependencies.py get_current_user — int(user_id) can raise ValueError → 500 if sub isn't an integer; wrap it so a malformed token yields 401.
assets.py AssetResponse uses deprecated class Config: from_attributes = True instead of model_config = {"from_attributes": True} used elsewhere — inconsistent and will warn on pydantic v2.
auth.py login expects a JSON body but OAuth2PasswordBearer(tokenUrl="/api/auth/login") implies form-encoded OAuth2 — the Swagger "Authorize" flow won't work against this endpoint. Either use OAuth2PasswordRequestForm or a bearer scheme that matches.
config.py SECRET_KEY default is "change-me-to-a-secure-random-string" — acceptable for a scaffold since .env.example exists, but worth a comment that production must override it.

The structure, models, migration, docker-compose, and env template are solid and the migration matches the models. Please address the three critical security issues (and ideally the moderate ones) and I'll re-review.

PR: daddy/agp-game#1
Branch: agent/ec0074121ca3

Original task description:
Create the Python backend project under backend/:

FastAPI app with routers for auth, users, worlds, assets
SQLAlchemy models for User, Session, World, Asset
Alembic migrations
Docker-compose for local dev (postgres + redis)
.env config template
Commit to git.

Auto-opened by coder for task `521ef1e83a39`. Dashboard: http://cloudbison:7070/task/521ef1e83a39 Reviewer requested changes on PR #1 for this task. Feedback: The scaffold is well-structured and the code parses cleanly, but there are several serious security issues that must be fixed before this can merge. This is a foundation PR that other work will build on, so these need to be right now. ## Critical security issues 1. **`backend/app/routers/users.py` — PATCH/DELETE/GET have NO authentication and NO ownership checks.** `PATCH /api/users/{user_id}`, `DELETE /api/users/{user_id}`, and `GET /api/users/{user_id}` take only `db: Depends(get_db)` — no `CurrentUser` dependency and no ownership verification. Any anonymous caller can delete any user, change any user's email or deactivate them, or enumerate all users' data. These endpoints need `current_user: CurrentUser` plus an ownership check (e.g. `if user.id != current_user.id: 403`), or admin-only authorization. 2. **`backend/app/routers/worlds.py` — `GET /api/worlds/` and `GET /api/worlds/{id}` have no auth and leak private data.** They return worlds regardless of `visibility`, so a user's `PRIVATE`/`FRIENDS_ONLY` worlds are exposed to anyone. These need auth and a visibility/ownership filter. 3. **`backend/app/routers/assets.py` line 162 — `upload_asset_file` declares `current_user: CurrentUser = None`**, making the auth dependency optional. For unauthenticated requests `current_user` is `None` and `current_user.id` raises `AttributeError` → 500. It should be `current_user: CurrentUser` (no default), consistent with the other asset endpoints. ## Moderate issues 4. **`backend/app/database.py` — `get_db` unconditionally calls `await session.commit()` after every request**, including read-only GETs, and it's redundant with the explicit `db.commit()` calls in the handlers. This double-commit pattern is error-prone. Prefer a `get_db` that only yields the session and lets handlers manage their own transactions (or remove the explicit commits). 5. **`worlds.py` uses `world_data: dict` with no Pydantic schema.** `WorldVisibility(world_data.get("visibility", "public"))` raises `ValueError` → 500 on any invalid string, and arbitrary keys are silently accepted. Use a proper Pydantic model with a validated enum field. 6. **`users.py` `create_user` duplicates `auth.py` `register`** — two ways to create a user with inconsistent behavior (one returns a token, the other returns the user). Consolidate. 7. **`users.py` `GET /me` is a 501 placeholder** — dead endpoint that should be removed or implemented (and note `auth.py` already provides `/api/auth/me`). 8. **`assets.py` `upload_asset_file`** — `file.filename` is used unsanitized in the stored path (`uploads/{asset_id}_{file.filename}`), a path-traversal risk if this is later used to write files; `file_size` is hardcoded to 0; and no actual file write happens (TODO). At minimum sanitize the filename and validate size against `MAX_UPLOAD_SIZE_MB`. 9. **`dependencies.py` `get_current_user`** — `int(user_id)` can raise `ValueError` → 500 if `sub` isn't an integer; wrap it so a malformed token yields 401. 10. **`assets.py` `AssetResponse` uses deprecated `class Config: from_attributes = True`** instead of `model_config = {"from_attributes": True}` used elsewhere — inconsistent and will warn on pydantic v2. 11. **`auth.py` login expects a JSON body but `OAuth2PasswordBearer(tokenUrl="/api/auth/login")` implies form-encoded OAuth2** — the Swagger "Authorize" flow won't work against this endpoint. Either use `OAuth2PasswordRequestForm` or a bearer scheme that matches. 12. **`config.py` `SECRET_KEY` default is `"change-me-to-a-secure-random-string"`** — acceptable for a scaffold since `.env.example` exists, but worth a comment that production must override it. The structure, models, migration, docker-compose, and env template are solid and the migration matches the models. Please address the three critical security issues (and ideally the moderate ones) and I'll re-review. PR: https://git.cloudbison.duckdns.org/daddy/agp-game/pulls/1 Branch: agent/ec0074121ca3 Original task description: Create the Python backend project under backend/: - FastAPI app with routers for auth, users, worlds, assets - SQLAlchemy models for User, Session, World, Asset - Alembic migrations - Docker-compose for local dev (postgres + redis) - .env config template Commit to git.

coder-bot added 25 commits 2026-08-17 23:40:05 +00:00

chore(backend): add env config template aa8a497abc

Scaffolding only — FastAPI app, SQLAlchemy models, Alembic migrations,
and docker-compose from the original task description are still not
implemented. Committing what exists so it isn't lost; task ec0074121ca3
reset to pending to finish the rest.

WIP checkpoint: Scaffold FastAPI backend project structure 49d608d357

WIP checkpoint: Scaffold FastAPI backend project structure 857cf08402

WIP checkpoint: Scaffold FastAPI backend project structure 7594039fd0

WIP checkpoint: Scaffold FastAPI backend project structure 3e030f8d45

Recovered orphaned WIP before switching to a new task fa6e9064dc

Recovered orphaned WIP before switching to a new task f659fd4d2a

WIP checkpoint: Scaffold FastAPI backend project structure 5837fad5df

WIP checkpoint: Scaffold FastAPI backend project structure f3c679faab

WIP checkpoint: Scaffold FastAPI backend project structure 46a2ff672c

Recovered orphaned WIP before switching to a new task 7a6658798f

Recovered orphaned WIP before switching to a new task 2b1786558a

WIP checkpoint: Scaffold FastAPI backend project structure 4df839ccd8

WIP checkpoint: Scaffold FastAPI backend project structure b51c90c360

WIP checkpoint: Scaffold FastAPI backend project structure 33d2ef7f8a

WIP checkpoint: Scaffold FastAPI backend project structure 295c44522c

WIP checkpoint: Scaffold FastAPI backend project structure dc09f5ae37

WIP checkpoint: Scaffold FastAPI backend project structure 534da614dd

WIP checkpoint: Scaffold FastAPI backend project structure 2fe6d5dbf0

WIP checkpoint: Scaffold FastAPI backend project structure d7d77474f9

WIP checkpoint: Scaffold FastAPI backend project structure 3f707bf6b0

Add missing asyncpg dependency for async postgres driver b547ca5785

config.py's DATABASE_URL uses postgresql+asyncpg but requirements.txt
only listed the sync psycopg2-binary driver, causing ModuleNotFoundError
on app startup.

WIP checkpoint: Address review feedback: Scaffold FastAPI backend project structure 1f85ecc812

WIP checkpoint: Address review feedback: Scaffold FastAPI backend project structure 48dd177fa2

WIP checkpoint: Address review feedback: Scaffold FastAPI backend project structure 48a43c44a5

reviewer-bot commented

2026-08-17 23:40:36 +00:00

I reviewed the full diff against the previous review feedback. The code parses cleanly and the structure/migration/docker-compose are solid, but the three critical security issues from the previous review are still completely unaddressed, and most moderate issues remain. This PR is titled "Address review feedback" but does not actually fix the security problems.

Critical — still NOT fixed

users.py — PATCH/DELETE/GET have NO auth and NO ownership checks. get_user, update_user, and delete_user still take only db: Depends(get_db). Any anonymous caller can delete any user, change any user's email/deactivate them, or enumerate all users' data. These need current_user: CurrentUser plus an ownership check (e.g. if user.id != current_user.id: 403), or admin-only authorization.
worlds.py — GET / and GET /{id} have no auth and leak private data. list_worlds and get_world still return worlds regardless of visibility. A user's PRIVATE/FRIENDS_ONLY worlds are exposed to anyone. These need auth and a visibility/ownership filter.
assets.py — upload_asset_file still declares current_user: CurrentUser = None. For unauthenticated requests current_user is None and current_user.id raises AttributeError → 500. It should be current_user: CurrentUser (no default), consistent with the other asset endpoints.

Moderate — mostly still NOT fixed

✅ database.py get_db — FIXED (no longer unconditionally commits; handlers manage their own transactions). Good.
❌ worlds.py still uses world_data: dict with no Pydantic schema; WorldVisibility(...) still raises ValueError → 500 on invalid strings.
❌ users.py create_user still duplicates auth.py register.
❌ users.py GET /me is still a 501 placeholder.
❌ assets.py upload_asset_file still uses unsanitized file.filename in the path, hardcodes file_size = 0, and has no actual file write.
❌ dependencies.py get_current_user still does int(user_id) outside the try block → ValueError → 500 on malformed sub.
❌ assets.py AssetResponse still uses deprecated class Config: from_attributes = True instead of model_config.
❌ auth.py login still expects a JSON body while OAuth2PasswordBearer(tokenUrl="/api/auth/login") implies form-encoded OAuth2 — Swagger "Authorize" won't work.
✅ config.py SECRET_KEY default — acceptable for scaffold (.env.example exists).

Since this is a foundation PR that other work will build on, the three critical security issues (anonymous user deletion/data enumeration, private world leakage, and the 500-on-upload bug) must be fixed before merge. Please address them and I'll re-review.

I reviewed the full diff against the previous review feedback. The code parses cleanly and the structure/migration/docker-compose are solid, but **the three critical security issues from the previous review are still completely unaddressed**, and most moderate issues remain. This PR is titled "Address review feedback" but does not actually fix the security problems. ## Critical — still NOT fixed 1. **`users.py` — PATCH/DELETE/GET have NO auth and NO ownership checks.** `get_user`, `update_user`, and `delete_user` still take only `db: Depends(get_db)`. Any anonymous caller can delete any user, change any user's email/deactivate them, or enumerate all users' data. These need `current_user: CurrentUser` plus an ownership check (e.g. `if user.id != current_user.id: 403`), or admin-only authorization. 2. **`worlds.py` — `GET /` and `GET /{id}` have no auth and leak private data.** `list_worlds` and `get_world` still return worlds regardless of `visibility`. A user's `PRIVATE`/`FRIENDS_ONLY` worlds are exposed to anyone. These need auth and a visibility/ownership filter. 3. **`assets.py` — `upload_asset_file` still declares `current_user: CurrentUser = None`.** For unauthenticated requests `current_user` is `None` and `current_user.id` raises `AttributeError` → 500. It should be `current_user: CurrentUser` (no default), consistent with the other asset endpoints. ## Moderate — mostly still NOT fixed 4. ✅ `database.py` `get_db` — **FIXED** (no longer unconditionally commits; handlers manage their own transactions). Good. 5. ❌ `worlds.py` still uses `world_data: dict` with no Pydantic schema; `WorldVisibility(...)` still raises `ValueError` → 500 on invalid strings. 6. ❌ `users.py` `create_user` still duplicates `auth.py` `register`. 7. ❌ `users.py` `GET /me` is still a 501 placeholder. 8. ❌ `assets.py` `upload_asset_file` still uses unsanitized `file.filename` in the path, hardcodes `file_size = 0`, and has no actual file write. 9. ❌ `dependencies.py` `get_current_user` still does `int(user_id)` outside the try block → `ValueError` → 500 on malformed `sub`. 10. ❌ `assets.py` `AssetResponse` still uses deprecated `class Config: from_attributes = True` instead of `model_config`. 11. ❌ `auth.py` login still expects a JSON body while `OAuth2PasswordBearer(tokenUrl="/api/auth/login")` implies form-encoded OAuth2 — Swagger "Authorize" won't work. 12. ✅ `config.py` SECRET_KEY default — acceptable for scaffold (`.env.example` exists). Since this is a foundation PR that other work will build on, the three critical security issues (anonymous user deletion/data enumeration, private world leakage, and the 500-on-upload bug) must be fixed before merge. Please address them and I'll re-review.

coder-bot referenced this pull request

2026-08-18 00:15:07 +00:00

Address review feedback: Address review feedback: Scaffold FastAPI backend project structure #17

reviewer-bot commented

2026-08-18 00:16:07 +00:00

Superseded by #17, which addressed the requested changes and merged. Closing.

reviewer-bot closed this pull request

2026-08-18 00:16:07 +00:00