fix(auth): guard against empty DashboardURL in invite + password-reset email construction (part 1 of #355) - #362
Merged
Conversation
…pty (#355) Two stacked URL construction sites in the auth service produced broken relative links when s.dashboardURL was empty: - internal/auth/service_user.go invite path setupURL = fmt.Sprintf("%s/reset-password?token=%s", "", token) yields "/reset-password?token=..." which is unclickable in any MUA. - internal/auth/service_password.go RequestPasswordReset has the same shape. Both now check s.dashboardURL early and refuse to send. The invite path sets InviteEmailError on the result so the admin sees a clear "fix DASHBOARD_URL" hint in the API response rather than a misleading "email sent: true" with a dead link. The forgot-password path skips the send silently to preserve the existing email-enumeration protection but logs an ERROR so the operator sees the failure in CloudWatch. NewService also logs a fatal-class WARN at startup when DashboardURL is empty — the misconfiguration is now visible at process startup instead of only when the first invite goes out. This is the URL-construction half of #355. The HTML pretty-button half (multipart bodies, CTA buttons modelled on the purchase-approval email at templates.go:367) lands in subsequent commits on this branch. Sister fix already merged ahead of this one: #357 (resetPassword handler base64-decode). Without that, the password the user sets via the reset flow does not work even when the URL is correct.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a startup warning for empty DashboardURL, guards password-reset and invite sends to skip email generation when the dashboard URL is unset, refactors invite email sending, and adds HTML email templates, renderers, multipart send logic, and tests. ChangesDashboardURL Configuration Safeguards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/auth/service_password.go`:
- Around line 275-284: In RequestPasswordReset, the s.dashboardURL empty check
runs after generating and persisting a reset token so it can rotate a token
without sending email; move the s.dashboardURL == "" guard to run before any
token creation or persistence logic (i.e., before the code that generates/saves
the reset token) so that no token is created/saved when DashboardURL is unset,
and keep the same non-error early return and logging behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d320826-83aa-436b-9fe9-37c490f070b9
📒 Files selected for processing (3)
internal/auth/service.gointernal/auth/service_password.gointernal/auth/service_user.go
Comment on lines
+275
to
+284
| // Skip the email entirely if dashboardURL is unconfigured — a broken | ||
| // relative link in an inbox is worse than no email; the operator's | ||
| // startup-time WARN already names the missing env var. Don't return an | ||
| // error to the caller to preserve the email-enumeration protection | ||
| // (RequestPasswordReset must look identical whether or not the email | ||
| // exists, and that includes whether or not a send happened). Issue #355. | ||
| if s.dashboardURL == "" { | ||
| logging.Errorf("RequestPasswordReset: skipping send — DashboardURL empty would produce a broken relative link (set DASHBOARD_URL).") | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Move the empty-DashboardURL guard before token persistence.
At Line 281, the function returns early only after generating and saving a new reset token. When DashboardURL is empty, this still rotates the token without sending any email, which can invalidate a previously delivered reset link and strand the user.
💡 Suggested fix
func (s *Service) RequestPasswordReset(ctx context.Context, email string) error {
user, err := s.store.GetUserByEmail(ctx, email)
if err != nil {
if errors.Is(err, pgx.ErrNoRows) {
// Don't reveal if email exists
logging.Debugf("Password reset requested for non-existent email: %s", redactEmail(email))
return nil
}
return err
}
if user == nil {
// Don't reveal if email exists
logging.Debugf("Password reset requested for non-existent email: %s", redactEmail(email))
return nil
}
+
+ // Skip before mutating reset-token state if DashboardURL is unconfigured.
+ if s.dashboardURL == "" {
+ logging.Errorf("RequestPasswordReset: skipping send — DashboardURL empty would produce a broken relative link (set DASHBOARD_URL).")
+ return nil
+ }
// Generate reset token
token, err := generateToken()
if err != nil {
return fmt.Errorf("failed to generate reset token: %w", err)
}
@@
- // Skip the email entirely if dashboardURL is unconfigured — a broken
- // relative link in an inbox is worse than no email; the operator's
- // startup-time WARN already names the missing env var. Don't return an
- // error to the caller to preserve the email-enumeration protection
- // (RequestPasswordReset must look identical whether or not the email
- // exists, and that includes whether or not a send happened). Issue `#355`.
- if s.dashboardURL == "" {
- logging.Errorf("RequestPasswordReset: skipping send — DashboardURL empty would produce a broken relative link (set DASHBOARD_URL).")
- return nil
- }
-
// Send reset email (use unhashed token in URL)
resetURL := fmt.Sprintf("%s/reset-password?token=%s", s.dashboardURL, token)🤖 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 `@internal/auth/service_password.go` around lines 275 - 284, In
RequestPasswordReset, the s.dashboardURL empty check runs after generating and
persisting a reset token so it can rotate a token without sending email; move
the s.dashboardURL == "" guard to run before any token creation or persistence
logic (i.e., before the code that generates/saves the reset token) so that no
token is created/saved when DashboardURL is unset, and keep the same non-error
early return and logging behavior.
…come (#355) Part 2 of #355. Replaces the raw-URL plain-text shape with a styled HTML email that carries a single CTA button, falling back to the existing plain-text body for MUAs that strip HTML. The visual idiom mirrors purchaseApprovalRequestHTMLTemplate (templates.go:367) — inline styles only, since most email clients (Outlook, mobile Gmail) ignore class-based CSS. New constants in internal/email/templates.go: - passwordResetHTMLTemplate (CTA "Reset your password", blue button) - welcomeUserHTMLTemplate (CTA "Open dashboard", green button) - userInviteHTMLTemplate (CTA "Set your password", green button) Each template includes a "if the button doesn't work, paste this URL" fallback that surfaces the absolute URL verbatim — keeps the link recoverable even in stripped-HTML clients without forcing users to deal with a button that doesn't render. New render functions (template_renderers.go): - RenderPasswordResetEmailHTML - RenderWelcomeEmailHTML - RenderUserInviteEmailHTML Shared multipart dispatch helper (templates.go): - sendMultipartVia(ctx, s, recipient, subject, kind, renderText, renderHTML) extracted from the existing sendPurchaseApprovalRequestVia pattern. HTML render failures degrade to text-only delivery — a template bug never drops the email. Both AWS-SES (Sender) and Azure-ACS/GCP-SendGrid (SMTPSender) paths route through sendMultipartVia. Subjects stay verbatim with the existing flavours so no Inbox-rule regression is introduced. Tests in template_renderers_test.go assert that each HTML output contains the CTA button text, the absolute URL in the href, the fallback "paste this URL" copy with the same absolute URL, and the salutation that pulls in the recipient's email. Closes #355 (combined with the URL-construction guards in the previous commit on this branch, ebbddea).
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 13, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two stacked URL construction sites in the auth service produced broken relative links when
s.dashboardURLwas empty, breaking the invite email AND the forgot-password email. This PR is part 1 of #355 — the URL-construction guards. Part 2 (HTML pretty-button templates) lands in follow-up commits on this same branch.Closes #355 once both parts land.
Part 1 (this commit) — URL construction guards
internal/auth/service_user.goinvite pathfmt.Sprintf("%s/reset-password?token=%s", "", token)→/reset-password?...unclickabledashboardURL == "".InviteEmailErrorset so admin sees a clear "fixDASHBOARD_URL" hint.internal/auth/service_password.goRequestPasswordResetdashboardURL == "". Silent to preserve email-enumeration protection, but ERROR-logged so the operator sees the failure in CloudWatch.internal/auth/service.goNewServiceExtracted
sendInviteEmailfromService.CreateUserso the parent stays under gocyclo's complexity threshold.Part 2 (follow-up commits on this branch)
userInviteTemplatewith a styled CTA button, modelled on the purchase-approval HTML template atinternal/email/templates.go:367.passwordResetTemplate.welcomeUserTemplate.sendMultipartViahelper extracted fromsendPurchaseApprovalRequestViaso the new flows don't drift from each other.Out of scope (separate follow-up issues)
purchaseConfirmationTemplate,purchaseFailedTemplate,riExchangePendingApprovalTemplate,riExchangeCompletedTemplate,registrationReceivedTemplate,registrationDecisionTemplate. All currently plain-text only with raw URLs; admin-facing so lower priority.DASHBOARD_URLon the live Lambda. The code change alone closes the loophole at the service layer, but the deployment also needs the env var pointed at the dashboard origin.Sister fix
PR #357 (closes #356, P0/critical) base64-decode fix on the
resetPasswordAPI handler. Without that, the password the user "sets" via the reset flow doesn't work even when the URL is correct. Must land ahead of this PR for the end-to-end invite flow to work.Test plan
go test github.com/LeanerCloud/CUDly/internal/auth— passesgo build ./...— cleanDASHBOARD_URL-config):https://<dashboard>/reset-password?token=...link.DASHBOARD_URLis unset,gh logsshows the startup WARN + per-send ERROR; the invite API response carriesinvite_email_sent=false+ the explanatoryinvite_email_error.Summary by CodeRabbit
New Features
Bug Fixes
Tests