No validation on systemRole values — arbitrary roles accepted #5

New Issue

marcel · 2026-04-02T11:20:30+02:00

marcel commented

2026-04-02 11:20:30 +02:00

Problem

The AdminService.updateUser() and createUser() methods accept any string for systemRole without validating against an allowed set. An admin could set systemRole to an arbitrary value like "superadmin".

Affected files

AdminService.java:96-100 — updateUser sets role without validation
AdminService.java:62 — createUser sets role without validation

Attack scenario

A compromised admin account sets an arbitrary role value. While this doesn't directly escalate privileges today (only ROLE_ADMIN is checked in SecurityConfig), it's a ticking time bomb — any future role check against an unexpected value will behave unpredictably.

Recommended fix

Validate systemRole against a whitelist ("user", "admin") in both methods. Reject any other value with a ValidationException. Consider using an enum.

Severity

High — no input validation on a privilege-controlling field.

## Problem The `AdminService.updateUser()` and `createUser()` methods accept any string for `systemRole` without validating against an allowed set. An admin could set `systemRole` to an arbitrary value like `"superadmin"`. ## Affected files - `AdminService.java:96-100` — `updateUser` sets role without validation - `AdminService.java:62` — `createUser` sets role without validation ## Attack scenario A compromised admin account sets an arbitrary role value. While this doesn't directly escalate privileges today (only `ROLE_ADMIN` is checked in `SecurityConfig`), it's a ticking time bomb — any future role check against an unexpected value will behave unpredictably. ## Recommended fix Validate `systemRole` against a whitelist (`"user"`, `"admin"`) in both methods. Reject any other value with a `ValidationException`. Consider using an enum. ## Severity High — no input validation on a privilege-controlling field.

marcel added the kind/security priority/high labels 2026-04-02 11:21:00 +02:00

marcel commented

2026-04-03 09:25:21 +02:00

👨‍💻 Kai — Frontend Engineer

This fix is primarily backend, but there are frontend implications I want to flag before implementation starts.

Questions for the backend team:

What HTTP status and error body does the API return when an invalid systemRole is submitted — 400 with a validation message, or 422? I need to know so I can wire up the error state in the admin user form correctly.
Is the enum going to be reflected in the OpenAPI spec? That would let us derive the allowed values client-side from the schema rather than hardcoding them in a <select>.

Frontend-side concerns:

The role selector in the admin UI should be a <select> (or equivalent) bound to an explicit list of valid values — not a free-text input. If it's currently a text field, that's a UI design smell that mirrors the backend bug. I'll check the current admin form component.
Even with a <select>, we should validate server-side (which this issue is addressing) — a crafted HTTP request bypasses any client-side constraint. So the fix here is necessary regardless of what the UI does.
When the backend rejects the value, the form should surface the error message clearly next to the role field, not just a generic toast. I'll handle that in the form's +page.server.ts action error handling.

No blocking concerns on my end — just want to coordinate on the error response shape before I implement the error UI.

## 👨‍💻 Kai — Frontend Engineer This fix is primarily backend, but there are frontend implications I want to flag before implementation starts. **Questions for the backend team:** - What HTTP status and error body does the API return when an invalid `systemRole` is submitted — `400` with a validation message, or `422`? I need to know so I can wire up the error state in the admin user form correctly. - Is the enum going to be reflected in the OpenAPI spec? That would let us derive the allowed values client-side from the schema rather than hardcoding them in a `<select>`. **Frontend-side concerns:** - The role selector in the admin UI should be a `<select>` (or equivalent) bound to an explicit list of valid values — not a free-text input. If it's currently a text field, that's a UI design smell that mirrors the backend bug. I'll check the current admin form component. - Even with a `<select>`, we should validate server-side (which this issue is addressing) — a crafted HTTP request bypasses any client-side constraint. So the fix here is necessary regardless of what the UI does. - When the backend rejects the value, the form should surface the error message clearly next to the role field, not just a generic toast. I'll handle that in the form's `+page.server.ts` action error handling. **No blocking concerns on my end** — just want to coordinate on the error response shape before I implement the error UI.

marcel commented

2026-04-03 09:25:34 +02:00

🏗️ Backend Engineer — Spring Boot / PostgreSQL Specialist

Solid find. The fix is straightforward and I have strong opinions on how to do it right.

Recommended implementation path:

Use a Java enum, not a string whitelist — SystemRole.USER and SystemRole.ADMIN. Enums give you compile-time safety, exhaustive switch coverage, and Jackson deserializes them cleanly. A string whitelist is just a poor man's enum.
Map the enum to the DB column — if system_role is stored as varchar, annotate with @Enumerated(EnumType.STRING). If it's a PostgreSQL enum type, use a custom converter. Either way, invalid strings never reach the service layer.
Validate at the controller boundary — the AdminUserRequest DTO should declare systemRole as SystemRole (the enum), not String. Jackson will throw a HttpMessageNotReadableException with a 400 before AdminService is even called. No manual whitelist check needed.
Remove the manual validation in AdminService if you add it there — that's controller-layer concern, not service-layer. The service should accept a validated enum, not re-validate strings.

Questions before implementation:

Is system_role stored in the DB as a plain varchar or a PostgreSQL CREATE TYPE ... AS ENUM? The mapping strategy differs.
Does any existing test data or migration script set roles as lowercase ("admin") vs uppercase ("ADMIN")? We need a consistent casing convention before we add the enum.
Are there any existing API clients (scripts, Postman collections, frontend calls) sending role strings we'd break with a casing change?

DB-level consideration: Even with the enum fix in Java, I'd recommend adding a CHECK constraint on the system_role column: CHECK (system_role IN ('user', 'admin')). Defense in depth — the DB should not accept invalid values even if something bypasses the application layer.

## 🏗️ Backend Engineer — Spring Boot / PostgreSQL Specialist Solid find. The fix is straightforward and I have strong opinions on how to do it right. **Recommended implementation path:** 1. **Use a Java enum, not a string whitelist** — `SystemRole.USER` and `SystemRole.ADMIN`. Enums give you compile-time safety, exhaustive switch coverage, and Jackson deserializes them cleanly. A string whitelist is just a poor man's enum. 2. **Map the enum to the DB column** — if `system_role` is stored as `varchar`, annotate with `@Enumerated(EnumType.STRING)`. If it's a PostgreSQL enum type, use a custom converter. Either way, invalid strings never reach the service layer. 3. **Validate at the controller boundary** — the `AdminUserRequest` DTO should declare `systemRole` as `SystemRole` (the enum), not `String`. Jackson will throw a `HttpMessageNotReadableException` with a 400 before `AdminService` is even called. No manual whitelist check needed. 4. **Remove the manual validation in `AdminService`** if you add it there — that's controller-layer concern, not service-layer. The service should accept a validated enum, not re-validate strings. **Questions before implementation:** - Is `system_role` stored in the DB as a plain `varchar` or a PostgreSQL `CREATE TYPE ... AS ENUM`? The mapping strategy differs. - Does any existing test data or migration script set roles as lowercase (`"admin"`) vs uppercase (`"ADMIN"`)? We need a consistent casing convention before we add the enum. - Are there any existing API clients (scripts, Postman collections, frontend calls) sending role strings we'd break with a casing change? **DB-level consideration:** Even with the enum fix in Java, I'd recommend adding a `CHECK` constraint on the `system_role` column: `CHECK (system_role IN ('user', 'admin'))`. Defense in depth — the DB should not accept invalid values even if something bypasses the application layer.

marcel commented

2026-04-03 09:25:43 +02:00

🧪 QA Engineer

Good catch. Here's the test matrix I'd want covered before this is marked done.

Unit tests for AdminService:

createUser with valid role "user" → succeeds
createUser with valid role "admin" → succeeds
createUser with invalid role "superadmin" → throws ValidationException (or equivalent)
createUser with null role → what's the behavior? Should it default to "user" or reject?
updateUser with valid role → succeeds
updateUser with invalid role → throws ValidationException
updateUser with empty string role → throws ValidationException

Integration tests (controller level):

POST /v1/admin/users with systemRole: "superadmin" → expect 400 or 422, not 201
PUT /v1/admin/users/{id} with systemRole: "superadmin" → expect 400 or 422
Verify the error response body contains a meaningful message about the invalid value (not a raw stack trace)
Verify valid roles still work end-to-end after the fix

