Stop id/nano_id from being client-settable on create/update

[harry-rhesis]

Purpose

API endpoints for creating and updating entities currently accept id and nano_id fields in request bodies. Both are backend-assigned: id via a Postgres gen_random_uuid() server default, nano_id via a Python-side default on the SQLAlchemy base model. A client could set its own id (UUID squatting) on create, or repoint either field on an existing row via update, since the CRUD layer only stripped project_id. Closes #744.

What Changed

• apps/backend/.../schemas/base.py: split the shared Base schema (no identity fields, inherited by both write and read schemas today) from a new ServerIdentity mixin holding id/nano_id. ServerIdentity is added only to response schemas that need it — never to a Create/Update schema. This makes new entities safe by default: forgetting to mix in ServerIdentity on a future entity only costs a display convenience, never reopens the vulnerability, unlike stripping the fields the other way (opt-out on write) would.
• apps/backend/.../utils/crud_utils.py: defense-in-depth for callers that bypass Pydantic validation — strips nano_id on create (no internal writer ever sets one) and id+nano_id on update (identity is immutable; FileCreate's pre-generated id is a create-only, deliberate exception documented in its schema).
• sdk/: BaseEntity.push(), Model.push(), and native metric sync were sending id/nano_id in write bodies (one already asserted by an SDK test as expected behavior). Updated to pop both before building the request body.
• apps/frontend/: tightened two type-level holes (EndpointEditData, updateOrganization) that accepted id/nano_id via Partial<Entity>, though no current caller sends them.
• tests/backend/schemas/test_server_identity.py: route-introspection guardrail derived from the live FastAPI route table — fails if any request body schema exposes id/nano_id, and pins that the response schemas consumers actually read nano_id from keep exposing it.
• tests/backend/routes/test_server_owned_identity.py: behavioral test — POST/PUT with spoofed id/nano_id get ignored; the server's own values come back in the response.

Additional Context

• Closes API endpoints should not allow users to specify nano_id and id when creating entities #744.
• Deliberate deviation from the issue's stated acceptance criteria, explained in this comment: the fix makes the backend ignore id/nano_id rather than return a 422. Our own SDK sends these fields in write bodies today, and a hard rejection would break any already-released SDK version for no security benefit — the fields already have no effect on the write, since the backend assigns its own values regardless of what's sent. This also matches how Stripe/GitHub handle unrecognized fields on write.
• No documentation changes needed: the fields simply no longer appear in the generated OpenAPI schema for Create/Update bodies.

Testing

• cd apps/backend && uv run pytest ../../tests/backend/schemas/ ../../tests/backend/crud/ ../../tests/backend/routes/ ../../tests/backend/security/ — 3198 passed, 40 pre-existing skips, 0 failures.
• cd sdk && uv run pytest ../tests/sdk/entities/test_base_entity.py ../tests/sdk/entities/test_model.py ../tests/sdk/metrics/test_metric_scope.py — 26 passed.
• cd apps/frontend && npx tsc --noEmit && npx eslint src/utils/api-client/interfaces/endpoint.ts src/utils/api-client/organizations-client.ts — clean.
• New tests specifically exercise: a POST/PUT to /categories with spoofed id/nano_id in the body, asserting the response reflects the server's own values, not the caller's.

[peqy]

[Improvement] Defense-in-depth: crud_utils._prepare_item_data still allows id through on create for dict-based callers. Consider stripping id by default and only allowing it for the known pre-generated-PK use case(s) (e.g. FileCreate).

Everything else looks aligned with the stated goal: schema split + SDK/frontend payload tightening + strong route-introspection tests should make this hard to regress.

[peqy]

[Improvement] _prepare_item_data still allows id through on create for any dict-based caller.

Fix: consider popping id here too, and only allowing it for the specific create path(s) that need pre-generated PKs (e.g. FileCreate/File model) via an explicit allowlist/flag. That keeps the defense-in-depth guarantee symmetrical with the update path and avoids reintroducing UUID-squatting if a future endpoint bypasses Pydantic.

[peqy]

Good news: with the new schema split + route-introspection test, a client shouldn’t be able to send id on normal creates anymore.

The remaining reason I still suggested popping id in _prepare_item_data is purely defense-in-depth for dict-based internal call paths: I found at least crud/embedding.py:create_embedding() and crud/metric.py:create_metric() build a plain dict and pass it into create_item() (so _prepare_item_data will accept any id key if one is ever present). Today that dict originates from a validated Pydantic model, so it’s fine — but if a future endpoint/task bypasses validation, it could reintroduce UUID-squatting on create.

If you want to keep the “FileCreate can pre-generate id” behavior, one clean pattern would be data.pop("id", None) by default + an explicit allow_client_id=True flag/allowlist at the specific File create call site.

[harry-rhesis]

Fixed in e45f44e: _prepare_item_data now pops id by default and only lets it through when model.__name__ == "File" — matching the pattern you suggested. Confirmed crud/file.py:51 is the only internal caller that passes an explicit id through create_item; crud/embedding.py/crud/metric.py don't set one. Verified with the full backend suite (3198 passed) plus all File-specific tests (57 passed) to confirm the pre-generated-PK path still works.

[peqy]

Yep, that addresses my defense-in-depth concern — popping id by default and only allowing it for File keeps the create path safe.

Minor thought: using model is File (or an explicit allow_client_id flag) is a bit more robust than model.__name__ == "File", but the behavior you implemented is the key part. Feel free to mark this thread resolved.

[harry-rhesis]

Good call to double-check, but I'll keep the string comparison. This same file already dispatches on model.__name__ elsewhere: line 951 (model.__name__ == "TypeLookup") and create_item's exempt_models = ["User", "Organization", "Token"]. Matching that existing convention seemed better than introducing a different pattern (identity comparison + a new model import) for the same kind of check in the same file. Thanks for confirming the behavior is right.

[peqy]

Yep, fair — if crud_utils.py already uses model.__name__/string allowlists for similar dispatch, keeping that convention here is totally reasonable.

The main downside is it’s a bit more refactor/name-collision sensitive than model is File, but given this is an internal check and you’ve got test coverage around the File pre-generated-id path, I’m good with the string comparison.

[peqy]

Solid fix: splitting Base vs ServerIdentity makes it hard to accidentally re-expose server-owned identity on write, and the route-introspection test is a great guardrail.

One remaining defense-in-depth gap: _prepare_item_data() still allows id through on create for dict-based callers (even though schemas now hide it). Consider default-dropping id there too and explicitly allowing it only for the specific pre-generated-id create path(s) (e.g. FileCreate) so UUID-squatting can’t be reintroduced via a future validation-bypassing endpoint/task.

Found 1 issue (0 critical, 1 improvement).

[peqy]

Looks good. The schema split + route-introspection guardrail closes the client-settable id/nano_id hole, and the CRUD defense-in-depth now correctly strips id on create for all models except File.

Found 0 issues.