backend: env config template (partial — scaffold incomplete) #1

Closed
agent-bot wants to merge 22 commits from agent/ec0074121ca3 into main
Collaborator

Salvaged from an early agent run that only got as far as .env/.env.example before incorrectly marking the task done.

Still missing (per original task ec0074121ca3, reset to pending):

  • 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 itself is gitignored — only .env.example is committed.

Salvaged from an early agent run that only got as far as `.env`/`.env.example` before incorrectly marking the task done. **Still missing** (per original task ec0074121ca3, reset to pending): - 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` itself is gitignored — only `.env.example` is committed.
agent-bot force-pushed agent/ec0074121ca3 from 26092f36db to ba4e714d2e 2026-08-17 17:47:30 +00:00 Compare
architect-bot force-pushed agent/ec0074121ca3 from ba4e714d2e to f547bf1b1a 2026-08-17 18:02:50 +00:00 Compare
coder-bot added 13 commits 2026-08-17 19:20:11 +00:00
coder-bot force-pushed agent/ec0074121ca3 from 2ebf44262d to 72a994c5d1 2026-08-17 19:20:11 +00:00 Compare
coder-bot added 1 commit 2026-08-17 19:59:00 +00:00
coder-bot added 1 commit 2026-08-17 20:18:35 +00:00
coder-bot added 1 commit 2026-08-17 20:22:36 +00:00
coder-bot added 1 commit 2026-08-17 20:25:09 +00:00
coder-bot added 1 commit 2026-08-17 20:25:29 +00:00
coder-bot added 1 commit 2026-08-17 20:25:45 +00:00
coder-bot added 1 commit 2026-08-17 20:26:28 +00:00
coder-bot added 1 commit 2026-08-17 20:42:44 +00:00
coder-bot added 1 commit 2026-08-17 20:59:58 +00:00
config.py's DATABASE_URL uses postgresql+asyncpg but requirements.txt
only listed the sync psycopg2-binary driver, causing ModuleNotFoundError
on app startup.
Collaborator

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

  1. 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).

  2. 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.

  3. 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.

  4. 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).

  5. 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.

  6. 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.

  7. 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.

  8. 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.

  9. 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.

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.
Author
Collaborator

Superseded by a later review-feedback round that already merged. Closing — found while auditing why this PR was still sitting open (a chain-tracking bug meant multi-round review-feedback chains only ever closed the immediately prior PR, not the whole ancestry; fixed in agp's tools/definitions.py).

Superseded by a later review-feedback round that already merged. Closing — found while auditing why this PR was still sitting open (a chain-tracking bug meant multi-round review-feedback chains only ever closed the immediately prior PR, not the whole ancestry; fixed in agp's tools/definitions.py).
agent-bot closed this pull request 2026-08-18 15:54:55 +00:00

Pull request closed

Sign in to join this conversation.
No Reviewers
No Label
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: daddy/modbison#1