Skip to content

feat: password reset#1488

Merged
tankerkiller125 merged 6 commits into
mainfrom
mk/pass-reset
May 16, 2026
Merged

feat: password reset#1488
tankerkiller125 merged 6 commits into
mainfrom
mk/pass-reset

Conversation

@tankerkiller125
Copy link
Copy Markdown
Contributor

@tankerkiller125 tankerkiller125 commented May 12, 2026

What type of PR is this?

  • feature

What this PR does / why we need it:

Adds password reset capabilities to Homebox so people can reset passwords without opening up DB files and what not.

Which issue(s) this PR fixes:

Part of #439

Testing

Validate both with SMTP based password reset, and the CLI reset homebox reset-password --email=<email>

Summary by CodeRabbit

  • New Features

    • Self-service password recovery: "Forgot Password" and "Reset Password" pages, plus API and client methods to request and apply email-based resets.
    • Admin CLI: generate one-time password-reset links from the command line.
  • Maintenance

    • Background task added to purge expired/used reset tokens.
    • DB migration to store single-use password reset tokens.
  • Documentation

    • Public API spec updated to document password-reset endpoints.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds end-to-end email password reset: migrations, repo + atomic token claim, service methods, handlers and SecureBaseURL, CLI offline link generation, frontend pages/API client, OpenAPI/Swagger updates, daily purge task, and tests.

Changes

Password Reset Feature

Layer / File(s) Summary
End-to-end password reset implementation
backend/internal/data/migrations/*, backend/internal/data/repo/*, backend/internal/core/services/*, backend/app/api/*, frontend/*, docs/public/api/*, backend/go.mod, backend/pkgs/*
Adds DB migrations for password_reset_tokens, repository methods (Create, GetValidByHash, MarkUsed, ConsumeAndChangePassword, PurgeExpired), TokenRepository.DeleteAllByUser, mailer wiring and service methods (MailerReady, RequestPasswordReset, GenerateResetLink, ResetPassword), SecureBaseURL and handlers, CLI offline reset-link generator and DB/migration helpers, frontend pages and API client methods, OpenAPI/Swagger updates, recurring purge task, tests, and dependency updates.

Sequence Diagram (high-level)

sequenceDiagram
  participant Client
  participant V1Controller
  participant UserService
  participant PasswordResetRepo
  participant Mailer
  participant TokenRepo
  Client->>V1Controller: POST /forgot-password (email)
  V1Controller->>UserService: RequestPasswordReset(ctx, email, baseURL)
  UserService->>PasswordResetRepo: Create(userID, tokenHash, expiresAt)
  PasswordResetRepo-->>UserService: token record
  UserService->>Mailer: Send reset email (resetURL)
  Client->>V1Controller: POST /reset-password (token,password)
  V1Controller->>UserService: ResetPassword(ctx, token, password)
  UserService->>PasswordResetRepo: GetValidByHash(hash)
  PasswordResetRepo-->>UserService: token + user
  UserService->>PasswordResetRepo: ConsumeAndChangePassword(tokenID,userID,hashedPassword)
  UserService->>TokenRepo: DeleteAllByUser(userID)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • katosdev
  • tonyaellie

Security Recommendations

  • Verify tokens are generated with a cryptographically secure RNG and only token hashes are stored.
  • Ensure token creation, conditional claim, and password update execute inside a single DB transaction.
  • Maintain timing-equalized dummy-hash on invalid token lookups to prevent oracles.
  • Rate-limit forgot/reset endpoints and monitor for enumeration attempts.
  • Restrict and audit CLI offline reset-link generation use.
  • Ensure HTML email content escapes interpolated values and review for injection/XSS.
  • Confirm purge cadence and expiry durations meet policy/compliance requirements.

Poem

🔑 A token sent to set things right,
One-time link that fades with night,
Claimed once, hashed, and safely stored,
Mail queued, purges run, tests roared—
CLI and UI stitched the flight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.19% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: password reset' is concise and clearly describes the main feature addition. It directly corresponds to the primary change in the changeset.
Description check ✅ Passed The PR description provides the required sections (type, purpose, issue reference, and testing details) and adequately explains the password reset feature. However, some implementation details and testing notes could be more comprehensive.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mk/pass-reset

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot requested review from katosdev and tonyaellie May 12, 2026 01:05
@coderabbitai coderabbitai Bot added ⬆️ enhancement New feature or request review needed A review is needed on this PR or Issue go Pull requests that update Go code labels May 12, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 14

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/public/api/swagger-2.0.json (1)

5920-5932: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Duplicate enum value in usergroup.Role definition.

The usergroup.Role enum contains duplicate "user" values:

"enum": ["user", "user", "owner"]

This creates ambiguity and will cause issues:

  • Swagger/OpenAPI validators may reject this
  • Code generators will produce incorrect type definitions
  • Which "user" maps to DefaultRole vs RoleUser is unclear

Based on the x-enum-varnames, the values should likely be distinct. Recommend:

"enum": ["default", "user", "owner"]

or clarify if DefaultRole and RoleUser should be the same value.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/public/api/swagger-2.0.json` around lines 5920 - 5932, Update the
usergroup.Role enum to remove the duplicate "user" value so enum entries align
with x-enum-varnames; specifically modify the "enum" array for usergroup.Role to
use distinct values matching DefaultRole, RoleUser, RoleOwner (e.g., replace the
first "user" with "default" so the enum becomes ["default","user","owner"]) or,
if DefaultRole and RoleUser are intentionally identical, update x-enum-varnames
to reflect that mapping; ensure usergroup.Role's "enum" and "x-enum-varnames"
are consistent.
🧹 Nitpick comments (6)
backend/internal/core/services/service_user_password_reset.go (1)

37-77: Add security audit events for reset lifecycle actions.

Consider emitting a dedicated security/audit event (request accepted, token redeemed, token invalid/expired, revoke_sessions_failed) with consistent fields for alerting and incident review.

Also applies to: 110-175

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/core/services/service_user_password_reset.go` around lines
37 - 77, Add explicit security/audit events for the reset lifecycle by calling a
consistent audit emitter (e.g. svc.EmitSecurityEvent or a new svc.Audit method)
at the key places: in RequestPasswordReset after successful token creation/send
emit "password_reset.request_accepted" with metadata (user.id, email.length,
base_url.length, link.length), on the ent.IsNotFound and
errResetUserHasNoPassword branches emit a silent/ignored
"password_reset.request_ignored" (include email length only, no PII), and in the
redeem flow (RedeemPasswordResetToken) emit "password_reset.token_redeemed" on
success (user.id), "password_reset.token_invalid_or_expired" when the token is
invalid/expired, and if session revocation fails (where RevokeSessionsForUser or
similar is called) emit "password_reset.revoke_sessions_failed" including the
error; ensure field names are consistent across events and avoid logging raw
tokens or emails where not needed.
docs/public/api/swagger-2.0.yaml (1)

3731-3754: Add explicit abuse-protection signaling for unauthenticated reset endpoints.

Consider documenting/returning 429 for /v1/users/forgot-password and /v1/users/reset-password, and wiring endpoint-level rate limiting/alerts. These routes are prime abuse targets and benefit from explicit contract + observability.

Also applies to: 3854-3877

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/public/api/swagger-2.0.yaml` around lines 3731 - 3754, Add explicit
abuse-protection signaling and wiring for the unauthenticated reset endpoints by
updating the OpenAPI docs for the POST /v1/users/forgot-password and POST
/v1/users/reset-password paths to include a "429" response with a short
description (e.g., "Too Many Requests — rate limit exceeded") and an optional
retry-after header/schema, and then wire endpoint-level rate limiting and
alerting in the service layer where these routes are handled (apply the
rate-limit middleware/guard to the handlers for /v1/users/forgot-password and
/v1/users/reset-password and emit metrics/events on throttling to your
observability system so alerts can be created).
backend/app/api/handlers/v1/v1_ctrl_auth.go (2)

168-214: 🏗️ Heavy lift

Use adapters.Action for these POST body handlers instead of manual decode.

Both handlers manually parse request bodies. For consistency with the v1 handler pattern, switch to adapter-based request handling for POST JSON endpoints.

As per coding guidelines, "Use the adapter pattern from internal/web/adapters ... instead of manual request handling in handlers" and "Use adapters.Action for POST endpoints that accept a request body".

Also applies to: 228-262

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_auth.go` around lines 168 - 214, Summary:
Replace manual request body decoding in HandleForgotPassword (and the POST
handler at 228-262) with the adapters.Action pattern. Fix: convert
HandleForgotPassword to use internal/web/adapters.Action so the framework
performs JSON binding of ForgotPasswordRequest (remove server.Decode and manual
trimming/length-checks where adapters provide binding/validation hooks), then
call ctrl.svc.User.RequestPasswordReset(spanCtx, req.Email, baseURL) inside the
Action and return the same HTTP results via the adapter response path; ensure
you preserve the span attributes (forgot.outcome, user.email.length), the mailer
readiness check, GetHBURL use, and error handling (recordCtrlSpanError +
service_failed -> internal error) when migrating. Apply the same conversion to
the other POST JSON endpoint referenced (lines 228-262) so both use
adapters.Action instead of manual decoding.

183-188: ⚡ Quick win

Add explicit trim/empty checks for reset inputs (security hardening).

Line 248 forwards raw token/password directly to the service. Add strings.TrimSpace + empty checks in-handler so malformed input fails fast and doesn’t depend solely on downstream validation.

Suggested hardening
 var body ResetPasswordRequest
 if err := server.Decode(r, &body); err != nil {
   span.SetAttributes(attribute.String("reset.outcome", "decode_failed"))
   return validate.NewRequestError(err, http.StatusBadRequest)
 }
+body.Token = strings.TrimSpace(body.Token)
+body.NewPassword = strings.TrimSpace(body.NewPassword)
+if body.Token == "" || body.NewPassword == "" {
+  span.SetAttributes(attribute.String("reset.outcome", "missing_fields"))
+  return validate.NewRequestError(errors.New("token and password are required"), http.StatusBadRequest)
+}
 span.SetAttributes(
   attribute.Int("token.length", len(body.Token)),
   attribute.Int("password.new.length", len(body.NewPassword)),
 )

Also applies to: 238-248

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/handlers/v1/v1_ctrl_auth.go` around lines 183 - 188, The
handler currently forwards raw token/password to the service; add in-handler
trimming and empty checks so malformed inputs fail fast: apply strings.TrimSpace
to body.Token and body.Password (and any other reset fields), set span
attributes like attribute.Int("reset.token.length", len(body.Token)) /
attribute.Int("reset.password.length", len(body.Password)), and if any are empty
return validate.NewRequestError(errors.New("<field> is required"),
http.StatusBadRequest); do this alongside the existing email trim/check
(body.Email) in the same handler so validation is consistent before calling the
service.
docs/public/api/swagger-2.0.json (2)

2664-2697: 💤 Low value

Consider generic error message to prevent token state enumeration.

The 400 error description distinguishes between "invalid token, expired token, or missing field." While less critical than other issues, providing specific error states could help attackers:

  • Distinguishing "expired" from "invalid" reveals token lifecycle information
  • This is a minor timing-attack vector

Consider a generic error message like "Password reset failed. Please request a new reset link." The session revocation on success is good security practice.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/public/api/swagger-2.0.json` around lines 2664 - 2697, The 400 response
for the POST "/v1/users/reset-password" currently enumerates token states
("invalid token, expired token, or missing field"); update the 400 response
description in the swagger definition for that operation to a generic error
message (e.g., "Password reset failed. Please request a new reset link.") to
avoid revealing token state/timing info while keeping the 204 success and
session-revocation behavior unchanged.

6048-6056: ⚡ Quick win

Missing validation constraints in schema.

The ForgotPasswordRequest schema lacks validation constraints:

  • No required field array
  • No email format validation ("format": "email")
  • No maxLength to prevent abuse

While backend validation may be implemented, documenting these constraints in the OpenAPI spec improves client-side validation and API clarity.

🔒 Proposed schema enhancement
 "v1.ForgotPasswordRequest": {
     "type": "object",
+    "required": [
+        "email"
+    ],
     "properties": {
         "email": {
             "type": "string",
+            "format": "email",
+            "maxLength": 255,
             "example": "user@example.com"
         }
     }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/public/api/swagger-2.0.json` around lines 6048 - 6056, The
ForgotPasswordRequest schema is missing validation constraints — update the
"v1.ForgotPasswordRequest" object so its "email" property includes "format":
"email" and a reasonable "maxLength" (e.g., 254) and add a top-level "required":
["email"] array; ensure you modify the schema for v1.ForgotPasswordRequest so
the OpenAPI spec documents the required field and email format/length
constraints for client-side validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/cli_reset_password.go`:
- Around line 49-60: The CLI currently trims the email only for emptiness but
then passes the original raw *email to generateResetLinkOffline, causing lookups
to fail for inputs like " user@example.com ". Normalize the input once and reuse
it: call strings.TrimSpace on *email (and optionally strings.ToLower if your
service expects lowercase) and store the result in a local variable (e.g.,
trimmedEmail) after the emptiness check, then pass that normalized value to
generateResetLinkOffline instead of *email; update any other uses of *email in
this function to use the normalized variable.

In `@backend/app/api/handlers/v1/v1_ctrl_auth.go`:
- Around line 157-167: Update the Swagger comments for the forgot-password and
reset-password handlers so they match actual runtime responses: for
HandleForgotPassword add `@Produce` application/json, include `@Success` 204 and
`@Failure` entries for 400, 403, 500, and 503 with appropriate descriptions; for
HandleResetPassword (reset handler) add `@Produce` application/json, include
`@Success` and `@Failure` entries for 200/204 as used and for 400, 403, and 500 as
returned; also include `@Summary`, `@Tags`, `@Router` and `@Security` annotations per
guidelines so the generated docs/contracts align with the actual status codes
and security requirements (refer to HandleForgotPassword and HandleResetPassword
identifiers in v1_ctrl_auth.go).

In `@backend/internal/core/services/service_user_password_reset.go`:
- Around line 146-158: Wrap the password update and token consumption in a
single DB transaction: begin a transaction via the repository/DB layer, call
svc.repos.Users.ChangePassword(ctxTx, tok.UserID, hashed) then
svc.repos.PasswordResetTokens.MarkUsed(ctxTx, tok.ID, time.Now()), and commit on
success or rollback on any error; on errors call recordServiceSpanError(span,
err), set the appropriate span attribute (e.g., "reset.outcome" to
"persist_failed" or "mark_used_failed"), and return the error. Ensure you use
the transaction-aware context/tx objects for both calls (replace ctx with ctxTx
or pass the tx to repo methods as your repo API requires) so the change and
mark-used are atomic, and only proceed to session revocation after a successful
commit.

In
`@backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql`:
- Around line 17-19: There's a redundant non-unique index on the token column:
remove the non-unique index named "passwordresettokens_token" on the
"password_reset_tokens" table because the unique index
"password_reset_tokens_token_key" already enforces uniqueness and serves
lookups; keep the unique index "password_reset_tokens_token_key" and the
"passwordresettokens_user_id" index intact.

In
`@backend/internal/data/migrations/sqlite3/20260512000000_add_password_reset_tokens.sql`:
- Around line 21-25: Remove the redundant non-unique index creation by deleting
the "create index if not exists passwordresettokens_token on
password_reset_tokens (token);" statement from the migration; the unique index
"password_reset_tokens_token_key" already enforces uniqueness and provides the
lookup performance for the token column in the password_reset_tokens table.

In `@docs/public/api/openapi-3.0.json`:
- Around line 6260-6268: The OpenAPI schemas v1.ForgotPasswordRequest and the
reset-password schema (e.g., v1.ResetPasswordRequest) are missing required
arrays; update v1.ForgotPasswordRequest to include "required": ["email"] and
update the reset password schema to include "required": ["token","password"] at
the same object level as "properties" so clients know these fields are mandatory
and align with server validation.
- Around line 2658-2690: The OpenAPI entry for "/v1/users/forgot-password"
currently only documents a 204 and a plain-string 400 response which is
underspecified; update the responses to list every non-204 outcome the API can
actually return (e.g., 400 for bad request, 401/403 if applicable, 500 for
server errors) and replace the plain string schema for the 400 response with the
project's structured error schema (use a $ref to the canonical error component
such as components/schemas/Error or ErrorResponse) so generated clients preserve
typed failures; make the same changes for the other affected endpoint block
around lines 2856-2888 (the other user/password endpoint) and ensure the
requestBody reference to v1.ForgotPasswordRequest remains unchanged.

In `@docs/public/api/openapi-3.0.yaml`:
- Around line 1575-1600: Update the OpenAPI spec for the
/v1/users/forgot-password and /v1/users/reset-password operations to match the
handler behavior in v1_ctrl_auth.go: replace the misleading "Always returns 204"
text and change the documented responses to 403 (local login disabled), 503
(SMTP not configured) and 400 (missing/invalid email), and ensure all error
responses reference the JSON error schema used by
validate.NewRequestError()/validate.ErrorResponse rather than plain text; keep
rate-limit notes unchanged since a.mwAuthRateLimit already wraps these routes.
- Around line 4072-4078: Update the OpenAPI schemas and server validations: in
the spec add required: [token, password] to v1.ResetPasswordRequest and annotate
password with format: password, writeOnly: true, minLength: 8; apply the same
password property (format/writeOnly/minLength) to v1.ChangePassword and
services.UserRegistration schemas; then update server-side handlers that
currently check newPassword == "" (reset-password, change-password,
registration) to validate minimum length (>= 8) and return the existing 400
response path when the check fails so the API enforces the same constraint as
the spec.
- Around line 4015-4020: Update the schema and underlying Go struct so the API
contract requires and validates emails: mark the email property as required and
add format: email in the OpenAPI schema for v1.ForgotPasswordRequest, and in the
Go request struct (the struct used to generate this schema) add the binding tag
`binding:"required,email"` and the swag annotation that sets `format: email`
(e.g., `// `@param` Email body string true "email" format(email)` or the project’s
equivalent), then regenerate the OpenAPI YAML so `v1.ForgotPasswordRequest`
reflects a required, email-formatted field and prevents malformed/empty values
at the API boundary.

In `@docs/public/api/swagger-2.0.json`:
- Around line 2475-2508: The OpenAPI spec for the "/v1/users/forgot-password"
POST declares that the endpoint "Always returns 204" but also documents a 400
response for configuration issues, which leaks information; update the spec so
behavior is consistent: either remove the 400 response and document only 204
(and ensure server logs configuration errors internally), or change the endpoint
to return 400 for all requests when the feature is globally disabled;
specifically modify the "responses" block under the "/v1/users/forgot-password"
POST (and adjust any consumer-facing description) so it no longer exposes a
conditional 400, keeping the schema reference to v1.ForgotPasswordRequest
unchanged.
- Around line 6134-6144: The schema v1.ResetPasswordRequest is missing
validation constraints; update it to include a required array
["password","token"], add a minLength (e.g., 8) on the password property (and
optionally maxLength), and add validation for token (e.g., minLength or a
pattern) so both fields are enforced by the OpenAPI spec; modify the properties
for "password" and "token" in v1.ResetPasswordRequest to include these
constraints and ensure the required field lists both names.

In `@frontend/pages/forgot-password.vue`:
- Around line 33-38: The check in the forgot-password response handler only
treats status === 503 as the "reset-email unavailable" case, but the API docs
also use 400 for that scenario; update the conditional that currently references
status (and calls toast.error(t("index.toast.forgot_password_unavailable"))) to
handle both 503 and 400 (e.g., status === 503 || status === 400) so users see
the actionable unavailable message instead of the generic invalid-email flow.
- Around line 23-30: Replace the manual refs (email, loading, submitted) and the
submit() logic with vee-validate's useForm<T>() to get type-safe form state and
handlers: define a form schema/type for the email field, use useForm<{email:
string}>() and its handleSubmit to run the submit flow, and replace direct
email.value access with form.values.email (but keep trimming the value before
calling api.forgotPassword). Keep the loading and submitted semantics (you can
use form.meta.submitting or a dedicated loading ref tied to handleSubmit and a
submitted flag stored in form state), and ensure existing security behavior
remains: trim the email, preserve generic error messages for rate
limiting/network errors, and maintain the explicit SMTP-unavailable handling
around the api.forgotPassword response; update the template bindings (the form
at lines ~72-82) to use the form's fields instead of the old email ref.

---

Outside diff comments:
In `@docs/public/api/swagger-2.0.json`:
- Around line 5920-5932: Update the usergroup.Role enum to remove the duplicate
"user" value so enum entries align with x-enum-varnames; specifically modify the
"enum" array for usergroup.Role to use distinct values matching DefaultRole,
RoleUser, RoleOwner (e.g., replace the first "user" with "default" so the enum
becomes ["default","user","owner"]) or, if DefaultRole and RoleUser are
intentionally identical, update x-enum-varnames to reflect that mapping; ensure
usergroup.Role's "enum" and "x-enum-varnames" are consistent.

---

Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_auth.go`:
- Around line 168-214: Summary: Replace manual request body decoding in
HandleForgotPassword (and the POST handler at 228-262) with the adapters.Action
pattern. Fix: convert HandleForgotPassword to use internal/web/adapters.Action
so the framework performs JSON binding of ForgotPasswordRequest (remove
server.Decode and manual trimming/length-checks where adapters provide
binding/validation hooks), then call ctrl.svc.User.RequestPasswordReset(spanCtx,
req.Email, baseURL) inside the Action and return the same HTTP results via the
adapter response path; ensure you preserve the span attributes (forgot.outcome,
user.email.length), the mailer readiness check, GetHBURL use, and error handling
(recordCtrlSpanError + service_failed -> internal error) when migrating. Apply
the same conversion to the other POST JSON endpoint referenced (lines 228-262)
so both use adapters.Action instead of manual decoding.
- Around line 183-188: The handler currently forwards raw token/password to the
service; add in-handler trimming and empty checks so malformed inputs fail fast:
apply strings.TrimSpace to body.Token and body.Password (and any other reset
fields), set span attributes like attribute.Int("reset.token.length",
len(body.Token)) / attribute.Int("reset.password.length", len(body.Password)),
and if any are empty return validate.NewRequestError(errors.New("<field> is
required"), http.StatusBadRequest); do this alongside the existing email
trim/check (body.Email) in the same handler so validation is consistent before
calling the service.

In `@backend/internal/core/services/service_user_password_reset.go`:
- Around line 37-77: Add explicit security/audit events for the reset lifecycle
by calling a consistent audit emitter (e.g. svc.EmitSecurityEvent or a new
svc.Audit method) at the key places: in RequestPasswordReset after successful
token creation/send emit "password_reset.request_accepted" with metadata
(user.id, email.length, base_url.length, link.length), on the ent.IsNotFound and
errResetUserHasNoPassword branches emit a silent/ignored
"password_reset.request_ignored" (include email length only, no PII), and in the
redeem flow (RedeemPasswordResetToken) emit "password_reset.token_redeemed" on
success (user.id), "password_reset.token_invalid_or_expired" when the token is
invalid/expired, and if session revocation fails (where RevokeSessionsForUser or
similar is called) emit "password_reset.revoke_sessions_failed" including the
error; ensure field names are consistent across events and avoid logging raw
tokens or emails where not needed.

In `@docs/public/api/swagger-2.0.json`:
- Around line 2664-2697: The 400 response for the POST
"/v1/users/reset-password" currently enumerates token states ("invalid token,
expired token, or missing field"); update the 400 response description in the
swagger definition for that operation to a generic error message (e.g.,
"Password reset failed. Please request a new reset link.") to avoid revealing
token state/timing info while keeping the 204 success and session-revocation
behavior unchanged.
- Around line 6048-6056: The ForgotPasswordRequest schema is missing validation
constraints — update the "v1.ForgotPasswordRequest" object so its "email"
property includes "format": "email" and a reasonable "maxLength" (e.g., 254) and
add a top-level "required": ["email"] array; ensure you modify the schema for
v1.ForgotPasswordRequest so the OpenAPI spec documents the required field and
email format/length constraints for client-side validation.

In `@docs/public/api/swagger-2.0.yaml`:
- Around line 3731-3754: Add explicit abuse-protection signaling and wiring for
the unauthenticated reset endpoints by updating the OpenAPI docs for the POST
/v1/users/forgot-password and POST /v1/users/reset-password paths to include a
"429" response with a short description (e.g., "Too Many Requests — rate limit
exceeded") and an optional retry-after header/schema, and then wire
endpoint-level rate limiting and alerting in the service layer where these
routes are handled (apply the rate-limit middleware/guard to the handlers for
/v1/users/forgot-password and /v1/users/reset-password and emit metrics/events
on throttling to your observability system so alerts can be created).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 25feb706-bb51-4875-9d07-7865fd6f2d8f

📥 Commits

Reviewing files that changed from the base of the PR and between 89d4a9b and 87986e3.

⛔ Files ignored due to path filters (30)
  • backend/app/api/static/docs/docs.go is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.yaml is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.yaml is excluded by !backend/app/api/static/docs/**
  • backend/go.sum is excluded by !**/*.sum
  • backend/internal/data/ent/client.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/ent.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/has_id.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/hook/hook.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/migrate/schema.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/mutation.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens/passwordresettokens.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens/where.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens_create.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens_delete.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens_query.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/passwordresettokens_update.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/predicate/predicate.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/runtime.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/schema/password_reset_tokens.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/schema/user.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/tx.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user/user.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user/where.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user_create.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user_query.go is excluded by !backend/internal/data/ent/**
  • backend/internal/data/ent/user_update.go is excluded by !backend/internal/data/ent/**
📒 Files selected for processing (25)
  • backend/app/api/cli_reset_password.go
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/app/api/main.go
  • backend/app/api/recurring.go
  • backend/app/api/routes.go
  • backend/go.mod
  • backend/internal/core/services/all.go
  • backend/internal/core/services/service_user.go
  • backend/internal/core/services/service_user_password_reset.go
  • backend/internal/core/services/service_user_password_reset_test.go
  • backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql
  • backend/internal/data/migrations/sqlite3/20260512000000_add_password_reset_tokens.sql
  • backend/internal/data/repo/repo_password_reset.go
  • backend/internal/data/repo/repo_tokens.go
  • backend/internal/data/repo/repos_all.go
  • docs/public/api/openapi-3.0.json
  • docs/public/api/openapi-3.0.yaml
  • docs/public/api/swagger-2.0.json
  • docs/public/api/swagger-2.0.yaml
  • frontend/lib/api/public.ts
  • frontend/lib/api/types/data-contracts.ts
  • frontend/locales/en.json
  • frontend/pages/forgot-password.vue
  • frontend/pages/index.vue
  • frontend/pages/reset-password.vue

Comment thread backend/app/api/cli_reset_password.go Outdated
Comment thread backend/app/api/handlers/v1/v1_ctrl_auth.go
Comment thread backend/internal/core/services/service_user_password_reset.go Outdated
Comment thread docs/public/api/openapi-3.0.yaml
Comment thread docs/public/api/swagger-2.0.json
Comment thread docs/public/api/swagger-2.0.json
Comment thread frontend/pages/forgot-password.vue
Comment thread frontend/pages/forgot-password.vue Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/go.mod`:
- Around line 61-67: The go.mod currently pulls vulnerable AWS SDK modules;
update the affected modules github.com/aws/aws-sdk-go-v2/service/s3 and
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream to the patched releases
(bump them to the minimum versions that include the GHSA-xmrv-pmrh-hhx2 fix,
e.g., upgrade via `go get
github.com/aws/aws-sdk-go-v2/service/s3@<patched-version>` and run `go mod
tidy`), then add a CI job named something like "dependency-scan" that runs
govulncheck (or an OSV/GHSA scanner) against the repo (e.g., `govulncheck
./...`) and fails the build on findings so future vulnerable ranges are caught
before merge.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5e555f33-b4b8-4e6e-871a-917b0745d1a6

📥 Commits

Reviewing files that changed from the base of the PR and between 87986e3 and 3784ff9.

📒 Files selected for processing (6)
  • backend/app/api/handlers/v1/helpers.go
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/go.mod
  • backend/internal/core/services/service_user_password_reset.go
  • backend/internal/core/services/service_user_password_reset_test.go
  • backend/internal/data/repo/repo_password_reset.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • backend/internal/data/repo/repo_password_reset.go
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/internal/core/services/service_user_password_reset_test.go
  • backend/internal/core/services/service_user_password_reset.go

Comment thread backend/app/api/handlers/v1/helpers.go Outdated
Comment thread backend/go.mod
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 12, 2026

Deploying homebox-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: f259bc3
Status:🚫  Build failed.

View logs

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/app/api/setup.go (1)

140-166: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Escape PostgreSQL connection fields instead of concatenating them.

The DSN built here breaks when user, password, or SSL file paths contain spaces, quotes, or backslashes. That makes perfectly valid generated secrets fail at startup and can misparse connection options. Build the URL with net/url/query params (or pgx config formatting) so these values are encoded safely.

Suggested fix
+import (
+	"net"
+	"net/url"
+)
+
 	case config.DriverPostgres:
-		databaseURL = fmt.Sprintf("host=%s port=%s dbname=%s sslmode=%s", cfg.Database.Host, cfg.Database.Port, cfg.Database.Database, cfg.Database.SslMode)
-		if cfg.Database.Username != "" {
-			databaseURL += fmt.Sprintf(" user=%s", cfg.Database.Username)
-		}
-		if cfg.Database.Password != "" {
-			databaseURL += fmt.Sprintf(" password=%s", cfg.Database.Password)
-		}
+		pgURL := &url.URL{
+			Scheme: "postgres",
+			Host:   net.JoinHostPort(cfg.Database.Host, cfg.Database.Port),
+			Path:   cfg.Database.Database,
+		}
+		if cfg.Database.Username != "" {
+			if cfg.Database.Password != "" {
+				pgURL.User = url.UserPassword(cfg.Database.Username, cfg.Database.Password)
+			} else {
+				pgURL.User = url.User(cfg.Database.Username)
+			}
+		}
+		q := pgURL.Query()
+		q.Set("sslmode", cfg.Database.SslMode)
 		if cfg.Database.SslRootCert != "" {
 			if _, err := os.Stat(cfg.Database.SslRootCert); err != nil {
 				log.Error().Err(err).Str("path", cfg.Database.SslRootCert).Msg("SSL root certificate file is not accessible")
 				return "", fmt.Errorf("SSL root certificate file is not accessible: %w", err)
 			}
-			databaseURL += fmt.Sprintf(" sslrootcert=%s", cfg.Database.SslRootCert)
+			q.Set("sslrootcert", cfg.Database.SslRootCert)
 		}
 		if cfg.Database.SslCert != "" {
 			if _, err := os.Stat(cfg.Database.SslCert); err != nil {
 				log.Error().Err(err).Str("path", cfg.Database.SslCert).Msg("SSL certificate file is not accessible")
 				return "", fmt.Errorf("SSL certificate file is not accessible: %w", err)
 			}
-			databaseURL += fmt.Sprintf(" sslcert=%s", cfg.Database.SslCert)
+			q.Set("sslcert", cfg.Database.SslCert)
 		}
 		if cfg.Database.SslKey != "" {
 			if _, err := os.Stat(cfg.Database.SslKey); err != nil {
 				log.Error().Err(err).Str("path", cfg.Database.SslKey).Msg("SSL key file is not accessible")
 				return "", fmt.Errorf("SSL key file is not accessible: %w", err)
 			}
-			databaseURL += fmt.Sprintf(" sslkey=%s", cfg.Database.SslKey)
+			q.Set("sslkey", cfg.Database.SslKey)
 		}
+		pgURL.RawQuery = q.Encode()
+		databaseURL = pgURL.String()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/setup.go` around lines 140 - 166, The current manual
concatenation of databaseURL using cfg.Database fields can break with
spaces/quotes/backslashes; replace the ad-hoc string buildup (the block that
sets databaseURL and appends user/password/sslrootcert/sslcert/sslkey/sslmode)
with a proper URL/DSN builder: create a url.URL with Scheme "postgres", set User
via url.UserPassword(cfg.Database.Username, cfg.Database.Password) when present,
set Host to cfg.Database.Host:cfg.Database.Port, set Path to
"/"+cfg.Database.Database, and encode sslmode and file paths into RawQuery or
use pgx.ParseConfig/pgx.ConnConfig to build a safe DSN; also keep the existing
file existence checks for SslRootCert/SslCert/SslKey but add them into the
encoded query parameters instead of raw string concatenation so values are
percent-encoded and robust.
🧹 Nitpick comments (2)
backend/internal/core/services/reporting/io_sheet_test.go (1)

136-156: ⚡ Quick win

Apply constant extraction consistently to "HB.field.3".

headerHBField2 was extracted to reduce duplication, but "HB.field.3" appears multiple times (6 occurrences per static analysis) and is still hardcoded. For consistency and maintainability, consider extracting it as a constant as well.

♻️ Suggested refactor
 // Repeated fixture column header used across header-parse test cases.
-const headerHBField2 = "HB.field.2"
+const (
+	headerHBField2 = "HB.field.2"
+	headerHBField3 = "HB.field.3"
+)

Then replace hardcoded "HB.field.3" occurrences with headerHBField3 in the test cases below.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/core/services/reporting/io_sheet_test.go` around lines 136 -
156, The test hardcodes "HB.field.3" in multiple places while headerHBField2 was
extracted; create a new constant (e.g., headerHBField3) and replace all literal
"HB.field.3" occurrences in the tests (places constructing rawHeaders,
wantHbHeaders keys, and wantFieldHeaders slices) with headerHBField3 so the
pattern matches headerHBField2's extraction and reduces duplication across
functions like the test cases building rawHeaders and assertions.
backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql (1)

11-15: ⚡ Quick win

Enforce the SHA-256 hash width at the schema level.

The comment says token stores a SHA-256 digest, but bytea currently accepts arbitrary lengths. A check constraint makes malformed inserts fail fast and hardens this security-sensitive table.

Suggested fix
-    "token"      bytea NOT NULL,
+    "token"      bytea NOT NULL CHECK (octet_length("token") = 32),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql`
around lines 11 - 15, The token column currently uses bytea with no length
enforcement; add a CHECK constraint on the password_reset_tokens table to
require the SHA-256 digest width (octet_length("token") = 32) so malformed
lengths fail at insert; add a descriptive constraint name (e.g.,
password_reset_tokens_token_len_check) alongside the existing PRIMARY KEY and
FOREIGN KEY constraints in the CREATE TABLE statement so the DB enforces token
length at schema level.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@backend/app/api/setup.go`:
- Around line 140-166: The current manual concatenation of databaseURL using
cfg.Database fields can break with spaces/quotes/backslashes; replace the ad-hoc
string buildup (the block that sets databaseURL and appends
user/password/sslrootcert/sslcert/sslkey/sslmode) with a proper URL/DSN builder:
create a url.URL with Scheme "postgres", set User via
url.UserPassword(cfg.Database.Username, cfg.Database.Password) when present, set
Host to cfg.Database.Host:cfg.Database.Port, set Path to
"/"+cfg.Database.Database, and encode sslmode and file paths into RawQuery or
use pgx.ParseConfig/pgx.ConnConfig to build a safe DSN; also keep the existing
file existence checks for SslRootCert/SslCert/SslKey but add them into the
encoded query parameters instead of raw string concatenation so values are
percent-encoded and robust.

---

Nitpick comments:
In `@backend/internal/core/services/reporting/io_sheet_test.go`:
- Around line 136-156: The test hardcodes "HB.field.3" in multiple places while
headerHBField2 was extracted; create a new constant (e.g., headerHBField3) and
replace all literal "HB.field.3" occurrences in the tests (places constructing
rawHeaders, wantHbHeaders keys, and wantFieldHeaders slices) with headerHBField3
so the pattern matches headerHBField2's extraction and reduces duplication
across functions like the test cases building rawHeaders and assertions.

In
`@backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql`:
- Around line 11-15: The token column currently uses bytea with no length
enforcement; add a CHECK constraint on the password_reset_tokens table to
require the SHA-256 digest width (octet_length("token") = 32) so malformed
lengths fail at insert; add a descriptive constraint name (e.g.,
password_reset_tokens_token_len_check) alongside the existing PRIMARY KEY and
FOREIGN KEY constraints in the CREATE TABLE statement so the DB enforces token
length at schema level.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e5b733fa-8758-4215-b4ca-2652908bba43

📥 Commits

Reviewing files that changed from the base of the PR and between 3784ff9 and f259bc3.

⛔ Files ignored due to path filters (5)
  • backend/app/api/static/docs/docs.go is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.yaml is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.yaml is excluded by !backend/app/api/static/docs/**
📒 Files selected for processing (17)
  • backend/app/api/cli_reset_password.go
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/app/api/main.go
  • backend/app/api/middleware_ratelimit_test.go
  • backend/app/api/setup.go
  • backend/internal/core/services/reporting/import.go
  • backend/internal/core/services/reporting/io_sheet_test.go
  • backend/internal/core/services/service_user_password_reset_test.go
  • backend/internal/data/migrations/postgres/20260512000001_add_password_reset_tokens.sql
  • backend/internal/data/migrations/sqlite3/20260512000000_add_password_reset_tokens.sql
  • backend/internal/data/repo/repo_items_search_test.go
  • backend/internal/sys/validate/notifier_url_test.go
  • backend/pkgs/hasher/token_test.go
  • docs/public/api/openapi-3.0.json
  • docs/public/api/openapi-3.0.yaml
  • docs/public/api/swagger-2.0.json
  • docs/public/api/swagger-2.0.yaml
✅ Files skipped from review due to trivial changes (3)
  • backend/pkgs/hasher/token_test.go
  • backend/internal/data/repo/repo_items_search_test.go
  • backend/internal/sys/validate/notifier_url_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • backend/internal/data/migrations/sqlite3/20260512000000_add_password_reset_tokens.sql
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/app/api/cli_reset_password.go
  • docs/public/api/swagger-2.0.yaml
  • backend/internal/core/services/service_user_password_reset_test.go
  • docs/public/api/swagger-2.0.json
  • docs/public/api/openapi-3.0.json

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
docs/public/api/swagger-2.0.json (1)

6084-6095: ⚡ Quick win

Declare the email field as an email address.

Line 6090 models email as a plain string, so generated clients and docs lose basic validation on a security-sensitive endpoint.

📘 Suggested schema tweak
         "email": {
             "type": "string",
+            "format": "email",
             "example": "user@example.com"
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/public/api/swagger-2.0.json` around lines 6084 - 6095, Update the
OpenAPI schema for the v1.ForgotPasswordRequest model so the email property is
declared as an email address (e.g., keep "type": "string" and add "format":
"email" or an appropriate email "pattern") instead of a plain string; modify the
"email" property inside the v1.ForgotPasswordRequest schema to include the email
format/pattern to enable client and documentation validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/internal/data/repo/repo_password_reset.go`:
- Around line 104-106: The Update() atomic claim currently only checks
passwordresettokens.UsedAtIsNil(), allowing tokens to be claimed after their
expires_at; modify both Update() calls (the one that builds affected, err :=
r.db.PasswordResetTokens.Update() with Where(passwordresettokens.ID(id),
passwordresettokens.UsedAtIsNil()).SetUsedAt(at) and the similar update around
lines 142-145) to also include the expiry predicate, e.g. add
passwordresettokens.ExpiresAtGTE(at) (or ExpiresAtGT(at) if strict) to the
Where(...) so the database will only set UsedAt when the token is still within
its expiry window.

---

Nitpick comments:
In `@docs/public/api/swagger-2.0.json`:
- Around line 6084-6095: Update the OpenAPI schema for the
v1.ForgotPasswordRequest model so the email property is declared as an email
address (e.g., keep "type": "string" and add "format": "email" or an appropriate
email "pattern") instead of a plain string; modify the "email" property inside
the v1.ForgotPasswordRequest schema to include the email format/pattern to
enable client and documentation validations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b967a0a4-7bb5-4d7e-bf1e-4a15402ccdd0

📥 Commits

Reviewing files that changed from the base of the PR and between f259bc3 and 185cc84.

⛔ Files ignored due to path filters (5)
  • backend/app/api/static/docs/docs.go is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.yaml is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.yaml is excluded by !backend/app/api/static/docs/**
📒 Files selected for processing (9)
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/internal/core/services/reporting/io_sheet_test.go
  • backend/internal/core/services/service_user_password_reset.go
  • backend/internal/data/repo/repo_password_reset.go
  • docs/public/api/openapi-3.0.json
  • docs/public/api/openapi-3.0.yaml
  • docs/public/api/swagger-2.0.json
  • docs/public/api/swagger-2.0.yaml
  • frontend/lib/api/types/data-contracts.ts
✅ Files skipped from review due to trivial changes (2)
  • backend/internal/core/services/reporting/io_sheet_test.go
  • frontend/lib/api/types/data-contracts.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/internal/core/services/service_user_password_reset.go
  • docs/public/api/swagger-2.0.yaml
  • docs/public/api/openapi-3.0.json
  • docs/public/api/openapi-3.0.yaml

Comment thread backend/internal/data/repo/repo_password_reset.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
backend/internal/data/repo/repo_password_reset_test.go (1)

39-57: ⚡ Quick win

Add a token/user mismatch test to harden reset-flow authorization.

Nice expiry coverage. Please add a negative test where ConsumeAndChangePassword is called with a valid token ID but a different user ID, and assert it returns a claim/authorization error and leaves both used_at and password hash unchanged. This is a high-value security guardrail for account-takeover prevention.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/internal/data/repo/repo_password_reset_test.go` around lines 39 - 57,
Add a new negative test (e.g.,
TestPasswordResetTokens_ConsumeAndChangePassword_RejectsOnUserMismatch) that
creates a valid token with PasswordResetTokens.Create for tUser.ID, then calls
ConsumeAndChangePassword with that token.ID but a different user ID; assert the
call returns the same claim/authorization error (e.g.,
ErrPasswordResetTokenAlreadyClaimed or the repo's auth error), and verify both
the token's used_at value and the original user's Users.GetOneID().PasswordHash
remain unchanged after the call to ensure no side effects.
frontend/locales/en.json (1)

464-482: 💤 Low value

Translation keys look consistent with the new flow, and en.json is the only locale touched — that matches the Weblate workflow.

One tiny consistency nit: forgot_password_title and reset_password_title are both literally "Reset your password". The two pages serve different purposes (request a link vs. set a new password), so consider differentiating the forgot page title (e.g., "Forgot your password?") to give users a clearer wayfinding signal. Subtitles already do this well, so it's truly low-stakes.

Based on learnings: "Submit PRs by only adding or updating translation strings in frontend/locales/en.json. Do not modify other locale files… non-English translations are handled automatically through Weblate."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/locales/en.json` around lines 464 - 482, The two locale keys
forgot_password_title and reset_password_title both have the identical value
"Reset your password"; change forgot_password_title to a distinct,
context-appropriate string (e.g., "Forgot your password?") so the "forgot
password" request page and the "set new password" page have different titles;
update only the value for the forgot_password_title key in
frontend/locales/en.json to preserve Weblate workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/api/handlers/v1/helpers_test.go`:
- Around line 11-215: The test repeats the literals "https" and
"https://app.example.com", triggering goconst; update TestSecureBaseURL to reuse
the existing package constant schemeHTTPS for all protocol occurrences and add a
new test constant testBaseURLExample to replace the full URL literal, then
update all references in the test cases and assertions that currently use
"https" or "https://app.example.com" (and any other identical occurrences in
this file) so SecureBaseURL tests use schemeHTTPS and testBaseURLExample instead
of the duplicated string literals.

---

Nitpick comments:
In `@backend/internal/data/repo/repo_password_reset_test.go`:
- Around line 39-57: Add a new negative test (e.g.,
TestPasswordResetTokens_ConsumeAndChangePassword_RejectsOnUserMismatch) that
creates a valid token with PasswordResetTokens.Create for tUser.ID, then calls
ConsumeAndChangePassword with that token.ID but a different user ID; assert the
call returns the same claim/authorization error (e.g.,
ErrPasswordResetTokenAlreadyClaimed or the repo's auth error), and verify both
the token's used_at value and the original user's Users.GetOneID().PasswordHash
remain unchanged after the call to ensure no side effects.

In `@frontend/locales/en.json`:
- Around line 464-482: The two locale keys forgot_password_title and
reset_password_title both have the identical value "Reset your password"; change
forgot_password_title to a distinct, context-appropriate string (e.g., "Forgot
your password?") so the "forgot password" request page and the "set new
password" page have different titles; update only the value for the
forgot_password_title key in frontend/locales/en.json to preserve Weblate
workflow.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 321daed6-75a4-435a-9531-a9077dfe362b

📥 Commits

Reviewing files that changed from the base of the PR and between 185cc84 and bc3e7cc.

⛔ Files ignored due to path filters (5)
  • backend/app/api/static/docs/docs.go is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/openapi-3.yaml is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.json is excluded by !backend/app/api/static/docs/**
  • backend/app/api/static/docs/swagger.yaml is excluded by !backend/app/api/static/docs/**
📒 Files selected for processing (13)
  • backend/app/api/handlers/v1/helpers.go
  • backend/app/api/handlers/v1/helpers_test.go
  • backend/app/api/handlers/v1/v1_ctrl_auth.go
  • backend/internal/core/services/service_user_password_reset.go
  • backend/internal/core/services/service_user_password_reset_test.go
  • backend/internal/data/repo/repo_password_reset.go
  • backend/internal/data/repo/repo_password_reset_test.go
  • docs/public/api/openapi-3.0.json
  • docs/public/api/openapi-3.0.yaml
  • docs/public/api/swagger-2.0.json
  • docs/public/api/swagger-2.0.yaml
  • frontend/locales/en.json
  • frontend/pages/forgot-password.vue
🚧 Files skipped from review as they are similar to previous changes (7)
  • docs/public/api/openapi-3.0.yaml
  • backend/internal/data/repo/repo_password_reset.go
  • docs/public/api/openapi-3.0.json
  • docs/public/api/swagger-2.0.json
  • backend/internal/core/services/service_user_password_reset_test.go
  • docs/public/api/swagger-2.0.yaml
  • backend/internal/core/services/service_user_password_reset.go

Comment thread backend/app/api/handlers/v1/helpers_test.go
@tankerkiller125 tankerkiller125 merged commit 6163773 into main May 16, 2026
29 of 30 checks passed
@tankerkiller125 tankerkiller125 deleted the mk/pass-reset branch May 16, 2026 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⬆️ enhancement New feature or request go Pull requests that update Go code review needed A review is needed on this PR or Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant