No password complexity requirements beyond minimum length #15

New Issue

marcel · 2026-04-02T11:21:37+02:00

marcel commented

2026-04-02 11:21:37 +02:00

Problem

SignupRequest only enforces @Size(min = 8) on passwords. There are no checks for character diversity or known-breached passwords.

Affected files

SignupRequest.java — @NotBlank @Size(min = 8) String password

Recommended fix

Consider:

Checking passwords against the Have I Been Pwned breached password list (k-anonymity API)
Or at minimum, adding a max length cap to prevent denial-of-service via BCrypt with extremely long passwords (BCrypt truncates at 72 bytes, but the hashing itself is expensive)

Severity

Low — minimum length is acceptable but could be strengthened.

## Problem `SignupRequest` only enforces `@Size(min = 8)` on passwords. There are no checks for character diversity or known-breached passwords. ## Affected files - `SignupRequest.java` — `@NotBlank @Size(min = 8) String password` ## Recommended fix Consider: 1. Checking passwords against the Have I Been Pwned breached password list (k-anonymity API) 2. Or at minimum, adding a max length cap to prevent denial-of-service via BCrypt with extremely long passwords (BCrypt truncates at 72 bytes, but the hashing itself is expensive) ## Severity Low — minimum length is acceptable but could be strengthened.

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

marcel commented

2026-04-03 09:27:35 +02:00

👨‍💻 Kai — Frontend Engineer

This is a backend issue, but it has a direct frontend implication: whatever password rules the backend enforces, the signup form (A2) and the join-household form (A4) need to communicate them clearly to the user before they submit.

Frontend impact of each proposed fix:

Max length cap: If the backend adds @Size(max = 72) (or whatever the BCrypt boundary is), the <input type="password"> should have a matching maxlength attribute so users aren't cut off silently. I'd set maxlength="128" on the input and let the backend enforce the actual BCrypt limit — but the two need to agree.
HIBP integration: If a server-side HIBP check is added, the form needs to handle a new error response from the backend. Right now signup probably handles email-already-exists (409) errors. We'd need to handle a "password found in breach database" response too — with a clear, non-alarming message for the user (something like "This password has been seen in data breaches. Please choose a different one.").
Character diversity rules: If rules like "must contain a number" are added, the frontend can show a live password strength indicator. That's pure UI — no backend dependency — but it should mirror whatever rules the backend enforces exactly.

Questions for me before implementation:

Will HIBP checks happen synchronously on form submit (user waits for the result), or asynchronously? Async adds UI complexity.
What HTTP status code does the backend return for a breached password — 422 (validation failure) or something else? I need to know the error shape to map it into form error state correctly.
Is there a password strength indicator in the design (Atlas spec)? If not, this issue might be a good trigger to add one.

## 👨‍💻 Kai — Frontend Engineer This is a backend issue, but it has a direct frontend implication: whatever password rules the backend enforces, the signup form (A2) and the join-household form (A4) need to communicate them clearly to the user before they submit. **Frontend impact of each proposed fix:** - **Max length cap**: If the backend adds `@Size(max = 72)` (or whatever the BCrypt boundary is), the `<input type="password">` should have a matching `maxlength` attribute so users aren't cut off silently. I'd set `maxlength="128"` on the input and let the backend enforce the actual BCrypt limit — but the two need to agree. - **HIBP integration**: If a server-side HIBP check is added, the form needs to handle a new error response from the backend. Right now signup probably handles email-already-exists (409) errors. We'd need to handle a "password found in breach database" response too — with a clear, non-alarming message for the user (something like "This password has been seen in data breaches. Please choose a different one."). - **Character diversity rules**: If rules like "must contain a number" are added, the frontend can show a live password strength indicator. That's pure UI — no backend dependency — but it should mirror whatever rules the backend enforces exactly. **Questions for me before implementation:** - Will HIBP checks happen synchronously on form submit (user waits for the result), or asynchronously? Async adds UI complexity. - What HTTP status code does the backend return for a breached password — 422 (validation failure) or something else? I need to know the error shape to map it into form error state correctly. - Is there a password strength indicator in the design (Atlas spec)? If not, this issue might be a good trigger to add one.

marcel commented

2026-04-03 09:27:48 +02:00

🔧 Backend Engineer — Password Complexity (Issue #15)

Good catch. The BCrypt DoS vector is the more pressing of the two concerns. Let me break down both recommendations:

BCrypt max length (higher priority):

BCrypt silently truncates input at 72 bytes. A 10,000-character password is hashed at full BCrypt cost just the same — this is an application-layer DoS: an attacker can hammer the signup endpoint with huge passwords and drive CPU usage. Adding @Size(max = 72) to SignupRequest.password closes this with zero friction.
One nuance: 72 bytes, not 72 characters. A string of 72 ASCII characters is 72 bytes, but emoji or multibyte Unicode chars are larger per character. Enforcing @Size(max = 72) at the Java String level (characters) is a safe approximation that slightly undershoots — that's fine. Document the reason in a comment so no one "optimizes" it away later.
The fix is a one-liner: @NotBlank @Size(min = 8, max = 72) String password. Add a validation test for the boundary (71, 72, 73 chars).

HIBP integration (lower priority, worth discussing):

The k-anonymity API approach is elegant: hash the password with SHA-1, send the first 5 hex chars to HIBP, check the response. No plaintext password leaves the server.
The risk to weigh: this adds a network call to the signup path. HIBP API downtime = signup broken (unless we make it fail-open, which defeats the purpose). Consider: fail-open with a warning log, or cache the HIBP response list locally?
Alternatively, ship with just the max-length fix now and add HIBP as a separate, non-blocking improvement later.

Questions:

Should the max-length fix go in as a quick PR now (it's genuinely trivial), or is it being bundled with the HIBP work? I'd advocate for shipping the DoS fix immediately — it's a one-line change with high safety value.
Is there a PasswordValidator abstraction planned, or is validation staying in the Bean Validation annotations? If we're adding HIBP, a dedicated PasswordValidationService would be cleaner than stuffing it into a custom @Constraint.

## 🔧 Backend Engineer — Password Complexity (Issue #15) Good catch. The BCrypt DoS vector is the more pressing of the two concerns. Let me break down both recommendations: **BCrypt max length (higher priority):** - BCrypt silently truncates input at 72 bytes. A 10,000-character password is hashed at full BCrypt cost just the same — this is an application-layer DoS: an attacker can hammer the signup endpoint with huge passwords and drive CPU usage. Adding `@Size(max = 72)` to `SignupRequest.password` closes this with zero friction. - One nuance: 72 bytes, not 72 characters. A string of 72 ASCII characters is 72 bytes, but emoji or multibyte Unicode chars are larger per character. Enforcing `@Size(max = 72)` at the Java String level (characters) is a safe approximation that slightly undershoots — that's fine. Document the reason in a comment so no one "optimizes" it away later. - The fix is a one-liner: `@NotBlank @Size(min = 8, max = 72) String password`. Add a validation test for the boundary (71, 72, 73 chars). **HIBP integration (lower priority, worth discussing):** - The k-anonymity API approach is elegant: hash the password with SHA-1, send the first 5 hex chars to HIBP, check the response. No plaintext password leaves the server. - The risk to weigh: this adds a network call to the signup path. HIBP API downtime = signup broken (unless we make it fail-open, which defeats the purpose). Consider: fail-open with a warning log, or cache the HIBP response list locally? - Alternatively, ship with just the max-length fix now and add HIBP as a separate, non-blocking improvement later. **Questions:** - Should the max-length fix go in as a quick PR now (it's genuinely trivial), or is it being bundled with the HIBP work? I'd advocate for shipping the DoS fix immediately — it's a one-line change with high safety value. - Is there a `PasswordValidator` abstraction planned, or is validation staying in the Bean Validation annotations? If we're adding HIBP, a dedicated `PasswordValidationService` would be cleaner than stuffing it into a custom `@Constraint`.

marcel commented

2026-04-03 09:27:59 +02:00

🧪 QA Engineer — Password Complexity (Issue #15)

The current test coverage for password validation is presumably just the happy path and the @Size(min = 8) rejection. Here's what I'd want covered after any fix lands.

Tests for the BCrypt max-length fix (if @Size(max = 72) is added):

shouldAcceptPasswordAtMinimumLength()          // 8 chars → 201
shouldAcceptPasswordAtMaximumLength()          // 72 chars → 201
shouldRejectPasswordJustBelowMinimum()         // 7 chars → 422
shouldRejectPasswordJustAboveMaximum()         // 73 chars → 422
shouldHandleMultibyteCharactersInPassword()    // emoji/unicode, ensure byte vs char boundary is correct

Tests for HIBP integration (if implemented):

shouldRejectPasswordFoundInBreachDatabase()
shouldAcceptPasswordNotFoundInBreachDatabase()
shouldAllowSignupWhenHIBPApiIsUnreachable()   // fail-open behavior must be tested and documented
shouldNotLogOrExposePasswordInHIBPErrorResponse()

Integration test concern:

The HIBP check makes a real HTTP call from the service layer. Integration tests must mock the HIBP client — we can't have tests hitting a live third-party API. How is the HIBP client being injected? If it's a Spring bean, mocking it with @MockBean in the test is straightforward. If it's a static call, it's harder to test and harder to fail-open.

Existing tests to verify still pass:

Signup happy path with a valid password should continue to return 201
All existing auth integration tests should remain green — the change should be purely additive

Questions:

Is there currently an integration test that exercises the password validation path? If not, this issue is a good opportunity to add one for the validation boundary, regardless of the HIBP decision.

## 🧪 QA Engineer — Password Complexity (Issue #15) The current test coverage for password validation is presumably just the happy path and the `@Size(min = 8)` rejection. Here's what I'd want covered after any fix lands. **Tests for the BCrypt max-length fix (if `@Size(max = 72)` is added):** ``` shouldAcceptPasswordAtMinimumLength() // 8 chars → 201 shouldAcceptPasswordAtMaximumLength() // 72 chars → 201 shouldRejectPasswordJustBelowMinimum() // 7 chars → 422 shouldRejectPasswordJustAboveMaximum() // 73 chars → 422 shouldHandleMultibyteCharactersInPassword() // emoji/unicode, ensure byte vs char boundary is correct ``` **Tests for HIBP integration (if implemented):** ``` shouldRejectPasswordFoundInBreachDatabase() shouldAcceptPasswordNotFoundInBreachDatabase() shouldAllowSignupWhenHIBPApiIsUnreachable() // fail-open behavior must be tested and documented shouldNotLogOrExposePasswordInHIBPErrorResponse() ``` **Integration test concern:** - The HIBP check makes a real HTTP call from the service layer. Integration tests must mock the HIBP client — we can't have tests hitting a live third-party API. How is the HIBP client being injected? If it's a Spring bean, mocking it with `@MockBean` in the test is straightforward. If it's a static call, it's harder to test and harder to fail-open. **Existing tests to verify still pass:** - Signup happy path with a valid password should continue to return 201 - All existing auth integration tests should remain green — the change should be purely additive **Questions:** - Is there currently an integration test that exercises the password validation path? If not, this issue is a good opportunity to add one for the validation boundary, regardless of the HIBP decision.

marcel commented

2026-04-03 09:28:11 +02:00

🔐 Sable — Security Engineer

Good issue — this is exactly the kind of low-severity finding that gets ignored until it isn't. Let me add some depth to both recommendations.

BCrypt DoS — this should be treated as Medium, not Low:

The attack is trivial: a script that POSTs 10,000-byte passwords to /signup repeatedly. BCrypt at cost factor 12 is intentionally slow — ~200–400ms per hash. Each oversized request holds a thread for that duration with zero authentication required. At modest concurrency this could saturate the thread pool.
Fix: @Size(max = 72) in SignupRequest. Ship this immediately. It's a one-line fix with a test, and it closes the vector entirely.
Note: if you're using Spring's default thread pool, check the max-threads configuration too — this is a useful defense-in-depth measure regardless.

HIBP integration — genuinely useful, worth doing thoughtfully:

The k-anonymity model is correct: SHA-1 the password, send 5-char prefix, receive matching suffix list, check locally. The plaintext password never leaves the JVM.
Key implementation requirements if this goes in:
- The HIBP client must have a hard timeout (e.g., 2 seconds). Never let a third-party API block a signup indefinitely.
- Fail-open is the only acceptable behavior for a third-party dependency — but log a warning so we know HIBP is down.
- The error message to the user must not reveal that SHA-1 is used or how the check works — just "this password has appeared in a data breach, please choose another."
- Do not log the password or the full SHA-1 hash in any error or debug output.

Additional hardening to consider alongside this issue:

Rate limiting on /signup endpoint — without it, the BCrypt fix reduces per-request cost but doesn't prevent volume attacks.
Consider adding @RateLimit or Spring's built-in throttling once the max-length is in place.

Questions:

Is there currently any rate limiting on the auth endpoints (/signup, /login)? If not, that's a related finding worth opening as a separate issue.

## 🔐 Sable — Security Engineer Good issue — this is exactly the kind of low-severity finding that gets ignored until it isn't. Let me add some depth to both recommendations. **BCrypt DoS — this should be treated as Medium, not Low:** - The attack is trivial: a script that POSTs 10,000-byte passwords to `/signup` repeatedly. BCrypt at cost factor 12 is intentionally slow — ~200–400ms per hash. Each oversized request holds a thread for that duration with zero authentication required. At modest concurrency this could saturate the thread pool. - Fix: `@Size(max = 72)` in `SignupRequest`. Ship this immediately. It's a one-line fix with a test, and it closes the vector entirely. - Note: if you're using Spring's default thread pool, check the max-threads configuration too — this is a useful defense-in-depth measure regardless. **HIBP integration — genuinely useful, worth doing thoughtfully:** - The k-anonymity model is correct: SHA-1 the password, send 5-char prefix, receive matching suffix list, check locally. The plaintext password never leaves the JVM. - Key implementation requirements if this goes in: - The HIBP client must have a hard timeout (e.g., 2 seconds). Never let a third-party API block a signup indefinitely. - Fail-open is the only acceptable behavior for a third-party dependency — but log a warning so we know HIBP is down. - The error message to the user must not reveal that SHA-1 is used or how the check works — just "this password has appeared in a data breach, please choose another." - Do not log the password or the full SHA-1 hash in any error or debug output. **Additional hardening to consider alongside this issue:** - Rate limiting on `/signup` endpoint — without it, the BCrypt fix reduces per-request cost but doesn't prevent volume attacks. - Consider adding `@RateLimit` or Spring's built-in throttling once the max-length is in place. **Questions:** - Is there currently any rate limiting on the auth endpoints (`/signup`, `/login`)? If not, that's a related finding worth opening as a separate issue.

marcel commented

2026-04-03 09:28:22 +02:00

🎨 Atlas — UI/UX Designer

This is a backend security issue, but password rules always have a UX dimension. A few thoughts from the design side.

If a max length is added:

Users should never hit an invisible wall. If the backend enforces 72 chars max, the password input needs a clear maxlength attribute — silently truncating input in the browser (or worse, accepting it and rejecting server-side) is a poor experience.
We don't need to announce "maximum 72 characters" in the label — that's an unusual and slightly alarming constraint to surface. But the input should prevent going beyond it gracefully.

If HIBP or character diversity rules are added:

This is the right moment to add a password strength indicator to the signup and join-household forms. A simple 3-state indicator (Weak / Good / Strong) using our color system (--color-error / --yellow-text / --green-deeper) would surface feedback before the user submits — reducing failed submission frustration.
The error message for a breached password needs copy: I'd suggest "This password has appeared in a data breach. Please choose a different one." — factual, non-blaming, actionable.
Character diversity rules (if added) should be shown as a checklist beneath the input — not only shown on error. Users shouldn't have to guess the rules.

Consistency check:

The join-household form (A4, issue #21) has the same password field and the same rules should apply. Whatever we spec for the signup form (A2) must be reflected in A4 as well. One design, two places.

Questions:

Is a password strength indicator planned anywhere in the current specs? If not, this issue may be the right trigger to add it — it's low-cost UI and high UX value for a security-motivated change.

## 🎨 Atlas — UI/UX Designer This is a backend security issue, but password rules always have a UX dimension. A few thoughts from the design side. **If a max length is added:** - Users should never hit an invisible wall. If the backend enforces 72 chars max, the password input needs a clear `maxlength` attribute — silently truncating input in the browser (or worse, accepting it and rejecting server-side) is a poor experience. - We don't need to announce "maximum 72 characters" in the label — that's an unusual and slightly alarming constraint to surface. But the input should prevent going beyond it gracefully. **If HIBP or character diversity rules are added:** - This is the right moment to add a password strength indicator to the signup and join-household forms. A simple 3-state indicator (Weak / Good / Strong) using our color system (`--color-error` / `--yellow-text` / `--green-deeper`) would surface feedback before the user submits — reducing failed submission frustration. - The error message for a breached password needs copy: I'd suggest "This password has appeared in a data breach. Please choose a different one." — factual, non-blaming, actionable. - Character diversity rules (if added) should be shown as a checklist beneath the input — not only shown on error. Users shouldn't have to guess the rules. **Consistency check:** - The join-household form (A4, issue #21) has the same password field and the same rules should apply. Whatever we spec for the signup form (A2) must be reflected in A4 as well. One design, two places. **Questions:** - Is a password strength indicator planned anywhere in the current specs? If not, this issue may be the right trigger to add it — it's low-cost UI and high UX value for a security-motivated change.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#15