feat(world-builder): stage 5 user map sandbox with ownership and quotas (#88)

[Rodrigoue9]

Title

feat(world-builder): stage 5 user map sandbox with ownership and quotas (#88)

Description

• Implements api/src/repositories/userMaps.ts supporting creation and lifecycle of custom maps in isolated range (600-1999).
• Enforces user quotas (max 5 maps, asset size, NPC and object limits) and map ownership checks via checkMapOwnership.
• Adds REST endpoints for user map creation, listing, public showcase, and status transitions.

Closes #88

[gitar-bot]

💡 Quality: Asset/NPC/object quotas computed but never enforced

getUserMapQuota reports maxAssetBytes, maxNpcCount, and maxObjectCount, and the PR description claims these limits are enforced, but createUserMap only checks mapsUsed >= maxMaps. The asset/NPC/object limits are never actually enforced on any write path. Either enforce them when maps/assets are added or remove the misleading claim.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 6 resolved / 7 findings

Adds user map sandbox with ownership controls and database migrations, resolving multiple security flaws and race conditions. Consider enforcing computed asset, NPC, and object quotas during map creation, as only the map count limit is currently checked.

💡 Quality: Asset/NPC/object quotas computed but never enforced

📄 api/src/repositories/userMaps.ts:34-48 📄 api/src/repositories/userMaps.ts:71-74

getUserMapQuota reports maxAssetBytes, maxNpcCount, and maxObjectCount, and the PR description claims these limits are enforced, but createUserMap only checks mapsUsed >= maxMaps. The asset/NPC/object limits are never actually enforced on any write path. Either enforce them when maps/assets are added or remove the misleading claim.

✅ 6 resolved

✅ Security: requireAuth never sets request.user; all maps use accountId 1

📄 api/src/server.ts:2897 📄 api/src/server.ts:2910 📄 api/src/server.ts:2923 📄 api/src/server.ts:2968
The requireAuth middleware (api/src/middleware/auth.ts) only checks a shared token against config.tokenAuth and never attaches a user/accountId to the request. Therefore (request as any).user?.accountId is always undefined, and the || 1 fallback forces every endpoint to operate as accountId 1. Ownership, quotas, and checkMapOwnership are all evaluated against a single shared account, so any authenticated caller can create/list/modify every user's maps and the entire ownership model is defeated. Derive accountId from the authenticated session (e.g. via getAuthorizedSession) and reject the request with 401 when it cannot be resolved instead of defaulting to 1.

✅ Bug: createUserMap id allocation & quota check are racy (TOCTOU)

📄 api/src/repositories/userMaps.ts:71-85
createUserMap computes the next id via SELECT COALESCE(MAX(id)+1, ...) and checks the quota with a separate query before inserting, all outside a transaction/lock. Two concurrent requests can read the same MAX(id) and collide on the primary key, or both pass the mapsUsed < maxMaps check and exceed the quota. Wrap the read+insert in a transaction with appropriate locking (e.g. SELECT ... FOR UPDATE / advisory lock) or use a dedicated sequence, and enforce the quota atomically.

✅ Bug: user_maps table has no migration/schema definition

📄 api/src/repositories/userMaps.ts:34-48
user_maps is queried throughout userMaps.ts but no CREATE TABLE / migration for it exists in the repo (only referenced in application code; api/schema.sql lacks it). If the table is not provisioned out-of-band, every endpoint will fail at runtime with a relation-does-not-exist error. Add a migration defining user_maps (including columns npc_count/object_count/asset_bytes defaults and a CHECK constraint on status).

✅ Security: GET /internal/user-maps/:id lacks ownership check (IDOR)

📄 api/src/server.ts:2948-2962 📄 api/src/repositories/userMaps.ts:98-108
getUserMap returns any map by id with no ownership or status filter, so the /internal/user-maps/:id route exposes other users' non-published (draft/proposed/archived) maps to any authenticated caller. Restrict the lookup to maps owned by the caller (or only published maps) before returning the record.

✅ Bug: status transition value is not validated

📄 api/src/server.ts:2964-2978 📄 api/src/repositories/userMaps.ts:141-155
The PATCH status endpoint passes request.body.status straight into updateUserMapStatus, which writes it into the status column without validating it against the allowed UserMapStatus values. Combined with the missing DB CHECK constraint, an arbitrary string can be persisted. Validate status against ['draft','proposed','published','archived'] and return 400 on invalid input.

...and 1 more resolved from earlier reviews

🤖 Prompt for agents

Code Review: Adds user map sandbox with ownership controls and database migrations, resolving multiple security flaws and race conditions. Consider enforcing computed asset, NPC, and object quotas during map creation, as only the map count limit is currently checked.

1. 💡 Quality: Asset/NPC/object quotas computed but never enforced
   Files: api/src/repositories/userMaps.ts:34-48, api/src/repositories/userMaps.ts:71-74

   `getUserMapQuota` reports `maxAssetBytes`, `maxNpcCount`, and `maxObjectCount`, and the PR description claims these limits are enforced, but `createUserMap` only checks `mapsUsed >= maxMaps`. The asset/NPC/object limits are never actually enforced on any write path. Either enforce them when maps/assets are added or remove the misleading claim.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}