Auth + Profile (Plan 2)#1
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implement TDD-driven user and session persistence methods on *Repo: - CreateUser, UserByLogin, UserByID, SaveUser for user management - CreateSession, SessionUser, DeleteSession, DeleteUserSessions for sessions - IsDuplicate helper for unique constraint violations All tests pass with GORM/SQLite integration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implement recovery-related persistence with TDD: backup code management (replace, consume, delete) and single-use password reset tokens with expiration support. Uses hashed storage and transaction safety. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds RequireAuth middleware, CurrentUser helper, API struct with cookie helpers, minimal mail.Mailer interface, and GET /api/me handler (Me). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implement account endpoints with TDD approach: - PATCH /api/me: Update username and email with validation - PUT /api/me/password: Change password with current password verification - Session rotation on password change to invalidate old sessions - Full test coverage for both endpoints Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Implement TwoFactorSetup, TwoFactorVerify, and TwoFactorDisable handlers to replace 501 stubs. Setup generates pending TOTP secrets, Verify confirms enrollment and returns backup codes, and Disable turns 2FA off with TOTP or backup code validation. All handlers follow TDD with comprehensive tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Promote go-mail (and otp) from indirect to direct in go.mod - Add Logger field + nil-safe logger() accessor to handlers.API - Wire logger into runServer's API literal - Replace swallowed Mailer.Send error with slog.Warn (keeps 204 for anti-enumeration) - Document token-burn trade-off above ConsumePasswordReset call - Extend TestPasswordResetFlow to assert pre-reset sessions are revoked Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add positive test case to LoginPage.test.tsx to verify forgot-password link displays when feature flag is enabled - Update api.ts request function to only include Content-Type header when request body is present - Add autoComplete="one-time-code" to TOTP input for better mobile keyboard support and password manager integration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Added Architecture section details for model, repository, auth, middleware, and mail packages. New Configuration section documents all BANDWIDTH_* env vars (port, logging, database, cookies, SMTP, BASE_URL) and explains session hashing (SHA-256), CSRF (Sec-Fetch-Site), and rate limiting. Updated Testing section to detail repository and handler testing patterns. Expanded .golangci.yml exclusion note to cover both version and mail package names. Updated README Stack section to describe completed auth implementation with TOTP 2FA and password reset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughImplements full authentication and profile: backend auth primitives, persistence, handlers (signup/login/logout, TOTP 2FA, password reset, account APIs), server wiring and tests, plus frontend API client, React Query hooks, pages/components, tests, and docs/config updates. ChangesFull Authentication & Profile System
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 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 `@cmd/bandwidth/server.go`:
- Around line 91-93: The IP extractor currently uses
echo.ExtractIPFromXFFHeader() with default trust rules which can let Echo fall
back to a spoofable leftmost XFF entry; change the extractor initialization
(e.IPExtractor) to call ExtractIPFromXFFHeader with explicit TrustOption(s) that
match Fly’s proxy/edge CIDRs (or otherwise exclude default private-network
trust) so Echo will treat the rightmost trusted hop as the client IP; update the
e.IPExtractor assignment in cmd/bandwidth/server.go (referencing
ExtractIPFromXFFHeader and TrustOption) to supply the correct trust
configuration for Fly’s network.
In `@frontend/src/components/profile/AccountSettings.tsx`:
- Around line 12-17: The effect in AccountSettings.tsx that syncs local state
from useMe() (useEffect calling setUsername/setEmail) is overwriting in-progress
edits when the query refetches; change the logic so it only initializes/syncs
when the local fields are empty or when the user is first loaded, or implement
an isDirty flag: add a local isDirty state, setIsDirty(true) in the
username/email onChange handlers, and update the useEffect to only call
setUsername/setEmail when user exists AND isDirty is false (or when
username/email are empty), leaving in-progress edits intact during background
refetches of useMe (staleTime behavior).
In `@frontend/src/components/profile/PasswordSettings.tsx`:
- Around line 38-52: The success flag that shows "Password changed" isn't
cleared when the user edits fields; update both onChange handlers (the ones
calling setCurrentPassword and setNewPassword) to also clear the changePassword
success state — e.g., call a reset/clear method on the mutation
(changePassword.reset() or changePassword.clear()) or call the local setter that
tracks success (e.g., setPasswordChanged(false)) so any prior success message is
removed immediately when either input changes; apply the same fix to the other
password input onChange (the block referenced at lines 62-65).
In `@frontend/src/components/profile/TwoFactorSettings.test.tsx`:
- Around line 53-56: Update the TwoFactorSettings tests to avoid QR async
flakiness by anchoring assertions to deterministic text lookup and mocking QR
generation: replace the waitFor/getAllByText checks for "SECRET123" and
"AAAA-BBBB" with await screen.findByText('SECRET123') and await
screen.findByText('AAAA-BBBB'), and add a test-local mock for qrcode's
QRCode.toDataURL to return a resolved promise (so EnrollTwoFactor's useEffect QR
generation never blocks); locate references to EnrollTwoFactor and
QRCode.toDataURL in the test file and ensure the mock is cleaned up after the
test run.
In `@frontend/src/components/RequireAuth.tsx`:
- Around line 16-18: The current RequireAuth component unconditionally redirects
on any useMe query error; change the logic to only redirect when the error
indicates unauthorized (HTTP 401/403 or an auth-specific error code) and for
other errors render a transient error/retry UI instead of Navigate. Locate the
useMe hook usage and the isError branch in RequireAuth, inspect the error object
(e.g., error.status / error.response?.status or error.code) and if status is 401
or 403 return <Navigate to="/login" replace />; otherwise render a retry/error
state that calls useMe.refetch (or queryClient.invalidateQueries for the useMe
query) and shows a message/button so network/5xx failures are recoverable rather
than forcing logout.
In `@frontend/src/pages/ForgotPasswordPage.tsx`:
- Around line 41-44: The component currently renders raw backend error text via
request.error.message in ForgotPasswordPage.tsx; create a centralized
error-mapping helper (e.g., mapAuthRecoveryError or sanitizeAuthError) that maps
API error objects to safe user-facing strings (like "Request failed, please try
again") and use that helper wherever auth recovery mutations are shown (replace
usages of request.error.message in ForgotPasswordPage.tsx and related recovery
pages); ensure the helper accepts the original error object for logging or
telemetry but only returns sanitized text for UI, and update the component to
call the helper before rendering the alert.
In `@internal/auth/auth_test.go`:
- Around line 8-60: The three standalone tests TestPasswordHashRoundTrip,
TestNewTokenIsRandomAndHashable, and TestNewBackupCodes should be refactored
into table-driven subtests using t.Run: for each original test, create a slice
of test cases (structs with name, inputs and expected results) and iterate them,
calling t.Run(tc.name, func(t *testing.T){ ... }) to assert behavior; reference
and reuse the existing helpers HashPassword and VerifyPassword for the password
cases, NewToken and HashToken for the token cases, and NewBackupCodes for the
backup code cases, ensuring each case checks the same conditions (format,
determinism, uniqueness, length, verify/accept/reject) as the original tests.
In `@internal/auth/token.go`:
- Around line 11-15: The function NewToken should not panic on crypto/rand
failure; change its signature from NewToken() string to NewToken() (string,
error), remove the panic in the rand.Read error path and return a wrapped error
instead, and update all callers (token issuance paths such as
signup/login/password reset/session issuance) to handle the error and return a
5xx response when NewToken fails; locate the function NewToken and replace the
panic branch, then propagate the error up through any functions that call
NewToken so they can translate it into an appropriate server error response.
In `@internal/handlers/account_test.go`:
- Around line 44-69: TestUpdateMe currently bypasses CSRF because
newAccountAPI/newTestAPI registers handlers on a bare echo instance; to
accurately test /api/me CSRF behavior, register the same csrf middleware
(csrfMW) or use the real api group when building the test server so /api/me is
mounted under apiGroup, or alternatively ensure the test requests include the
required CSRF header (e.g., add Sec-Fetch-Site header) when calling jsonReq;
update TestUpdateMe to construct the handler chain with csrfMW (or set the
Sec-Fetch-Site header on jsonReq calls) so CSRF is exercised for /api/me.
In `@internal/handlers/account.go`:
- Around line 16-19: The nil check for appmw.CurrentUser(c) returns a 500 but
that state should be impossible if RequireAuth middleware is applied; update the
three handlers in internal/handlers/account.go that call CurrentUser(c) to
either remove the redundant nil-checks (if you ensure RequireAuth is always
applied) or replace the return of echo.NewHTTPError with a hard assertion/panic
(e.g., panic or log.Panicf with a clear message) so a middleware
misconfiguration surfaces as a programmer error; search for usages of
CurrentUser(c) in the file and apply one consistent approach across all three
handlers.
- Around line 46-49: The current email normalization (email :=
strings.ToLower(strings.TrimSpace(*req.Email))) uses a weak check
(strings.Contains(email, "@")) that accepts invalid values; replace that check
with proper parsing (use net/mail.ParseAddress(email)) and return
echo.NewHTTPError(http.StatusBadRequest, "a valid email address is required")
when ParseAddress returns an error; keep using the normalized email variable
(email) for downstream use and validate before any DB write so malformed
addresses like "@", "user@" or "`@domain`" are rejected.
In `@internal/handlers/passwordreset.go`:
- Around line 28-32: The password reset link currently includes the token in the
query string (see Repo.CreatePasswordReset usage and Mailer.Send building link
using a.BaseURL and "reset-password?token="), which can leak via Referer, logs,
or history; change the flow so the token is not exposed in the URL: have
CreatePasswordReset still generate a short-lived token but send an email linking
to a landing endpoint (e.g., /reset-password) without the token, or to a landing
endpoint that immediately exchanges a one-time token sent in the URL for a
session via a backend call and then redirects to a clean URL (remove token from
browser history) and set a secure, HttpOnly cookie; update the Mailer.Send
invocation to use the new link format and adjust the reset page/handler to
accept the token via POST or perform the server-side exchange (modify the reset
handler that currently reads token from query to perform the exchange and clear
the token from the URL).
In `@internal/handlers/twofa.go`:
- Around line 73-101: TwoFactorDisable currently disables 2FA after only
validating a TOTP or backup code; require a step-up re-authentication by
verifying the user's current password before clearing TOTPSecret/TOTPConfirmedAt
and deleting backup codes. Update TwoFactorDisable to bind and check a
CurrentPassword field on twoFactorCodeRequest (or a new request struct), call
auth.VerifyPassword(user, req.CurrentPassword) (similar to the password-change
flow in account.go) and return HTTP 401/400 on failure, then proceed to consume
the TOTP/backup code and save/delete via a.Repo.SaveUser and
a.Repo.DeleteBackupCodes only after password verification.
In `@internal/mail/mail.go`:
- Around line 61-64: The mail setup currently uses
gomail.WithTLSPortPolicy(gomail.TLSOpportunistic) which allows STARTTLS fallback
to plaintext; change this to gomail.WithTLSPortPolicy(gomail.TLSMandatory) or
make the policy configurable via the mail config (e.g., m.cfg.TLSPolicy) and use
that value when building the opts slice (reference opts,
gomail.WithTLSPortPolicy, TLSOpportunistic/TLSMandatory) so production relays
require encrypted connections while preserving an option for legacy servers if
needed.
- Around line 72-76: The SMTP client is created without a timeout causing
potential indefinite hangs; update the mail send path by adding a connection
timeout option to gomail.NewClient (e.g., include
gomail.WithTimeout(time.Duration) and add the time import) or switch to
client.DialAndSendWithContext and pass a context with timeout per request;
modify the code around gomail.NewClient and the client.DialAndSend(msg) call to
use either gomail.WithTimeout(...) or DialAndSendWithContext(ctx) with
context.WithTimeout so the operation fails after the configured deadline.
In `@internal/repository/recovery_test.go`:
- Around line 12-74: The tests currently ignore errors from repo.CreateUser,
repo.CreatePasswordReset, and the manual expiry update; change them to check
returned errors and fail the test immediately (use t.Fatalf or t.Fatal) if any
are non-nil. Specifically, after calling repo.CreateUser(...) and
repo.CreatePasswordReset(...) verify the error value and stop the test on error,
and capture/check the error returned by
repo.db.Model(&model.PasswordReset{}).Where("user_id = ?", user.ID).Update(...)
and fail the test if it returns an error so you never proceed with an
invalid/empty token.
In `@internal/repository/repository_test.go`:
- Around line 9-16: The test helper testRepo opens a Repo via Open but never
closes it; modify testRepo to register t.Cleanup to close the repo when the test
finishes by calling repo.Close() inside t.Cleanup (and handle any Close error
with t.Fatalf or t.Error), i.e., after obtaining repo and err, call
t.Cleanup(func(){ if cerr := repo.Close(); cerr != nil { t.Fatalf("Close: %v",
cerr) } }) so the Repo is always cleaned up; reference the testRepo function and
the Open/Repo.Close methods when making the change.
In `@internal/repository/repository.go`:
- Around line 33-46: When AutoMigrate on the opened gorm DB fails, the opened
connection is leaked; after detecting an error from db.AutoMigrate (the db
returned by gorm.Open), call db.DB() to obtain the underlying *sql.DB and
Close() it before returning the migration error. In other words, on error from
AutoMigrate, call sqlDB, _ := db.DB(); sqlDB.Close() (handle any returned error
as needed) and then return fmt.Errorf("migrate database: %w", err) so the SQL
handle is closed on initialization failure.
In `@internal/repository/users.go`:
- Around line 9-12: The IsDuplicate(err error) helper relies on fragile
substring matching of the SQLite/GORM error message; add a short code comment
above the IsDuplicate function documenting this limitation and the SQLite/GORM
version dependency (or link to an issue) so future maintainers know why string
matching is used and to revisit if SQLite error formats change; mention the
exact sentinel phrase "UNIQUE constraint failed" and that structured constraint
codes are unavailable with GORM's SQLite driver.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a6cf303-03fa-4d5f-8a0a-17639461effd
⛔ Files ignored due to path filters (2)
frontend/bun.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
.golangci.ymlAGENTS.mdREADME.mdcmd/bandwidth/main.gocmd/bandwidth/server.gocmd/bandwidth/server_test.godocs/superpowers/plans/2026-06-10-auth-and-profile.mdfrontend/package.jsonfrontend/src/App.tsxfrontend/src/components/Layout.tsxfrontend/src/components/RequireAuth.test.tsxfrontend/src/components/RequireAuth.tsxfrontend/src/components/profile/AccountSettings.test.tsxfrontend/src/components/profile/AccountSettings.tsxfrontend/src/components/profile/DisableTwoFactor.tsxfrontend/src/components/profile/EnrollTwoFactor.tsxfrontend/src/components/profile/PasswordSettings.tsxfrontend/src/components/profile/TwoFactorSettings.test.tsxfrontend/src/components/profile/TwoFactorSettings.tsxfrontend/src/hooks/auth.tsfrontend/src/lib/api.test.tsfrontend/src/lib/api.tsfrontend/src/lib/types.tsfrontend/src/main.tsxfrontend/src/pages/ForgotPasswordPage.tsxfrontend/src/pages/HomePage.test.tsxfrontend/src/pages/HomePage.tsxfrontend/src/pages/LoginPage.test.tsxfrontend/src/pages/LoginPage.tsxfrontend/src/pages/ProfilePage.tsxfrontend/src/pages/ResetPasswordPage.tsxfrontend/src/pages/SignupPage.tsxfrontend/src/test/setup.tsfrontend/src/test/utils.tsxgo.modinternal/auth/auth_test.gointernal/auth/backupcodes.gointernal/auth/password.gointernal/auth/session.gointernal/auth/token.gointernal/auth/totp.gointernal/auth/totp_test.gointernal/handlers/account.gointernal/handlers/account_test.gointernal/handlers/api.gointernal/handlers/auth.gointernal/handlers/auth_test.gointernal/handlers/passwordreset.gointernal/handlers/passwordreset_test.gointernal/handlers/twofa.gointernal/handlers/twofa_test.gointernal/mail/mail.gointernal/mail/mail_test.gointernal/middleware/auth.gointernal/middleware/auth_test.gointernal/model/model.gointernal/repository/backupcodes.gointernal/repository/passwordresets.gointernal/repository/recovery_test.gointernal/repository/repository.gointernal/repository/repository_test.gointernal/repository/sessions.gointernal/repository/sessions_test.gointernal/repository/users.gointernal/repository/users_test.go
| // fly.io's edge proxy appends the real client IP to X-Forwarded-For; | ||
| // the default legacy extractor trusts the leftmost (spoofable) entry. | ||
| e.IPExtractor = echo.ExtractIPFromXFFHeader() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Echo v5 IP extraction behavior
# Check the source implementation
rg -A 10 'func ExtractIPFromXFFHeader' --type goRepository: jwhumphries/BandWidth
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Echo version from go.mod (and any go.sum hints)
ls -la
rg -n --hidden -S 'github.com/labstack/echo/v5' go.mod go.sum || true
# Find where ExtractIPFromXFFHeader is referenced in the repo
rg -n --hidden -S 'ExtractIPFromXFFHeader' . || true
# Attempt to locate any vendored Echo source (unlikely, but check)
fd -a 'echo' . -t f 2>/dev/null | head -n 50 || trueRepository: jwhumphries/BandWidth
Length of output: 1922
🌐 Web query:
Echo v5.1.1 ExtractIPFromXFFHeader implementation rightmost X-Forwarded-For leftmost
💡 Result:
In Echo v5.1.1 (and recent v5 versions), the ExtractIPFromXFFHeader implementation does not blindly take either the leftmost or rightmost IP address [1][2]. Instead, it uses a trust-based strategy that processes the X-Forwarded-For (XFF) header chain from right to left [1][2]. The implementation follows this logic: 1. It combines the XFF header values with the direct request IP (the immediate source) [1][2]. 2. It iterates through this combined list of IP addresses starting from the rightmost (closest to the server) and moving toward the left [1][2]. 3. It checks each IP against configured trust options (e.g., loopback, private networks, or specific CIDR ranges) [3][1][2]. 4. It returns the first (rightmost) IP address that it considers untrustable [1][2]. 5. If all IP addresses in the list are deemed trustable, it falls back to the leftmost IP address (the first element in the XFF header), as this is considered the furthest point from the server [1][2]. This mechanism is designed to prevent IP spoofing by ensuring that untrusted proxies or client-provided headers are not used as the definitive client IP when internal, trusted infrastructure is present [3][1][2]. Note that as of v5.1.0, the context.RealIP method no longer uses potentially spoofable headers by default and instead relies on the configured e.IPExtractor [4][5]. Users requiring the older, simpler behavior (which just takes the leftmost IP) should explicitly use the LegacyIPExtractor [4][5][6].
Citations:
- 1: https://github.com/labstack/echo/blob/v5.0.4/ip.go
- 2: https://github.com/labstack/echo/blob/master/ip.go
- 3: https://echo.labstack.com/docs/ip-address
- 4: https://github.com/labstack/echo/releases/tag/v5.1.0
- 5: Remove legacy IP extraction logic from context.RealIP method labstack/echo#2933
- 6: labstack/echo@c192e50
🌐 Web query:
Echo v5.1.1 ip.go ExtractIPFromXFFHeader implementation scans right to left returns first untrusted
💡 Result:
The Echo web framework's ExtractIPFromXFFHeader function, including in versions relevant to v5.1.1, indeed implements a right-to-left scanning strategy to identify the client IP address [1][2]. This implementation is designed to securely extract the client's IP address when the application is deployed behind trusted proxies [3]. The logic operates as follows: 1. Right-to-Left Scan: The function processes the combined list of IP addresses from the X-Forwarded-For header and the request's direct IP address (from RemoteAddr) starting from the rightmost element [1][2]. 2. Trust Validation: As it moves from right to left, it checks each IP address against a configurable set of "trusted" IP ranges or networks [1][3][2]. 3. Returns First Untrusted IP: The function returns the first IP address that it determines is not trusted [1][2]. This IP is considered the genuine client IP because it represents the first entity outside of the known, trusted infrastructure [3]. 4. Fallback: If all IP addresses in the header and the request's RemoteAddr are found to be trusted (i.e., the entire chain belongs to the known proxy infrastructure), the function returns the leftmost IP address as a best-effort, final fallback [1][2]. This approach is considered best practice for security, as it prevents malicious clients from spoofing their IP address by prepending arbitrary IP values to the X-Forwarded-For header [3][4]. Users are expected to correctly configure the trusted IP ranges (e.g., using TrustIPRanges) to match their proxy environment [1][3].
Citations:
- 1: https://github.com/labstack/echo/blob/v5.0.4/ip.go
- 2: https://github.com/labstack/echo/blob/master/ip.go
- 3: https://echo.labstack.com/docs/ip-address
- 4: https://adam-p.ca/blog/2022/03/x-forwarded-for/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to locate Echo source in the module cache (no build).
GOMODCACHE="$(go env GOMODCACHE 2>/dev/null || true)"
echo "GOMODCACHE=$GOMODCACHE"
if [ -n "${GOMODCACHE}" ] && [ -d "${GOMODCACHE}" ]; then
fd -a 'echo@v5.1.1|echo@v5.1.1$|labstack/echo' "${GOMODCACHE}" -t f -d 4 2>/dev/null | head -n 50
rg -n 'func ExtractIPFromXFFHeader' "${GOMODCACHE}" -S --hidden --no-ignore-vcs 2>/dev/null | head -n 20 || true
fiRepository: jwhumphries/BandWidth
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOMODCACHE="$(go env GOMODCACHE)"
echo "GOMODCACHE=$GOMODCACHE"
ls -la "$GOMODCACHE/github.com/labstack" 2>/dev/null || true
# Locate echo v5.1.1 source directory (if present in the cache)
echo_dir="$(find "$GOMODCACHE" -type d -path "*/github.com/labstack/echo@v5.1.1" | head -n 1)"
echo "echo_dir=$echo_dir"
if [ -n "${echo_dir}" ]; then
sed -n '1,200p' "$echo_dir/ip.go" | sed -n '1,200p' || true
rg -n 'ExtractIPFromXFFHeader|TrustIP|X-Forwarded-For|XFF' "$echo_dir/ip.go" || true
# Print the function body region
rg -n -C 2 'func ExtractIPFromXFFHeader' "$echo_dir/ip.go" || true
# Best-effort range extraction (avoid whole file output)
start="$(rg -n 'func ExtractIPFromXFFHeader' "$echo_dir/ip.go" | head -n 1 | cut -d: -f1)"
if [ -n "${start}" ]; then
end=$((start+120))
sed -n "${start},${end}p" "$echo_dir/ip.go"
fi
else
# Fallback: locate any echo@v5.1.1 occurrence and show ip.go
find "$GOMODCACHE" -type f -path "*/github.com/labstack/echo@v5.1.1/*/ip.go" -o -path "*/github.com/labstack/echo@v5.1.1/ip.go" 2>/dev/null | head -n 20
fiRepository: jwhumphries/BandWidth
Length of output: 112
🌐 Web query:
https://github.com/labstack/echo/blob/v5.1.1/ip.go ExtractIPFromXFFHeader function body
💡 Result:
ExtractIPFromXFFHeader function body (Echo v5.1.1): func ExtractIPFromXFFHeader(options...TrustOption) IPExtractor { checker:= newIPChecker(options) return func(req *http.Request) string { directIP:= extractIP(req) xffs:= req.Header[HeaderXForwardedFor] if len(xffs) == 0 { return directIP } ips:= append(strings.Split(strings.Join(xffs, ","), ","), directIP) for i:= len(ips) - 1; i >= 0; i-- { ips[i] = strings.TrimSpace(ips[i]) ips[i] = strings.TrimPrefix(ips[i], "[") ips[i] = strings.TrimSuffix(ips[i], "]") ip:= net.ParseIP(ips[i]) if ip == nil { // Unable to parse IP; cannot trust entire records return directIP } if!checker.trust(ip) { return ip.String } } // All of the IPs are trusted; return first element because it is furthest from server (best effort strategy). return strings.TrimSpace(ips) } } Notes on behavior implemented by this function: - If the X-Forwarded-For header is absent, it returns the direct IP from extractIP(req) [1]. - It splits XFF values on commas, trims whitespace and optional surrounding brackets (IPv6 form), then parses each candidate IP with net.ParseIP [1]. - It walks the list from right-to-left (nearest-hop to farthest) and returns the first IP that is not trusted by the checker; if all are trusted, it returns the left-most value (furthest from server / best-effort) [1]. - If any candidate IP fails parsing, it returns the direct IP rather than trusting any record [1].
Citations:
🌐 Web query:
ExtractIPFromXFFHeader(v5.1.1) right-to-left untrusted leftmost fallback TrustIPRanges ip.go lines
💡 Result:
The ExtractIPFromXFFHeader function in the Labstack Echo framework (such as v5.1.1) is designed to securely extract the client's IP address from the X-Forwarded-For (XFF) header by avoiding reliance on user-controllable input [1][2]. The implementation follows a right-to-left strategy to identify the client IP [1][3]: 1. Mechanism: It iterates through the XFF header values and the direct connection IP (from http.Request.RemoteAddr) in reverse order, starting from the rightmost address (the one closest to the server) [1][4]. 2. Trust Logic: It uses a configured IP checker (defined via TrustOption, such as TrustIPRanges) to determine if an IP address belongs to your trusted infrastructure [1][3]. 3. Selection: The function returns the first IP address encountered while moving right-to-left that is considered untrusted [1][3]. This represents the client IP because it is the first address not originating from your own proxy chain [1][5]. 4. Fallback: If all IP addresses in the list (including the direct connection IP) are found to be trusted, the function falls back to returning the leftmost IP address (the first element in the XFF list), as this is considered the best-effort strategy to find the furthest point of origin [1][2][4]. This approach is intended to prevent IP spoofing, as an attacker could easily manipulate the leftmost part of the XFF header, but they cannot control the IPs appended by your trusted proxies on the right side of the header [1][6][3].
Citations:
- 1: https://github.com/labstack/echo/blob/master/ip.go
- 2: https://github.com/labstack/echo/blob/v5.0.4/ip.go
- 3: https://echo.labstack.com/docs/ip-address
- 4: https://github.com/clems71/echo/blob/master/ip.go
- 5: https://www.alibabacloud.com/help/en/asm/sidecar/enable-asm-gateway-to-obtain-client-ip-by-configuring-xff-request-header
- 6: https://www.bookstack.cn/read/echo-4.11-en/7a18c94ebf00d96b.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around cmd/bandwidth/server.go:91-93
sed -n '70,120p' cmd/bandwidth/server.goRepository: jwhumphries/BandWidth
Length of output: 1731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to populate module cache for Echo v5.1.1 so we can inspect ip.go directly.
go mod download github.com/labstack/echo/v5@v5.1.1
GOMODCACHE="$(go env GOMODCACHE)"
echo_dir="$(find "$GOMODCACHE" -type d -path "*/github.com/labstack/echo@v5.1.1*" | head -n 1)"
echo "echo_dir=$echo_dir"
if [ -n "${echo_dir}" ]; then
rg -n 'type ipChecker|func newIPChecker|trust\(' "$echo_dir/ip.go" || true
rg -n -C 3 'func ExtractIPFromXFFHeader' "$echo_dir/ip.go" || true
# Print the whole function (should be small)
start="$(rg -n 'func ExtractIPFromXFFHeader' "$echo_dir/ip.go" | head -n 1 | cut -d: -f1)"
if [ -n "${start}" ]; then
end=$((start+200))
sed -n "${start},${end}p" "$echo_dir/ip.go"
fi
fiRepository: jwhumphries/BandWidth
Length of output: 75
🌐 Web query:
Echo v5.1.1 newIPChecker default TrustIPRanges when no TrustOption passed ExtractIPFromXFFHeader
💡 Result:
In Echo v5.1.1, when using ExtractIPFromXFFHeader without providing explicit TrustOptions, the underlying newIPChecker initializes with a default set of trusted internal IP address ranges [1][2][3]. Specifically, the default behavior of the ipChecker (which is used by ExtractIPFromXFFHeader) is to trust the following [2][3]: 1. Loopback addresses (e.g., 127.0.0.1,::1) [2][3] 2. Link-local unicast addresses (e.g., 169.254.0.0/16) [2][3] 3. Private-use and unique local addresses (as defined by RFC6890, RFC4291, and RFC4193) [1][2][3] The newIPChecker is implemented to enable these by default in its constructor: checker:= &ipChecker{trustLoopback: true, trustLinkLocal: true, trustPrivateNet: true} [2][3] When ExtractIPFromXFFHeader is called without arguments, it uses these defaults to determine which IP addresses in the X-Forwarded-For header are considered trustable [2][3]. The function traverses the IP list (from the XFF header and the direct connection) from right to left; it returns the first IP address it encounters that is not considered trustable [2][3][4]. If all IPs in the list are trustable, it falls back to returning the first element (the furthest one) [2][3][4]. Users are advised to explicitly configure their trust options (e.g., TrustLoopback(false), TrustPrivateNet(false), or TrustIPRange(...)) to avoid trusting internal network IPs that might be spoofed if the application is not properly protected by a trusted proxy [1][5].
Citations:
- 1: https://echo.labstack.com/docs/ip-address
- 2: https://github.com/labstack/echo/blob/v5.0.4/ip.go
- 3: https://github.com/labstack/echo/blob/master/ip.go
- 4: https://sources.debian.org/src/golang-github-labstack-echo/4.2.1-3/ip.go
- 5: Remove legacy IP extraction logic from context.RealIP method labstack/echo#2933
🌐 Web query:
site:github.com/labstack/echo v5.1.1 type ipChecker newIPChecker trust default options
💡 Result:
In Echo framework version 5.1.1, the ipChecker is an internal struct used by IP extractors to validate whether an IP address source is trusted [1][2]. The newIPChecker function initializes this checker with specific default trust configurations [1][2]. When newIPChecker is called, it defaults to trusting loopback addresses, link-local addresses, and private network addresses [1][2]: checker:= &ipChecker{ trustLoopback: true, trustLinkLocal: true, trustPrivateNet: true, } These default options can be overridden using TrustOption functions, which are passed as arguments to newIPChecker [1][2]. Available TrustOptions include: - TrustLoopback(bool): Configures trust for loopback addresses (default: true) [1][2]. - TrustLinkLocal(bool): Configures trust for link-local addresses (default: true) [1][2]. - TrustPrivateNet(bool): Configures trust for private network addresses (default: true) [1][2]. - TrustIPRange(*net.IPNet): Adds a specific CIDR range to the list of trusted IP ranges [1][2]. In practice, these options are used when configuring IP extractors like ExtractIPFromRealIPHeader or ExtractIPFromXFFHeader [3][4]. Because Echo v5.x no longer uses potentially spoofable headers by default, you must configure the IPExtractor explicitly with appropriate trust options to safely parse client IP addresses from headers when behind a proxy [4].
Citations:
- 1: https://github.com/labstack/echo/blob/master/ip.go
- 2: https://github.com/labstack/echo/blob/v5.0.4/ip.go
- 3: labstack/echo@v5.0.4...v5.1.0
- 4: https://github.com/labstack/echo/releases/tag/v5.1.0
Fix IP extraction trust so Echo won’t fall back to spoofable XFF leftmost values
Echo v5.1.1 ExtractIPFromXFFHeader does not simply “take the rightmost IP”: it walks the XFF chain right-to-left (plus RemoteAddr) and returns the first IP that is not trusted; if all IPs are trusted, it falls back to the leftmost (furthest) XFF value. With no TrustOption provided in cmd/bandwidth/server.go, Echo’s default trust includes loopback, link-local, and private networks, meaning the leftmost XFF value could be returned if the proxy + appended client IPs are all considered trusted.
Update the extractor to use explicit TrustOption(s) matching Fly’s proxy/edge network (and/or disable the default private-net trust) so the real client IP is taken from the trusted proxy’s rightmost hop, not from user-controlled header entries.
🤖 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 `@cmd/bandwidth/server.go` around lines 91 - 93, The IP extractor currently
uses echo.ExtractIPFromXFFHeader() with default trust rules which can let Echo
fall back to a spoofable leftmost XFF entry; change the extractor initialization
(e.IPExtractor) to call ExtractIPFromXFFHeader with explicit TrustOption(s) that
match Fly’s proxy/edge CIDRs (or otherwise exclude default private-network
trust) so Echo will treat the rightmost trusted hop as the client IP; update the
e.IPExtractor assignment in cmd/bandwidth/server.go (referencing
ExtractIPFromXFFHeader and TrustOption) to supply the correct trust
configuration for Fly’s network.
| // Manual-entry secret appears (QR may render async). | ||
| await waitFor(() => | ||
| expect(screen.getAllByText(/SECRET123/).length).toBeGreaterThan(0), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where the enrollment flow can stall and whether QR generation is in the critical path.
set -euo pipefail
echo "== Locate 2FA enrollment component implementation =="
fd -i "EnrollTwoFactor.tsx|TwoFactorSettings.tsx" frontend/src/components/profile
echo
echo "== Inspect QR/TOTP-related imports and awaited calls =="
rg -n -C3 "qrcode|toDataURL|otpauth|setup|verify|waitFor|findBy" frontend/src/components/profile frontend/src/components/profile/TwoFactorSettings.test.tsx
echo
echo "== Inspect API endpoints used by the enrollment UI =="
rg -n -C2 "/2fa/setup|/2fa/verify|api\\.(post|get|put|patch)" frontend/src/components/profileRepository: jwhumphries/BandWidth
Length of output: 19360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show TwoFactorSettings component =="
sed -n '1,220p' frontend/src/components/profile/TwoFactorSettings.tsx
echo
echo "== Show EnrollTwoFactor component =="
sed -n '1,200p' frontend/src/components/profile/EnrollTwoFactor.tsx
echo
echo "== Locate and show auth hooks used by EnrollTwoFactor =="
# find hooks files
fd -i "useTwoFactorSetup|useTwoFactorVerify" frontend/src/hooks frontend/src -a 2>/dev/null || true
# show implementation if found
for f in $(fd -i "useTwoFactorSetup|useTwoFactorVerify" frontend/src/hooks -a 2>/dev/null | head -n 20); do
echo "--- $f ---"
sed -n '1,240p' "$f"
done
echo
echo "== Show TwoFactorSettings.test.tsx fully (if small) =="
wc -l frontend/src/components/profile/TwoFactorSettings.test.tsx
sed -n '1,220p' frontend/src/components/profile/TwoFactorSettings.test.tsxRepository: jwhumphries/BandWidth
Length of output: 6607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show TwoFactorSettings component =="
sed -n '1,220p' frontend/src/components/profile/TwoFactorSettings.tsx
echo
echo "== Show EnrollTwoFactor component =="
sed -n '1,200p' frontend/src/components/profile/EnrollTwoFactor.tsx
echo
echo "== Locate and show auth hooks used by EnrollTwoFactor =="
# find hooks files
fd -i "useTwoFactorSetup|useTwoFactorVerify" frontend/src/hooks frontend/src -a 2>/dev/null || true
# show implementation if found
for f in $(fd -i "useTwoFactorSetup|useTwoFactorVerify" frontend/src/hooks -a 2>/dev/null | head -n 20); do
echo "--- $f ---"
sed -n '1,240p' "$f"
done
echo
echo "== Show TwoFactorSettings.test.tsx fully (if small) =="
wc -l frontend/src/components/profile/TwoFactorSettings.test.tsx
sed -n '1,220p' frontend/src/components/profile/TwoFactorSettings.test.tsxRepository: jwhumphries/BandWidth
Length of output: 6607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate hooks/auth index for useTwoFactorSetup/useTwoFactorVerify =="
fd -i "useTwoFactorSetup|useTwoFactorVerify" frontend/src/hooks frontend/src -a | head -n 50
echo
echo "== Grep for definitions =="
rg -n "function useTwoFactorSetup|const useTwoFactorSetup|export function useTwoFactorSetup|useTwoFactorSetup\s*=" frontend/src/frontend/src frontend/src 2>/dev/null || true
rg -n "function useTwoFactorVerify|const useTwoFactorVerify|export function useTwoFactorVerify|useTwoFactorVerify\s*=" frontend/src 2>/dev/null || true
echo
echo "== Show auth hooks file(s) where they are defined =="
for f in $(fd -i "*auth*" frontend/src/hooks frontend/src | head -n 50); do
rg -n "useTwoFactorSetup|useTwoFactorVerify" "$f" >/dev/null 2>&1 && echo "HIT: $f" && sed -n '1,260p' "$f"
done
echo
echo "== Also inspect the auth hooks exports (frontend/src/hooks/auth) =="
ls -la frontend/src/hooks || true
ls -la frontend/src/hooks/auth || true
fd -i "auth" frontend/src/hooks -a | head -n 50
# try common entry points
for p in frontend/src/hooks/auth.ts frontend/src/hooks/auth/index.ts frontend/src/hooks/auth.tsx frontend/src/hooks/auth.js frontend/src/hooks/auth/index.js; do
if [ -f "$p" ]; then
echo "---- $p ----"
sed -n '1,260p' "$p"
fi
doneRepository: jwhumphries/BandWidth
Length of output: 4500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show frontend/src/lib/api.ts (or matching) =="
fd -i "api\\.ts|api\\.tsx" frontend/src/lib frontend/src -a | head -n 20
for f in $(fd -i "api\\.ts|api\\.tsx" frontend/src/lib frontend/src -a | head -n 20); do
echo "--- $f ---"
sed -n '1,240p' "$f"
done
echo
echo "== Search for base URL / request path building =="
rg -n "baseURL|BASE_URL|/api/auth/2fa/setup|/2fa/setup|fetch\\(" frontend/src/lib frontend/src/hooks frontend/src/components/profile/TwoFactorSettings.test.tsxRepository: jwhumphries/BandWidth
Length of output: 3336
Stabilize TwoFactorSettings enrollment test by removing QR-code async flakiness
The manual secret (setup.data.secret / SECRET123) is rendered as soon as /api/auth/2fa/setup resolves, independent of the QR. The nondeterministic part is EnrollTwoFactor’s useEffect calling QRCode.toDataURL(setup.data.otpauthUrl)—this QR generation can slow/hang in jsdom/CI and cause the test to hit its 5s timeout even if the secret appears quickly.
// Manual-entry secret appears (QR may render async).
await waitFor(() =>
expect(screen.getAllByText(/SECRET123/).length).toBeGreaterThan(0),
);Stabilize by:
- Anchor on deterministic UI signals via
await screen.findByText('SECRET123')(and similarly forAAAA-BBBB), instead of polling withwaitFor/getAllByText. - Mock
qrcode’sQRCode.toDataURLin this test to resolve immediately (so QR generation never impacts runtime).
Also applies to:
frontend/src/components/profile/TwoFactorSettings.test.tsxlines 61-63 (AAAA-BBBBwait).
🤖 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/src/components/profile/TwoFactorSettings.test.tsx` around lines 53 -
56, Update the TwoFactorSettings tests to avoid QR async flakiness by anchoring
assertions to deterministic text lookup and mocking QR generation: replace the
waitFor/getAllByText checks for "SECRET123" and "AAAA-BBBB" with await
screen.findByText('SECRET123') and await screen.findByText('AAAA-BBBB'), and add
a test-local mock for qrcode's QRCode.toDataURL to return a resolved promise (so
EnrollTwoFactor's useEffect QR generation never blocks); locate references to
EnrollTwoFactor and QRCode.toDataURL in the test file and ensure the mock is
cleaned up after the test run.
Source: Pipeline failures
| client, err := gomail.NewClient(m.cfg.Host, opts...) | ||
| if err != nil { | ||
| return fmt.Errorf("smtp client: %w", err) | ||
| } | ||
| return client.DialAndSend(msg) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check go-mail timeout configuration options
rg -nP --type=go 'func With.*Timeout'Repository: jwhumphries/BandWidth
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where gomail is used/imported and what version is pinned
rg -n --type=go "gomail\.|wneessen/go-mail|NewClient\(" .
# Show relevant module info
if [ -f go.mod ]; then
rg -n "wneessen/go-mail" go.mod || true
fi
if [ -f go.sum ]; then
rg -n "wneessen/go-mail" go.sum | head -n 50 || true
fiRepository: jwhumphries/BandWidth
Length of output: 943
🌐 Web query:
wneessen go-mail gomail WithTimeout option name DialAndSend timeout
💡 Result:
In the wneessen/go-mail library, the WithTimeout option is used to set the connection timeout for a Client [1][2]. When you use the DialAndSend method to send messages, it internally calls DialAndSendWithContext, using context.Background by default [1][2]. Because the library is designed around Go context handling, if you require specific timeout behavior during the send process, you should use the DialAndSendWithContext method directly and provide a context with a timeout or deadline [1][3][4]. The WithTimeout option specifically configures the connection timeout duration for the Client and is applied during client initialization [1][2]. It is not a parameter passed to DialAndSend itself [1][5]. Example usage with a timeout context: // Create the client client, err:= mail.NewClient("smtp.example.com", mail.WithTimeout(5 * time.Second)) if err!= nil { log.Fatal(err) } // Use a context with a timeout for the operation ctx, cancel:= context.WithTimeout(context.Background, 10 * time.Second) defer cancel // Use DialAndSendWithContext instead of DialAndSend if err:= client.DialAndSendWithContext(ctx, message); err!= nil { log.Fatal(err) } Note: This library (wneessen/go-mail) is a separate, modern project and should not be confused with the legacy gopkg.in/go-mail/gomail.v2 or go-mail/mail libraries, which utilize different APIs for timeout management [3][4][6][7].
Citations:
- 1: https://github.com/wneessen/go-mail/blob/main/client.go
- 2: https://github.com/wneessen/go-mail/blob/v0.7.2/client.go
- 3: https://pkg.go.dev/github.com/wneessen/go-mail
- 4: https://github.com/wneessen/go-mail/blob/main/README.md
- 5: https://github.com/wneessen/go-mail/wiki/Getting-started
- 6: https://github.com/wneessen/go-mail
- 7: https://pkg.go.dev/gopkg.in/go-mail/gomail.v2
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go env GOMODCACHE
GOMODCACHE="$(go env GOMODCACHE)"
# Locate go-mail v0.7.3 in the module cache
rg -n --hidden --no-ignore-vcs --glob='*client.go' "WithTimeout" "${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3" || true
rg -n --hidden --no-ignore-vcs --glob='*.go' "WithTimeout" "${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3" || true
# Show the WithTimeout definition (if found)
rg -n --hidden --no-ignore-vcs "WithTimeout" "${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3" | head -n 50 || trueRepository: jwhumphries/BandWidth
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOMODCACHE="$(go env GOMODCACHE)"
echo "GOMODCACHE=$GOMODCACHE"
# Search for the WithTimeout symbol in the cached dependency source
DEP_DIR="${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3"
if [ -d "$DEP_DIR" ]; then
echo "Found dep dir: $DEP_DIR"
rg -n --glob='*.go' "WithTimeout" "$DEP_DIR" || true
rg -n --glob='*.go' "DialAndSendWithContext" "$DEP_DIR" || true
else
echo "Dep dir not found: $DEP_DIR"
fiRepository: jwhumphries/BandWidth
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOMODCACHE="$(go env GOMODCACHE)"
DEP_DIR="${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3"
echo "GOMODCACHE=$GOMODCACHE"
if [ -d "$DEP_DIR" ]; then
rg -n --glob='*.go' "WithTimeout" "$DEP_DIR" || true
rg -n --glob='*.go' "DialAndSendWithContext" "$DEP_DIR" || true
else
echo "Dep dir not found: $DEP_DIR"
# Try to locate any version and show what has WithTimeout
rg -n --glob='*.go' "WithTimeout" "${GOMODCACHE}/github.com/wneessen/go-mail@"*"/" 2>/dev/null | head -n 50 || true
fiRepository: jwhumphries/BandWidth
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOMODCACHE="$(go env GOMODCACHE)"
DEP_DIR="${GOMODCACHE}/github.com/wneessen/go-mail@v0.7.3"
echo "GOMODCACHE=$GOMODCACHE"
if [ -d "$DEP_DIR" ]; then
rg -n --glob='*.go' "WithTimeout" "$DEP_DIR" || true
rg -n --glob='*.go' "DialAndSendWithContext" "$DEP_DIR" || true
else
echo "Dep dir not found: $DEP_DIR"
# Try to locate any version and show what has WithTimeout
rg -n --glob='*.go' "WithTimeout" "${GOMODCACHE}/github.com/wneessen/go-mail@"*"/" 2>/dev/null | head -n 50 || true
fiRepository: jwhumphries/BandWidth
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mail code around the reported lines
sed -n '1,120p' internal/mail/mail.go
# Check if time is already imported/used
rg -n --type=go '\btime\.' internal/mail/mail.go || trueRepository: jwhumphries/BandWidth
Length of output: 1889
🌐 Web query:
wneessen/go-mail v0.7.3 func WithTimeout
💡 Result:
In wneessen/go-mail version 0.7.3, the WithTimeout function is an option used to configure the connection timeout for a Client [1]. Function Signature: func WithTimeout(timeout time.Duration) Option Purpose: This function sets the connection timeout for a Client, overriding the default timeout (which is 15 seconds) [1][2]. Behavior: - It accepts a time.Duration parameter representing the desired timeout duration [1]. - It returns an Option function that applies the setting to the Client instance [1]. - It validates that the provided timeout is greater than zero; if the timeout is invalid (less than or equal to zero), it returns an ErrInvalidTimeout error [1]. Usage: When creating a new Client, you can pass WithTimeout as an argument to NewClient to apply the custom duration [1][3]. Example: client, err:= mail.NewClient("smtp.example.com", mail.WithTimeout(time.Second * 30))
Citations:
- 1: https://github.com/wneessen/go-mail/blob/main/client.go
- 2: https://github.com/wneessen/go-mail/blob/v0.7.2/client.go
- 3: https://github.com/wneessen/go-mail/blob/main/client_test.go
Add SMTP connection timeout to prevent indefinite hangs in email sending
internal/mail/mail.go creates the SMTP client without any timeout, and client.DialAndSend(msg) has no deadline—network issues can cause the request to block indefinitely. Add gomail.WithTimeout(...) (and import time), or use DialAndSendWithContext with a per-request timeout.
⏱️ Proposed fix to add connection timeout
+ gomail.WithTimeout(10 * time.Second),
gomail.WithPort(m.cfg.Port),
gomail.WithTLSPortPolicy(gomail.TLSOpportunistic),
}🤖 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/mail/mail.go` around lines 72 - 76, The SMTP client is created
without a timeout causing potential indefinite hangs; update the mail send path
by adding a connection timeout option to gomail.NewClient (e.g., include
gomail.WithTimeout(time.Duration) and add the time import) or switch to
client.DialAndSendWithContext and pass a context with timeout per request;
modify the code around gomail.NewClient and the client.DialAndSend(msg) call to
use either gomail.WithTimeout(...) or DialAndSendWithContext(ctx) with
context.WithTimeout so the operation fails after the configured deadline.
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | ||
|
|
||
| codes := []string{"AAAA-BBBB", "CCCC-DDDD"} | ||
| if err := repo.ReplaceBackupCodes(user.ID, codes); err != nil { | ||
| t.Fatalf("ReplaceBackupCodes: %v", err) | ||
| } | ||
|
|
||
| if !repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | ||
| t.Error("valid code rejected") | ||
| } | ||
| if repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | ||
| t.Error("code consumed twice") | ||
| } | ||
| if repo.ConsumeBackupCode(user.ID, "XXXX-YYYY") { | ||
| t.Error("unknown code accepted") | ||
| } | ||
|
|
||
| // Replacing wipes old codes. | ||
| if err := repo.ReplaceBackupCodes(user.ID, []string{"EEEE-FFFF"}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if repo.ConsumeBackupCode(user.ID, "CCCC-DDDD") { | ||
| t.Error("old code survived replacement") | ||
| } | ||
|
|
||
| if err := repo.DeleteBackupCodes(user.ID); err != nil { | ||
| t.Fatalf("DeleteBackupCodes: %v", err) | ||
| } | ||
| if repo.ConsumeBackupCode(user.ID, "EEEE-FFFF") { | ||
| t.Error("code survived deletion") | ||
| } | ||
| } | ||
|
|
||
| func TestPasswordResetLifecycle(t *testing.T) { | ||
| repo := testRepo(t) | ||
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | ||
|
|
||
| token, err := repo.CreatePasswordReset(user.ID) | ||
| if err != nil { | ||
| t.Fatalf("CreatePasswordReset: %v", err) | ||
| } | ||
|
|
||
| gotID, err := repo.ConsumePasswordReset(token) | ||
| if err != nil || gotID != user.ID { | ||
| t.Fatalf("ConsumePasswordReset = %d, %v", gotID, err) | ||
| } | ||
| // Single use. | ||
| if _, err := repo.ConsumePasswordReset(token); err == nil { | ||
| t.Error("reset token consumed twice") | ||
| } | ||
| if _, err := repo.ConsumePasswordReset("bogus"); err == nil { | ||
| t.Error("bogus reset token accepted") | ||
| } | ||
| } | ||
|
|
||
| func TestExpiredPasswordResetRejected(t *testing.T) { | ||
| repo := testRepo(t) | ||
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | ||
| token, _ := repo.CreatePasswordReset(user.ID) | ||
|
|
||
| repo.db.Model(&model.PasswordReset{}). | ||
| Where("user_id = ?", user.ID). | ||
| Update("expires_at", time.Now().Add(-time.Minute)) |
There was a problem hiding this comment.
Do not discard setup errors in recovery lifecycle tests.
Ignoring errors from CreateUser, CreatePasswordReset, and the expiry-forcing update can make tests pass/fail for the wrong reason (for example, consuming an empty token already errors).
Suggested fix pattern
- user, _ := repo.CreateUser("alice", "alice@example.com", "h")
+ user, err := repo.CreateUser("alice", "alice@example.com", "h")
+ if err != nil {
+ t.Fatalf("CreateUser: %v", err)
+ }
- token, _ := repo.CreatePasswordReset(user.ID)
+ token, err := repo.CreatePasswordReset(user.ID)
+ if err != nil {
+ t.Fatalf("CreatePasswordReset: %v", err)
+ }
- repo.db.Model(&model.PasswordReset{}).
+ if err := repo.db.Model(&model.PasswordReset{}).
Where("user_id = ?", user.ID).
- Update("expires_at", time.Now().Add(-time.Minute))
+ Update("expires_at", time.Now().Add(-time.Minute)).Error; err != nil {
+ t.Fatalf("expire reset token: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | |
| codes := []string{"AAAA-BBBB", "CCCC-DDDD"} | |
| if err := repo.ReplaceBackupCodes(user.ID, codes); err != nil { | |
| t.Fatalf("ReplaceBackupCodes: %v", err) | |
| } | |
| if !repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | |
| t.Error("valid code rejected") | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | |
| t.Error("code consumed twice") | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "XXXX-YYYY") { | |
| t.Error("unknown code accepted") | |
| } | |
| // Replacing wipes old codes. | |
| if err := repo.ReplaceBackupCodes(user.ID, []string{"EEEE-FFFF"}); err != nil { | |
| t.Fatal(err) | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "CCCC-DDDD") { | |
| t.Error("old code survived replacement") | |
| } | |
| if err := repo.DeleteBackupCodes(user.ID); err != nil { | |
| t.Fatalf("DeleteBackupCodes: %v", err) | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "EEEE-FFFF") { | |
| t.Error("code survived deletion") | |
| } | |
| } | |
| func TestPasswordResetLifecycle(t *testing.T) { | |
| repo := testRepo(t) | |
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | |
| token, err := repo.CreatePasswordReset(user.ID) | |
| if err != nil { | |
| t.Fatalf("CreatePasswordReset: %v", err) | |
| } | |
| gotID, err := repo.ConsumePasswordReset(token) | |
| if err != nil || gotID != user.ID { | |
| t.Fatalf("ConsumePasswordReset = %d, %v", gotID, err) | |
| } | |
| // Single use. | |
| if _, err := repo.ConsumePasswordReset(token); err == nil { | |
| t.Error("reset token consumed twice") | |
| } | |
| if _, err := repo.ConsumePasswordReset("bogus"); err == nil { | |
| t.Error("bogus reset token accepted") | |
| } | |
| } | |
| func TestExpiredPasswordResetRejected(t *testing.T) { | |
| repo := testRepo(t) | |
| user, _ := repo.CreateUser("alice", "alice@example.com", "h") | |
| token, _ := repo.CreatePasswordReset(user.ID) | |
| repo.db.Model(&model.PasswordReset{}). | |
| Where("user_id = ?", user.ID). | |
| Update("expires_at", time.Now().Add(-time.Minute)) | |
| user, err := repo.CreateUser("alice", "alice@example.com", "h") | |
| if err != nil { | |
| t.Fatalf("CreateUser: %v", err) | |
| } | |
| codes := []string{"AAAA-BBBB", "CCCC-DDDD"} | |
| if err := repo.ReplaceBackupCodes(user.ID, codes); err != nil { | |
| t.Fatalf("ReplaceBackupCodes: %v", err) | |
| } | |
| if !repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | |
| t.Error("valid code rejected") | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "AAAA-BBBB") { | |
| t.Error("code consumed twice") | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "XXXX-YYYY") { | |
| t.Error("unknown code accepted") | |
| } | |
| // Replacing wipes old codes. | |
| if err := repo.ReplaceBackupCodes(user.ID, []string{"EEEE-FFFF"}); err != nil { | |
| t.Fatal(err) | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "CCCC-DDDD") { | |
| t.Error("old code survived replacement") | |
| } | |
| if err := repo.DeleteBackupCodes(user.ID); err != nil { | |
| t.Fatalf("DeleteBackupCodes: %v", err) | |
| } | |
| if repo.ConsumeBackupCode(user.ID, "EEEE-FFFF") { | |
| t.Error("code survived deletion") | |
| } | |
| } | |
| func TestPasswordResetLifecycle(t *testing.T) { | |
| repo := testRepo(t) | |
| user, err := repo.CreateUser("alice", "alice@example.com", "h") | |
| if err != nil { | |
| t.Fatalf("CreateUser: %v", err) | |
| } | |
| token, err := repo.CreatePasswordReset(user.ID) | |
| if err != nil { | |
| t.Fatalf("CreatePasswordReset: %v", err) | |
| } | |
| // Single use. | |
| if _, err := repo.ConsumePasswordReset(token); err == nil { | |
| t.Error("reset token consumed twice") | |
| } | |
| if _, err := repo.ConsumePasswordReset("bogus"); err == nil { | |
| t.Error("bogus reset token accepted") | |
| } | |
| } | |
| func TestExpiredPasswordResetRejected(t *testing.T) { | |
| repo := testRepo(t) | |
| user, err := repo.CreateUser("alice", "alice@example.com", "h") | |
| if err != nil { | |
| t.Fatalf("CreateUser: %v", err) | |
| } | |
| token, err := repo.CreatePasswordReset(user.ID) | |
| if err != nil { | |
| t.Fatalf("CreatePasswordReset: %v", err) | |
| } | |
| if err := repo.db.Model(&model.PasswordReset{}). | |
| Where("user_id = ?", user.ID). | |
| Update("expires_at", time.Now().Add(-time.Minute)).Error; err != nil { | |
| t.Fatalf("expire reset token: %v", err) | |
| } |
🤖 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/repository/recovery_test.go` around lines 12 - 74, The tests
currently ignore errors from repo.CreateUser, repo.CreatePasswordReset, and the
manual expiry update; change them to check returned errors and fail the test
immediately (use t.Fatalf or t.Fatal) if any are non-nil. Specifically, after
calling repo.CreateUser(...) and repo.CreatePasswordReset(...) verify the error
value and stop the test on error, and capture/check the error returned by
repo.db.Model(&model.PasswordReset{}).Where("user_id = ?", user.ID).Update(...)
and fail the test if it returns an error so you never proceed with an
invalid/empty token.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Test Plan
just checkgreen (golangci-lint, ESLint, Prettier, tsc, go test, vitest — 6 gates)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores