fix(security): annotate AppUser.password with @JsonIgnore to prevent accidental hash leakage #110

New Issue

marcel · 2026-03-27T17:32:47+01:00

marcel commented

2026-03-27 17:32:47 +01:00

Problem

UserController currently guards against password hash exposure by calling user.setPassword(null) before returning the entity. This is fragile: any new endpoint, refactor, or projection that returns an AppUser without that manual null-out will silently leak the BCrypt hash.

Risk

BCrypt hashes are not reversible in practice, but exposing them:

Reveals that the account exists and has a password set
Allows offline dictionary attacks on weak passwords
Is a data breach in any compliance context

Fix

Add @JsonIgnore to the password field on the AppUser entity:

@JsonIgnore
private String password;

Then remove the user.setPassword(null) call(s) from controllers — they become dead code.

Acceptance Criteria

GET /api/users and GET /api/users/me responses contain no password field
GET /api/users/{id} response contains no password field
No controller manually nulls the password field anymore

## Problem `UserController` currently guards against password hash exposure by calling `user.setPassword(null)` before returning the entity. This is fragile: any new endpoint, refactor, or projection that returns an `AppUser` without that manual null-out will silently leak the BCrypt hash. ## Risk BCrypt hashes are not reversible in practice, but exposing them: - Reveals that the account exists and has a password set - Allows offline dictionary attacks on weak passwords - Is a data breach in any compliance context ## Fix Add `@JsonIgnore` to the `password` field on the `AppUser` entity: ```java @JsonIgnore private String password; ``` Then remove the `user.setPassword(null)` call(s) from controllers — they become dead code. ## Acceptance Criteria - `GET /api/users` and `GET /api/users/me` responses contain no `password` field - `GET /api/users/{id}` response contains no `password` field - No controller manually nulls the password field anymore

marcel added the security label 2026-03-31 20:49:55 +02:00

marcel commented

2026-03-31 20:55:02 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

The fix itself is a one-liner, but the cleanup work is equally important: every user.setPassword(null) call in the controllers becomes dead code and must be deleted. Dead code in a security-sensitive path is actively misleading — it implies protection where there no longer is any.
Before touching AppUser, I'd want to confirm: is AppUser ever used as @RequestBody? If so, @JsonIgnore blocks deserialization of the password field from incoming JSON too, which could silently break any endpoint that sets the password via the entity directly. Looking at the CLAUDE.md, there's a CreateUserRequest DTO — so this should be safe, but worth verifying explicitly.
Is AppUser passed to any Jackson ObjectMapper.writeValueAsString() calls outside of the normal Spring MVC serialization path (e.g., in logging, audit trails, or tests)? Those would also be affected.

Suggestions

Write the failing test before removing setPassword(null). A @WebMvcTest that GETs /api/users/me and asserts the response body contains no password key. Without this, the acceptance criteria are prose, not enforced code.
After the annotation lands: grep the entire codebase for setPassword(null) and remove every hit. Don't leave them as "harmless" dead code — they're noise that will confuse the next reader.
Consider a test name like should not expose password hash in user response — it reads as a security property, not just a serialization detail.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - The fix itself is a one-liner, but the cleanup work is equally important: every `user.setPassword(null)` call in the controllers becomes dead code and must be deleted. Dead code in a security-sensitive path is actively misleading — it implies protection where there no longer is any. - Before touching `AppUser`, I'd want to confirm: is `AppUser` ever used as `@RequestBody`? If so, `@JsonIgnore` blocks deserialization of the password field from incoming JSON too, which could silently break any endpoint that sets the password via the entity directly. Looking at the CLAUDE.md, there's a `CreateUserRequest` DTO — so this should be safe, but worth verifying explicitly. - Is `AppUser` passed to any Jackson `ObjectMapper.writeValueAsString()` calls outside of the normal Spring MVC serialization path (e.g., in logging, audit trails, or tests)? Those would also be affected. ### Suggestions - Write the failing test *before* removing `setPassword(null)`. A `@WebMvcTest` that GETs `/api/users/me` and asserts the response body contains no `password` key. Without this, the acceptance criteria are prose, not enforced code. - After the annotation lands: grep the entire codebase for `setPassword(null)` and remove every hit. Don't leave them as "harmless" dead code — they're noise that will confuse the next reader. - Consider a test name like `should not expose password hash in user response` — it reads as a security property, not just a serialization detail.

marcel commented

2026-03-31 20:55:20 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Questions & Observations

The root cause is correctly identified. Relying on manual setPassword(null) in every controller is a classic "defense by convention" anti-pattern — it works until someone adds a new endpoint and forgets the convention. @JsonIgnore at the model level is the right fix: the protection travels with the object.
@JsonIgnore vs @JsonProperty(access = WRITE_ONLY) — know the difference. @JsonIgnore suppresses the field in both serialization (output) and deserialization (input). If AppUser is ever deserialized from a JSON request body — even in a test — the password field will be silently ignored. For a field that is only ever set via DTOs or Spring Security's UserDetailsService, @JsonIgnore is fine. But if there's any path where the entity is bound from JSON input, use @JsonProperty(access = JsonProperty.Access.READ_ONLY) instead to block output while preserving input.
Mass assignment surface check. With @JsonIgnore in place, verify there are no @RequestBody AppUser bindings anywhere in the codebase. If there are, that's a mass assignment vulnerability independent of this issue — but worth catching now.
Scope question: are there other sensitive fields? AppUser likely has a groups relationship. Consider whether the full group permission set should also be suppressed in certain response contexts (e.g., a user fetching their own profile should not see a full enumeration of all permission strings if that information is useful for privilege escalation reconnaissance).

Suggestions

Add a Semgrep rule to catch future regressions: flag any @Entity class that has a field named password without @JsonIgnore or @JsonProperty(access = WRITE_ONLY).
The acceptance criteria should include a negative test: after the fix, confirm that a raw curl -s GET /api/users/me response does not contain the string "password" — not just null, but absent entirely.
CWE reference: CWE-312 — Cleartext Storage of Sensitive Information in a Serialized Object. Worth noting in the commit message for audit trail purposes.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Questions & Observations - **The root cause is correctly identified.** Relying on manual `setPassword(null)` in every controller is a classic "defense by convention" anti-pattern — it works until someone adds a new endpoint and forgets the convention. `@JsonIgnore` at the model level is the right fix: the protection travels with the object. - **`@JsonIgnore` vs `@JsonProperty(access = WRITE_ONLY)` — know the difference.** `@JsonIgnore` suppresses the field in both serialization (output) and deserialization (input). If `AppUser` is ever deserialized from a JSON request body — even in a test — the password field will be silently ignored. For a field that is *only ever set via DTOs or Spring Security's `UserDetailsService`*, `@JsonIgnore` is fine. But if there's any path where the entity is bound from JSON input, use `@JsonProperty(access = JsonProperty.Access.READ_ONLY)` instead to block output while preserving input. - **Mass assignment surface check.** With `@JsonIgnore` in place, verify there are no `@RequestBody AppUser` bindings anywhere in the codebase. If there are, that's a mass assignment vulnerability independent of this issue — but worth catching now. - **Scope question: are there other sensitive fields?** `AppUser` likely has a `groups` relationship. Consider whether the full group permission set should also be suppressed in certain response contexts (e.g., a user fetching their own profile should not see a full enumeration of all permission strings if that information is useful for privilege escalation reconnaissance). ### Suggestions - Add a Semgrep rule to catch future regressions: flag any `@Entity` class that has a field named `password` without `@JsonIgnore` or `@JsonProperty(access = WRITE_ONLY)`. - The acceptance criteria should include a negative test: after the fix, confirm that a raw `curl -s GET /api/users/me` response does not contain the string `"password"` — not just `null`, but absent entirely. - CWE reference: **CWE-312 — Cleartext Storage of Sensitive Information in a Serialized Object**. Worth noting in the commit message for audit trail purposes.

marcel commented

2026-03-31 20:55:39 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Questions & Observations

The acceptance criteria list three endpoints to verify, which is good. But there's no mention of how these will be tested or at which layer. Without a test, this fix is only as reliable as the next code review.
Is there an existing UserControllerTest or @WebMvcTest slice? If yes, the regression test slots in there. If not, this issue is a good trigger to add the slice.

Test Strategy

Integration layer — @WebMvcTest(UserController.class) with @WithMockUser:

@Test
void shouldNotExposePasswordHashInUserResponse() throws Exception {
    AppUser user = AppUser.builder().id(UUID.randomUUID()).username("test").password("$2a$...hash...").build();
    when(userService.getCurrentUser()).thenReturn(user);

    mockMvc.perform(get("/api/users/me").with(user("test").roles("USER")))
        .andExpect(status().isOk())
        .andExpect(jsonPath("$.password").doesNotExist());
}

Test name: shouldNotExposePasswordHashInUserResponse
Assert $.password does not exist — not isNull(). A null field still signals the hash is being computed; absent is the correct assertion.
Run the same assertion for GET /api/users/{id} and GET /api/users (array element).

Edge Cases to Cover

What if Spring Data REST or any @RepositoryRestResource is configured? Those can expose entity fields independently of @JsonIgnore in some configurations — worth a quick check.
Confirm the fix survives a future Lombok @Data regeneration — @JsonIgnore on the field survives Lombok, but it's worth a note in the PR.

Suggestions

Once this test exists, it becomes a permanent regression guard. Name it to reflect the security property, not the implementation: shouldNotExposePasswordHashInUserResponse, not testPasswordFieldAbsent.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Questions & Observations - The acceptance criteria list three endpoints to verify, which is good. But there's no mention of *how* these will be tested or at which layer. Without a test, this fix is only as reliable as the next code review. - Is there an existing `UserControllerTest` or `@WebMvcTest` slice? If yes, the regression test slots in there. If not, this issue is a good trigger to add the slice. ### Test Strategy **Integration layer — `@WebMvcTest(UserController.class)` with `@WithMockUser`:** ```java @Test void shouldNotExposePasswordHashInUserResponse() throws Exception { AppUser user = AppUser.builder().id(UUID.randomUUID()).username("test").password("$2a$...hash...").build(); when(userService.getCurrentUser()).thenReturn(user); mockMvc.perform(get("/api/users/me").with(user("test").roles("USER"))) .andExpect(status().isOk()) .andExpect(jsonPath("$.password").doesNotExist()); } ``` - Test name: `shouldNotExposePasswordHashInUserResponse` - Assert `$.password` **does not exist** — not `isNull()`. A null field still signals the hash is being computed; absent is the correct assertion. - Run the same assertion for `GET /api/users/{id}` and `GET /api/users` (array element). ### Edge Cases to Cover - What if Spring Data REST or any `@RepositoryRestResource` is configured? Those can expose entity fields independently of `@JsonIgnore` in some configurations — worth a quick check. - Confirm the fix survives a future Lombok `@Data` regeneration — `@JsonIgnore` on the field survives Lombok, but it's worth a note in the PR. ### Suggestions - Once this test exists, it becomes a **permanent regression guard**. Name it to reflect the security property, not the implementation: `shouldNotExposePasswordHashInUserResponse`, not `testPasswordFieldAbsent`.

marcel commented

2026-03-31 20:56:00 +02:00

🏗️ Markus Keller — Application Architect

Questions & Observations

The diagnosis is exactly right. Placing the protection in every controller is logic at the wrong layer. @JsonIgnore on the entity moves the invariant to where it belongs — the model — so it holds regardless of how many controllers serialise the type in future.
Serialisation path audit before closing. Beyond Spring MVC's Jackson serializer, check: (1) any ObjectMapper.writeValueAsString(user) calls in services or tests; (2) any @RepositoryRestResource on AppUserRepository — Spring Data REST bypasses controller-level protection entirely; (3) any SSE or WebSocket handlers that push user objects.
Response DTO consideration. This project returns entities directly (no response DTOs, per the CLAUDE.md). That's fine for now, but AppUser is one of those entities where the gap between "what the DB stores" and "what the API should expose" is wide. The password field is the urgent case; groups with full permission strings may be a future one. Worth a comment in the PR.

Suggestions

If AppUser ever needs to accept a password via JSON (e.g., a password-change endpoint), use @JsonProperty(access = JsonProperty.Access.READ_ONLY) instead — it blocks serialization (output) while allowing deserialization (input). @JsonIgnore blocks both. Given the current DTO-first pattern for writes, @JsonIgnore is fine — just document the choice.
The removal of setPassword(null) is non-negotiable. Don't leave it as "technically harmless" — it creates false confidence in the code path and will confuse the next person tracing serialization behavior.
No architectural escalation needed here — this is a one-file fix that correctly pushes a cross-cutting concern down to the model layer. Ship it.

## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **The diagnosis is exactly right.** Placing the protection in every controller is logic at the wrong layer. `@JsonIgnore` on the entity moves the invariant to where it belongs — the model — so it holds regardless of how many controllers serialise the type in future. - **Serialisation path audit before closing.** Beyond Spring MVC's Jackson serializer, check: (1) any `ObjectMapper.writeValueAsString(user)` calls in services or tests; (2) any `@RepositoryRestResource` on `AppUserRepository` — Spring Data REST bypasses controller-level protection entirely; (3) any SSE or WebSocket handlers that push user objects. - **Response DTO consideration.** This project returns entities directly (no response DTOs, per the CLAUDE.md). That's fine for now, but `AppUser` is one of those entities where the gap between "what the DB stores" and "what the API should expose" is wide. The `password` field is the urgent case; `groups` with full permission strings may be a future one. Worth a comment in the PR. ### Suggestions - If `AppUser` ever needs to accept a password via JSON (e.g., a password-change endpoint), use `@JsonProperty(access = JsonProperty.Access.READ_ONLY)` instead — it blocks serialization (output) while allowing deserialization (input). `@JsonIgnore` blocks both. Given the current DTO-first pattern for writes, `@JsonIgnore` is fine — just document the choice. - The removal of `setPassword(null)` is non-negotiable. Don't leave it as "technically harmless" — it creates false confidence in the code path and will confuse the next person tracing serialization behavior. - No architectural escalation needed here — this is a one-file fix that correctly pushes a cross-cutting concern down to the model layer. Ship it.

marcel commented

2026-03-31 20:56:08 +02:00

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

No UI concerns from my angle — this fix touches only backend serialization with no user-facing rendering changes. I checked there are no frontend components that display or depend on a password field from the user API response, so no template or binding changes are required on the SvelteKit side.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist No UI concerns from my angle — this fix touches only backend serialization with no user-facing rendering changes. I checked there are no frontend components that display or depend on a `password` field from the user API response, so no template or binding changes are required on the SvelteKit side.

marcel commented

2026-03-31 20:56:20 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Questions & Observations

No deployment or infrastructure changes required for this fix — it's a source code annotation with no config, env, or compose file impact.
One operational concern worth raising: does any log aggregation (Promtail → Loki, or plain Spring Boot logging) serialize AppUser objects via toString()? Lombok's @Data generates a toString() that includes all fields by default, including password. @JsonIgnore has no effect on toString(). If user objects are ever logged — even at DEBUG level — the BCrypt hash ends up in your log store.

Suggestions

Add @ToString.Exclude to the password field alongside @JsonIgnore:
```
@JsonIgnore
@ToString.Exclude
private String password;
```
This ensures the hash never leaks through logging either.
Once production log aggregation is in place (issue #140), do a one-time grep of existing logs for $2a$ (BCrypt prefix) to confirm no prior leakage has been stored. Low effort, high value for a compliance audit.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Questions & Observations - No deployment or infrastructure changes required for this fix — it's a source code annotation with no config, env, or compose file impact. - **One operational concern worth raising:** does any log aggregation (Promtail → Loki, or plain Spring Boot logging) serialize `AppUser` objects via `toString()`? Lombok's `@Data` generates a `toString()` that includes *all fields by default*, including `password`. `@JsonIgnore` has no effect on `toString()`. If user objects are ever logged — even at DEBUG level — the BCrypt hash ends up in your log store. ### Suggestions - Add `@ToString.Exclude` to the `password` field alongside `@JsonIgnore`: ```java @JsonIgnore @ToString.Exclude private String password; ``` This ensures the hash never leaks through logging either. - Once production log aggregation is in place (issue #140), do a one-time grep of existing logs for `$2a$` (BCrypt prefix) to confirm no prior leakage has been stored. Low effort, high value for a compliance audit.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#110