Edge cases I'd flag:

What happens with casing variants — "Admin", "ADMIN", "USER"? Is the validation case-sensitive? Document and test both directions.
What if systemRole is omitted entirely from the request body — null vs missing field — does the behavior differ?
If we switch to a Java enum and rely on Jackson deserialization for validation, does the 400 error response body match the standard error format the frontend expects? Jackson's default message for an enum deserialization failure can be verbose and expose internal type names.

Test infrastructure note: The enum deserialization path is worth a dedicated @ParameterizedTest with a list of invalid strings — that keeps the test readable and makes adding new invalid values trivial.

## 🧪 QA Engineer Good catch. Here's the test matrix I'd want covered before this is marked done. **Unit tests for `AdminService`:** - `createUser` with valid role `"user"` → succeeds - `createUser` with valid role `"admin"` → succeeds - `createUser` with invalid role `"superadmin"` → throws `ValidationException` (or equivalent) - `createUser` with null role → what's the behavior? Should it default to `"user"` or reject? - `updateUser` with valid role → succeeds - `updateUser` with invalid role → throws `ValidationException` - `updateUser` with empty string role → throws `ValidationException` **Integration tests (controller level):** - `POST /v1/admin/users` with `systemRole: "superadmin"` → expect `400` or `422`, not `201` - `PUT /v1/admin/users/{id}` with `systemRole: "superadmin"` → expect `400` or `422` - Verify the error response body contains a meaningful message about the invalid value (not a raw stack trace) - Verify valid roles still work end-to-end after the fix **Edge cases I'd flag:** - What happens with casing variants — `"Admin"`, `"ADMIN"`, `"USER"`? Is the validation case-sensitive? Document and test both directions. - What if `systemRole` is omitted entirely from the request body — null vs missing field — does the behavior differ? - If we switch to a Java enum and rely on Jackson deserialization for validation, does the 400 error response body match the standard error format the frontend expects? Jackson's default message for an enum deserialization failure can be verbose and expose internal type names. **Test infrastructure note:** The enum deserialization path is worth a dedicated `@ParameterizedTest` with a list of invalid strings — that keeps the test readable and makes adding new invalid values trivial.

marcel commented

2026-04-03 09:25:55 +02:00

🔒 Sable — Security Engineer

I filed this issue, so let me add the threat model detail and fix constraints for whoever implements it.

Why this is High and not Medium:
The "only ROLE_ADMIN is checked today" reasoning is exactly why this is dangerous — it's a latent vulnerability waiting for a new role check to be added incorrectly. Setting systemRole: "superadmin" today writes an unexpected value to the DB. Any future code that does if (!role.equals("user")) instead of if (role.equals("admin")) would accidentally treat "superadmin" as admin-equivalent. We can't reason about future code correctness against an unbounded string field.

Security requirements for the fix:

Validation must happen server-side, always — client-side enum constraints in a <select> are bypassed trivially with a raw HTTP request
The error response for an invalid role must NOT echo back the submitted value verbatim in a way that could be used for enumeration (e.g., don't say "superadmin is not a valid role" — say "invalid systemRole value")
The rejection must be logged as a security event: who attempted it, what value was submitted, timestamp. This is potentially an indicator of a probing attack on a compromised admin account
Ensure the ValidationException (or 400 response) does not leak internal type names, package paths, or enum class names in the response body

DB-level enforcement: I want a CHECK constraint on system_role independent of the application fix. If someone connects directly to the DB or if the ORM is bypassed via a native query, the constraint is the last line of defense.

Audit log: Any successful role change via AdminService.updateUser() should already be hitting the admin_audit_log. Confirm that's the case — the fix shouldn't silently remove that logging path.

## 🔒 Sable — Security Engineer I filed this issue, so let me add the threat model detail and fix constraints for whoever implements it. **Why this is High and not Medium:** The "only `ROLE_ADMIN` is checked today" reasoning is exactly why this is dangerous — it's a latent vulnerability waiting for a new role check to be added incorrectly. Setting `systemRole: "superadmin"` today writes an unexpected value to the DB. Any future code that does `if (!role.equals("user"))` instead of `if (role.equals("admin"))` would accidentally treat `"superadmin"` as admin-equivalent. We can't reason about future code correctness against an unbounded string field. **Security requirements for the fix:** - Validation must happen server-side, always — client-side enum constraints in a `<select>` are bypassed trivially with a raw HTTP request - The error response for an invalid role must NOT echo back the submitted value verbatim in a way that could be used for enumeration (e.g., don't say "superadmin is not a valid role" — say "invalid systemRole value") - The rejection must be logged as a security event: who attempted it, what value was submitted, timestamp. This is potentially an indicator of a probing attack on a compromised admin account - Ensure the `ValidationException` (or 400 response) does not leak internal type names, package paths, or enum class names in the response body **DB-level enforcement:** I want a `CHECK` constraint on `system_role` independent of the application fix. If someone connects directly to the DB or if the ORM is bypassed via a native query, the constraint is the last line of defense. **Audit log:** Any successful role change via `AdminService.updateUser()` should already be hitting the `admin_audit_log`. Confirm that's the case — the fix shouldn't silently remove that logging path.

marcel commented

2026-04-03 09:26:05 +02:00

🎨 Atlas — UI/UX Designer

This is a backend validation issue, but the admin UI has a design responsibility here too.

Design concerns:

The role selector must be a constrained control — if the admin form uses a free-text input for systemRole, that's a design problem I want corrected regardless of this backend fix. A <select> or radio group with exactly the valid options makes it impossible for an admin to accidentally type an invalid value, and the intent is immediately clear.
Labeling: "System Role" is technical language. In the admin UI, consider labeling it "Account type" with options "Standard user" and "Administrator" — map to the enum values on submit. Clearer for admins who aren't engineers.
Error state: If the API rejects a role value (edge case after the UI fix, but still possible via direct API), the form needs a clear inline error on the role field — not just a generic toast. The error should tell the admin what went wrong without exposing technical details.
Confirmation for role escalation: Changing a user to admin is a significant privilege event. Consider a confirmation step ("You are granting administrator access to [name]. Confirm?") as a UX safety net against accidental escalations — independent of the technical validation.

Questions:

Is there currently a spec for the admin user management screen? I don't recall seeing one in the screen inventory (A1–E3). If this screen doesn't have a design yet, I'd like to spec it before Kai implements the error states.
Should the role selector display the current role value when editing an existing user? That seems obvious but I want to confirm the edit form pre-populates correctly.

## 🎨 Atlas — UI/UX Designer This is a backend validation issue, but the admin UI has a design responsibility here too. **Design concerns:** - **The role selector must be a constrained control** — if the admin form uses a free-text input for `systemRole`, that's a design problem I want corrected regardless of this backend fix. A `<select>` or radio group with exactly the valid options makes it impossible for an admin to accidentally type an invalid value, and the intent is immediately clear. - **Labeling:** "System Role" is technical language. In the admin UI, consider labeling it "Account type" with options "Standard user" and "Administrator" — map to the enum values on submit. Clearer for admins who aren't engineers. - **Error state:** If the API rejects a role value (edge case after the UI fix, but still possible via direct API), the form needs a clear inline error on the role field — not just a generic toast. The error should tell the admin what went wrong without exposing technical details. - **Confirmation for role escalation:** Changing a user to `admin` is a significant privilege event. Consider a confirmation step ("You are granting administrator access to [name]. Confirm?") as a UX safety net against accidental escalations — independent of the technical validation. **Questions:** - Is there currently a spec for the admin user management screen? I don't recall seeing one in the screen inventory (A1–E3). If this screen doesn't have a design yet, I'd like to spec it before Kai implements the error states. - Should the role selector display the current role value when editing an existing user? That seems obvious but I want to confirm the edit form pre-populates correctly.

marcel referenced this issue

2026-04-03 11:25:34 +02:00

feat: C3 — Variety review screen (Issue #28) #41

marcel referenced this issue

2026-04-04 17:02:10 +02:00

feat: Add-to-Plan flows — C4 recipe picker, C5 quick actions, C6 day picker #42

marcel referenced this issue

2026-04-06 20:00:04 +02:00

feat: D1 — Shopping list (Issue #30) #43

Sign in to join this conversation.