Add user invite link feature for embedded IdP#5157
Conversation
📝 WalkthroughWalkthroughAdds a user-invite subsystem (token generation/validation, encrypted persistence, Store/Manager APIs, HTTP handlers with public rate limiting and tests) and a cached instance version endpoint; includes activity codes, mocks, OpenAPI/types, middleware, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as "HTTP Handler"
participant Manager as "Account Manager"
participant Store as "SQL Store"
participant IdP as "Embedded IdP"
rect rgba(200,150,255,0.5)
Note over Client,API: Invite creation flow
Client->>API: POST /api/users/invites (create)
API->>Manager: CreateUserInvite(ctx, accountID, initiatorID, invite, expiresIn)
Manager->>Store: GetUserInviteByEmail(ctx, lock)
Store-->>Manager: (no existing invite)
Manager->>Manager: GenerateInviteToken() → token, hashed
Manager->>Store: SaveUserInvite(ctx, inviteRecord with hashed token)
Store-->>Manager: OK
Manager-->>API: UserInvite (invite_token, expires_at)
API-->>Client: 200 {invite_token, expires_at}
end
sequenceDiagram
actor Client
participant API as "HTTP Handler"
participant Manager as "Account Manager"
participant Store as "SQL Store"
participant IdP as "Embedded IdP"
rect rgba(150,200,255,0.5)
Note over Client,API: Invite acceptance flow
Client->>API: POST /api/users/invites/{token}/accept (password)
API->>Manager: AcceptUserInvite(ctx, token, password)
Manager->>Manager: ValidateInviteToken(token) → hashedToken
Manager->>Store: GetUserInviteByHashedToken(ctx, lock, hashedToken)
Store-->>Manager: UserInviteRecord
Manager->>IdP: CreateUser(email, password)
IdP-->>Manager: Created
Manager->>Store: DeleteUserInvite(ctx, inviteID) (within transaction)
Store-->>Manager: OK
Manager-->>API: success
API-->>Client: 200 {success}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@management/server/http/handler.go`:
- Around line 71-77: The bypass path for invite acceptance is too broad; update
the call to bypass.AddBypassPath that registers "/api/users/invites/*/accept" so
it only matches public invite tokens by using the "nbi_" prefix (i.e.,
"/api/users/invites/nbi_*/accept"). Locate the two calls to bypass.AddBypassPath
in this file (the one that currently adds "/api/users/invites/*/accept" and the
existing "/api/users/invites/nbi_*"), change the accept pattern to include
"nbi_" and keep the same error handling (returning fmt.Errorf on failure). If
endpoint-level rate limiting is required, implement it inside the invite accept
handler rather than in this middleware registration.
In `@management/server/http/handlers/users/invites_handler.go`:
- Around line 212-247: In regenerateInvite, the JSON decode currently treats any
decode error as an empty body and swallows malformed JSON; change the
json.NewDecoder(r.Body).Decode(&req) handling so that only io.EOF is treated as
an empty body — for any other decode error return a 400-style error via
util.WriteError (or status.Errorf(status.InvalidArgument,...)) and exit; locate
the decode in regenerateInvite and use io.EOF to distinguish the empty-body case
from real malformed JSON before calling h.accountManager.RegenerateUserInvite.
In `@management/server/store/sql_store.go`:
- Around line 864-889: The lookup fails because s.fieldEncrypt.Encrypt uses a
random nonce so ciphertexts differ; replace the encrypted-email equality search
with a deterministic lookup (add and use a separate deterministic column such as
invite.EmailHash or invite.NormalizedEmail). Compute the deterministic value
when creating/updating invites (e.g., HMAC-SHA256 or SHA256 of normalized
lowercased email using a server-side salt/key) and change this function to
compute that same hash (instead of calling s.fieldEncrypt.Encrypt) and call
tx.Take(&invite, "account_id = ? AND email_hash = ?", accountID, computedHash).
Also update create/update paths to populate EmailHash and add a DB migration for
the new column so existing rows are backfilled.
In `@management/server/user.go`:
- Around line 1457-1539: In CreateUserInvite, don't ignore the error returned by
am.Store.GetUserInviteByEmail: capture the error, and if it's non-nil then check
errors.Is(err, store.ErrNotFound) (or the repo's specific not-found sentinel)
and continue only for NotFound; for any other error return or wrap it (e.g.,
return fmt.Errorf("failed to query invites: %w", err)). Update the logic around
existingInvite and the GetUserInviteByEmail call so duplicates are still checked
but transient DB errors are propagated instead of discarded.
- Around line 1565-1632: AcceptUserInvite currently creates the IdP user via
embeddedIdp.CreateUserWithPassword before calling am.Store.ExecuteInTransaction,
so if the DB transaction (transaction.SaveUser or transaction.DeleteUserInvite)
fails the IdP user becomes orphaned; add a best-effort rollback that deletes the
IdP user when ExecuteInTransaction returns an error. Concretely: after calling
ExecuteInTransaction in AcceptUserInvite, if err != nil then call
embeddedIdp.DeleteUser(ctx, idpUser.ID) (or the EmbeddedIdPManager method that
removes a user) and log any deletion error, but still return the original
transaction error; keep the CreateUserWithPassword and transaction calls as-is
and only perform the deletion on transaction failure to avoid leaving orphaned
IdP users.
🧹 Nitpick comments (1)
management/server/http/handler.go (1)
142-143: Gate invite endpoints to embedded IdP mode to match the pattern for other IdP-dependent features.The endpoints
AddInvitesEndpointsandAddPublicInvitesEndpointsare registered unconditionally, while the underlying manager methods (CreateUserInvite,AcceptUserInvite, etc.) are specific to embedded IdP and include type assertions checking forEmbeddedIdPManager. Other IdP-dependent features in the codebase follow a consistent pattern: the accounts endpoints receive anembeddedIdpEnabledflag, and the OAuth2 handler is conditionally registered withif embeddedIdpEnabled. For consistency and clarity, conditionally register these invite endpoints only when embedded IdP is enabled.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shared/management/http/api/openapi.yml`:
- Around line 3543-3603: The operations for public invite endpoints are
inheriting the global auth; update the OpenAPI spec to explicitly mark them as
unauthenticated by adding security: [] to each operation (the GET operation
under the path /api/users/invites/{token} and the POST operation under
/api/users/invites/{token}/accept) so generated clients and docs won’t apply the
global Bearer/Token security to these endpoints.
♻️ Duplicate comments (2)
management/server/user.go (1)
1487-1490: HandleGetUserInviteByEmailerrors instead of discarding them.Line 1488 ignores store errors, which can mask DB outages and allow duplicate invites. Please handle NotFound explicitly and return other errors.
🐛 Suggested fix
- // Check if invite already exists for this email - existingInvite, _ := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) - if existingInvite != nil { + // Check if invite already exists for this email + existingInvite, err := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) + if err != nil { + if s, ok := status.FromError(err); !ok || s.Type() != status.NotFound { + return nil, err + } + } + if existingInvite != nil { return nil, status.Errorf(status.AlreadyExists, "invite already exists for this email") }management/server/http/handlers/users/invites_handler.go (1)
178-182: Don’t swallow malformed JSON inregenerateInvite.Right now any decode error is treated as empty body; only
io.EOFshould be. Malformed JSON should return 400.🐛 Suggested fix
-import ( - "encoding/json" - "net/http" +import ( + "encoding/json" + "io" + "net/http" @@ - var req api.UserInviteRegenerateRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - // Allow empty body - expiresIn is optional - req = api.UserInviteRegenerateRequest{} - } + var req api.UserInviteRegenerateRequest + dec := json.NewDecoder(r.Body) + if err := dec.Decode(&req); err != nil { + if err != io.EOF { + util.WriteErrorResponse("couldn't parse JSON request", http.StatusBadRequest, w) + return + } + }
🧹 Nitpick comments (1)
shared/management/http/api/openapi.yml (1)
596-605: Document invite password constraints to match other password endpoints.
UserInviteAcceptRequestdoesn’t specify format/minLength, whileSetupRequestdoes. If the same policy applies, exposing it here avoids client-side mismatches.♻️ Suggested OpenAPI update
UserInviteAcceptRequest: type: object description: Request to accept an invite and set password properties: password: description: The password the user wants to set type: string + format: password + minLength: 8 example: SecurePass123!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shared/management/http/api/openapi.yml`:
- Around line 596-605: The UserInviteAcceptRequest schema's password property
lacks validation and should match the SetupRequest constraints; update the
OpenAPI schema for UserInviteAcceptRequest (password property) to include the
same validation as SetupRequest (e.g., add minLength: 8 and any existing
pattern/format used by SetupRequest) so invited users cannot set weaker
passwords, ensuring consistency across schemas.
🧹 Nitpick comments (1)
shared/management/http/api/openapi.yml (1)
552-555: Consider clarifying theinvite_linkfield name/description.The field is named
invite_linkbut contains a token value (e.g.,nbi_d5p7eedra0h0lt6f59hg-RPWyaKMfu2yCIvO3NzxIYUCl5Rw1Ld4W), not a full URL. Consider either:
- Renaming to
invite_tokenfor accuracy, or- Making it a full URL that clients can use directly
This is a minor documentation concern but could help API consumers understand the expected format.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/types/user_invite.go`:
- Around line 49-66: GenerateInviteToken currently pads the base62-encoded
checksum using fmt.Sprintf("%06s", encodedChecksum) which pads with spaces and
causes base62.Decode in ValidateInviteToken to fail; change the padding to
explicit zero-padding (pad with '0' to length 6) so checksum strings never
contain spaces. Update the logic in GenerateInviteToken (refer to function
GenerateInviteToken, variables InviteTokenPrefix and InviteTokenSecretLength) to
produce a 6-character checksum by left-padding encodedChecksum with '0'
characters (e.g. use strings.Repeat or equivalent) before concatenating and
hashing.
♻️ Duplicate comments (2)
management/server/http/handlers/users/invites_handler.go (1)
195-219: Don’t swallow malformed JSON in regenerateInvite.
Line 216 currently treats any decode error as “empty body.” Onlyio.EOFshould be allowed; otherwise return 400.🐛 Proposed fix
-import ( - "encoding/json" - "net/http" +import ( + "encoding/json" + "io" + "net/http" @@ - var req api.UserInviteRegenerateRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - // Allow empty body - expiresIn is optional - req = api.UserInviteRegenerateRequest{} - } + var req api.UserInviteRegenerateRequest + dec := json.NewDecoder(r.Body) + if err := dec.Decode(&req); err != nil { + if err != io.EOF { + util.WriteErrorResponse("couldn't parse JSON request", http.StatusBadRequest, w) + return + } + }management/server/user.go (1)
1487-1491: HandleGetUserInviteByEmailerrors instead of discarding them.
Line 1488 ignores store errors; transient failures can allow duplicate invites or mask outages. Treat NotFound as empty and propagate other errors.🐛 Proposed fix
- // Check if invite already exists for this email - existingInvite, _ := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) - if existingInvite != nil { + // Check if invite already exists for this email + existingInvite, err := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) + if err != nil { + if s, ok := status.FromError(err); !ok || s.Type() != status.NotFound { + return nil, err + } + } + if existingInvite != nil { return nil, status.Errorf(status.AlreadyExists, "invite already exists for this email") }#!/bin/bash # Locate GetUserInviteByEmail implementation and verify its NotFound behavior. rg -n --type=go 'func\s+\(.*\)\s+GetUserInviteByEmail'
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/handlers/users/invites_handler.go`:
- Around line 180-190: The accept-invite handler currently decodes
api.UserInviteAcceptRequest and directly calls
h.accountManager.AcceptUserInvite(token, req.Password); add a password strength
check before that call (enforce minimum length, disallow trivial passwords, and
require complexity — reuse the project's existing password validation helper if
present, or implement a validator used by other user creation flows) and return
a 400 with a clear message on failure; additionally ensure this endpoint is
subject to rate limiting by removing it from the bypass list in handler
registration (handler.go bypass entries) or by applying the same rate-limiter
middleware used elsewhere (throttle by IP and/or invite token) so repeated
accept attempts are limited.
♻️ Duplicate comments (2)
management/server/http/handlers/users/invites_handler.go (1)
215-219: Malformed JSON should return 400, not be silently ignored.This issue was flagged in a previous review. Currently, any JSON decode error (including malformed JSON) is treated as "empty body." Only
io.EOFshould allow proceeding with defaults; other errors should return a 400 Bad Request.management/server/user.go (1)
1487-1491: Don't ignoreGetUserInviteByEmailerrors.Line 1488 discards store errors; transient DB failures could allow duplicate invites or hide outages. Handle NotFound explicitly and propagate other errors.
🐛 Proposed fix
// Check if invite already exists for this email - existingInvite, _ := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) + existingInvite, err := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) + if err != nil { + if s, ok := status.FromError(err); !ok || s.Type() != status.NotFound { + return nil, err + } + } if existingInvite != nil { return nil, status.Errorf(status.AlreadyExists, "invite already exists for this email") }
🧹 Nitpick comments (2)
management/server/http/handlers/users/invites_handler.go (1)
42-46: Redundant HTTP method checks can be removed.Since
router.HandleFunc(...).Methods("GET", "OPTIONS")already restricts the handler to specific methods, the manualr.Method != http.MethodGetcheck is redundant. The router won't invoke this handler for other methods. This pattern repeats in all handlers.♻️ Suggested simplification
func (h *invitesHandler) listInvites(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodGet { - util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) - return - } - userAuth, err := nbcontext.GetUserAuthFromContext(r.Context())Apply similar changes to
createInvite,getInviteInfo,acceptInvite, andregenerateInvite.management/server/store/sql_store.go (1)
932-940: Consider returning NotFound error when no rows are deleted.Unlike other
Delete*methods (e.g.,DeletePeerat line 3163), this method doesn't checkRowsAffectedand silently succeeds if the invite doesn't exist. This inconsistency could hide bugs.♻️ Proposed fix
func (s *SqlStore) DeleteUserInvite(ctx context.Context, inviteID string) error { result := s.db.Delete(&types.UserInvite{}, idQueryCondition, inviteID) if result.Error != nil { log.WithContext(ctx).Errorf("failed to delete user invite from store: %s", result.Error) return status.Errorf(status.Internal, "failed to delete user invite from store") } + + if result.RowsAffected == 0 { + return status.Errorf(status.NotFound, "user invite not found") + } + return nil }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@management/server/user.go`:
- Around line 1772-1812: The password validator validatePassword duplicates
logic elsewhere—extract this function into a shared helper (e.g., a new
package-level helper like password.Validate or auth.ValidatePassword) and
replace the duplicate implementation in invites_handler.go to call that shared
function (update imports and any error handling accordingly); then update the
OpenAPI/SDK comment or schema for UserInviteAcceptRequest to document the full
policy (minimum 8 characters, at least one digit, one uppercase letter, and one
special character) so docs and generated SDKs reflect the stricter requirements.
- Around line 1667-1742: RegenerateUserInvite currently deletes the old invite
and creates a new one with a new ID (newInviteID) but does not return that new
ID, breaking clients; instead reuse the existing invite ID: update
existingInvite.HashedToken = hashedToken, existingInvite.ExpiresAt = expiresAt
and set UpdatedBy/UpdatedAt (or CreatedBy if appropriate), then inside
ExecuteInTransaction call transaction.SaveUserInvite(ctx, existingInvite)
(remove the DeleteUserInvite and the types.NewInviteID creation), keep
am.StoreEvent using existingInvite.ID, and return types.UserInviteResponse with
ID = existingInvite.ID and the plainToken and new expiry so callers retain a
stable invite ID.
♻️ Duplicate comments (1)
management/server/http/handlers/users/invites_handler.go (1)
27-33: Public accept-invite route isn’t rate-limited yet.
inviteAcceptRateLimiteris configured but not attached to the/users/invites/{token}/accepthandler, so the public endpoint remains unthrottled. Please wire the limiter using the existing middleware pattern (or remove it if not intended).#!/bin/bash # Find existing APIRateLimiter usage to mirror rg -n 'NewAPIRateLimiter|APIRateLimiter' --type go -C3 # Locate invite accept route wiring rg -n 'invites/\{token\}/accept|acceptInvite' --type go -C3Also applies to: 91-98
🧹 Nitpick comments (2)
shared/management/http/api/openapi.yml (2)
513-516: Add lower bounds for invite expiration seconds.
expires_incurrently allows zero/negative values in the spec. If the server enforces a positive value, mirror it here so clients can validate early.♻️ Proposed diff
expires_in: description: Invite expiration time in seconds (default 72 hours) type: integer + minimum: 1 example: 259200 ... expires_in: description: Invite expiration time in seconds (default 72 hours) type: integer + minimum: 1 example: 259200Also applies to: 676-679
556-560: Standardize invite expiration field naming across schemas.The OpenAPI schema uses inconsistent field names for invite expiration:
invite_expires_atin create/regenerate responses (UserInviteCreateResponse, UserInviteRegenerateResponse) while list/info responses useexpires_at(UserInviteList, UserInviteInfo). Since the backend data structure uses onlyExpiresAt, standardizing onexpires_atwould reduce client special-casing and align with the actual backend representation.♻️ Proposed diff (standardizing on
expires_at)- invite_expires_at: - description: Invite expiration time + expires_at: + description: Invite expiration time type: string format: date-time example: "2024-01-25T10:00:00Z" ... - - invite_expires_at + - expires_at ... - invite_expires_at: - description: New invite expiration time + expires_at: + description: New invite expiration time type: string format: date-time example: "2024-01-28T10:00:00Z" ... - - invite_expires_at + - expires_atAlso applies to: 596-600, 631-635, 688-692
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/handlers/users/invites_handler_test.go`:
- Around line 526-531: The tests reveal inconsistent responses for invalid JSON
between createInvite and regenerateInvite; unify their error handling so both
return the same HTTP status for malformed JSON (choose either
http.StatusBadRequest or http.StatusUnprocessableEntity). Update the
parsing/error branch in the createInvite handler and/or the regenerateInvite
handler to use the agreed status code instead of mixing direct
http.StatusBadRequest with gRPC status.InvalidArgument mapping; ensure the
change is applied to the JSON decode/error handling logic in the createInvite
and regenerateInvite functions (and update tests accordingly) so both endpoints
behave consistently for invalid JSON.
🧹 Nitpick comments (5)
management/server/http/handlers/users/invites_handler_test.go (2)
25-32: Test token format doesn't match documented format.The PR description specifies the token format as
nbi_{xid}-{32-char-secret}, buttestInviteTokenis"nbi_testtoken123456789012345678"without the hyphen separator between XID and secret. While this doesn't break tests since token validation is mocked, aligning the test constant with the actual format would improve clarity and catch format-related regressions if tests evolve.Suggested format alignment
- testInviteToken = "nbi_testtoken123456789012345678" + testInviteToken = "nbi_abc123xid-testsecret1234567890123456"
391-425: Consider adding a test case for missing lowercase letter.The password validation tests cover missing digit, uppercase, and special character, but not missing lowercase. If the password policy requires lowercase letters, adding this test case would ensure complete coverage of all password requirements.
Optional additional test case
{ name: "password missing lowercase", token: testInviteToken, requestBody: `{"password":"NOLOWERCASE1!"}`, expectedStatus: http.StatusUnprocessableEntity, mockFunc: func(ctx context.Context, token, password string) error { return status.Errorf(status.InvalidArgument, "password must contain at least one lowercase letter") }, },management/server/user_invite_test.go (3)
335-357: Avoid fixed sleeps to reduce flakiness.A fixed
time.Sleepmakes tests slower and can still be flaky on busy CI. Preferrequire.Eventuallyto wait for the expiry condition.♻️ Suggested refactor
- // Wait for expiration - time.Sleep(2 * time.Second) - - // Get the invite info - should still return info but Valid should be false - info, err := am.GetUserInviteInfo(context.Background(), result.InviteLink) - require.NoError(t, err) - assert.False(t, info.Valid) + // Wait until invite expires (avoid fixed sleep) + require.Eventually(t, func() bool { + info, err := am.GetUserInviteInfo(context.Background(), result.InviteLink) + return err == nil && !info.Valid + }, 3*time.Second, 100*time.Millisecond)
625-650: Avoid fixed sleeps to reduce flakiness.Same concern as above; prefer waiting on the expiry condition rather than sleeping.
♻️ Suggested refactor
- // Wait for expiration - time.Sleep(2 * time.Second) + // Wait until invite expires (avoid fixed sleep) + require.Eventually(t, func() bool { + info, err := am.GetUserInviteInfo(context.Background(), result.InviteLink) + return err == nil && !info.Valid + }, 3*time.Second, 100*time.Millisecond) err = am.AcceptUserInvite(context.Background(), result.InviteLink, "Password1!") require.Error(t, err)
27-91: Usefilepath.Joinfor path construction.String concatenation with
"/"for path construction is non-portable and non-idiomatic in Go. Usefilepath.Joininstead for proper cross-platform compatibility (especially Windows).♻️ Suggested portability fix
import ( "context" "os" + "path/filepath" "testing" "time" "github.com/stretchr/testify/assert"tmpDir := t.TempDir() - dexDataDir := tmpDir + "/dex" + dexDataDir := filepath.Join(tmpDir, "dex") require.NoError(t, os.MkdirAll(dexDataDir, 0700))Config: idp.EmbeddedStorageTypeConfig{ - File: dexDataDir + "/dex.db", + File: filepath.Join(dexDataDir, "dex.db"), },
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/middleware/rate_limiter.go`:
- Around line 165-191: The getClientIP function currently trusts X-Forwarded-For
/ X-Real-IP as-is; harden it by: accept those headers only when requests
originate from trusted proxies (add a config flag/trusted proxy list check in
the middleware before calling getClientIP), then parse and normalize values
using net.ParseIP (for X-Forwarded-For split on ',' and trim spaces, take the
first non-empty token, call net.ParseIP and only return it if valid), likewise
trim/validate X-Real-IP before returning, and otherwise fall back to parsing
r.RemoteAddr with net.SplitHostPort and net.ParseIP; if any header value fails
validation, ignore it and continue to the next source so attackers cannot spoof
IPs to bypass rate limits.
| // getClientIP extracts the client IP address from the request. | ||
| // It checks X-Forwarded-For and X-Real-IP headers first, then falls back to RemoteAddr. | ||
| func getClientIP(r *http.Request) string { | ||
| // Check X-Forwarded-For header (may contain multiple IPs, take the first) | ||
| if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | ||
| if idx := len(xff); idx > 0 { | ||
| // Take the first IP in the list (original client) | ||
| for i := 0; i < len(xff); i++ { | ||
| if xff[i] == ',' { | ||
| return xff[:i] | ||
| } | ||
| } | ||
| return xff | ||
| } | ||
| } | ||
|
|
||
| // Check X-Real-IP header | ||
| if xri := r.Header.Get("X-Real-IP"); xri != "" { | ||
| return xri | ||
| } | ||
|
|
||
| // Fall back to RemoteAddr | ||
| ip, _, err := net.SplitHostPort(r.RemoteAddr) | ||
| if err != nil { | ||
| return r.RemoteAddr | ||
| } | ||
| return ip |
There was a problem hiding this comment.
Harden client IP extraction to prevent rate‑limit bypass.
X-Forwarded-For / X-Real-IP are client‑controlled unless you only trust them from known proxies. As written, an attacker can spoof arbitrary values to evade per‑IP limiting and inflate the limiter map. At minimum, normalize and validate IPs, and consider only honoring these headers when the request comes from a trusted proxy (or behind a config flag).
🔧 Suggested fix (normalize/validate IPs)
import (
"context"
"net"
"net/http"
+ "strings"
"sync"
"time"
"golang.org/x/time/rate"
"github.com/netbirdio/netbird/shared/management/http/util"
)
@@
func getClientIP(r *http.Request) string {
// Check X-Forwarded-For header (may contain multiple IPs, take the first)
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
- if idx := len(xff); idx > 0 {
- // Take the first IP in the list (original client)
- for i := 0; i < len(xff); i++ {
- if xff[i] == ',' {
- return xff[:i]
- }
- }
- return xff
- }
+ ip := strings.TrimSpace(strings.Split(xff, ",")[0])
+ if parsed := net.ParseIP(ip); parsed != nil {
+ return parsed.String()
+ }
}
// Check X-Real-IP header
- if xri := r.Header.Get("X-Real-IP"); xri != "" {
- return xri
+ if xri := strings.TrimSpace(r.Header.Get("X-Real-IP")); xri != "" {
+ if parsed := net.ParseIP(xri); parsed != nil {
+ return parsed.String()
+ }
+ if host, _, err := net.SplitHostPort(xri); err == nil {
+ if parsed := net.ParseIP(host); parsed != nil {
+ return parsed.String()
+ }
+ }
}🤖 Prompt for AI Agents
In `@management/server/http/middleware/rate_limiter.go` around lines 165 - 191,
The getClientIP function currently trusts X-Forwarded-For / X-Real-IP as-is;
harden it by: accept those headers only when requests originate from trusted
proxies (add a config flag/trusted proxy list check in the middleware before
calling getClientIP), then parse and normalize values using net.ParseIP (for
X-Forwarded-For split on ',' and trim spaces, take the first non-empty token,
call net.ParseIP and only return it if valid), likewise trim/validate X-Real-IP
before returning, and otherwise fall back to parsing r.RemoteAddr with
net.SplitHostPort and net.ParseIP; if any header value fails validation, ignore
it and continue to the next source so attackers cannot spoof IPs to bypass rate
limits.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/middleware/rate_limiter.go`:
- Around line 165-172: Remove the unused IP-extraction helper and the unused
HTTP middleware: delete the getClientIP function and the Middleware() method
that calls it (both are unused because the rate limiter is token-based via
rateLimiter.Allow(token)); ensure any references to getClientIP or Middleware
are removed and clean up any now-unused imports (e.g., net) or types introduced
solely for that method; if IP-based rate limiting is needed later, note to
integrate with the existing realip package instead of reintroducing getClientIP.
| // getClientIP extracts the client IP address from the request. | ||
| func getClientIP(r *http.Request) string { | ||
| ip, _, err := net.SplitHostPort(r.RemoteAddr) | ||
| if err != nil { | ||
| return r.RemoteAddr | ||
| } | ||
| return ip | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for proxy/forwarded header handling in the codebase
rg -n "X-Forwarded-For|X-Real-IP|TrustedProxy|trusted.*proxy" --type=go -C2Repository: netbirdio/netbird
Length of output: 1200
🏁 Script executed:
#!/bin/bash
# Check the full rate_limiter.go file for any configuration options
wc -l management/server/http/middleware/rate_limiter.goRepository: netbirdio/netbird
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Look for documentation on rate limiting or proxy deployment
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "proxy\|rate.*limit" 2>/dev/null | head -20Repository: netbirdio/netbird
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check boot.go for how realip middleware is set up
sed -n '100,150p' management/internals/server/boot.goRepository: netbirdio/netbird
Length of output: 2722
🏁 Script executed:
#!/bin/bash
# Check the full context of the rate_limiter.go file to see if it's middleware in a chain
head -50 management/server/http/middleware/rate_limiter.goRepository: netbirdio/netbird
Length of output: 1383
🏁 Script executed:
#!/bin/bash
# Search for where rate limiter middleware is registered in the HTTP handler chain
rg -n "rate_limiter|RateLimiter" --type=go -B2 -A2 | head -60Repository: netbirdio/netbird
Length of output: 4227
🏁 Script executed:
#!/bin/bash
# Check if realip is used in the middleware chain before rate limiter
rg -n "realip|middleware" management/internals/server/boot.go -C3Repository: netbirdio/netbird
Length of output: 1236
🏁 Script executed:
#!/bin/bash
# View the full rate_limiter.go file to understand the complete implementation
cat -n management/server/http/middleware/rate_limiter.goRepository: netbirdio/netbird
Length of output: 5634
🏁 Script executed:
#!/bin/bash
# Check handler.go to see how the rate limiter is integrated into the HTTP handler chain
sed -n '70,140p' management/server/http/handler.goRepository: netbirdio/netbird
Length of output: 2446
🏁 Script executed:
#!/bin/bash
# Check where rateLimitingConfig is used in authMiddleware
fd "auth.*middleware" --type=f management/Repository: netbirdio/netbird
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Search for how the rate limiter config is used in the auth middleware
rg -n "rateLimitingConfig\|RateLimiterConfig" management/ --type=go -A3 -B1 | head -80Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's any middleware that applies realip before the auth middleware
rg -n "realip.*http\|HTTP.*realip" management/ --type=goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the auth_middleware.go file to see how rateLimitingConfig is used
head -100 management/server/http/middleware/auth_middleware.goRepository: netbirdio/netbird
Length of output: 3321
🏁 Script executed:
#!/bin/bash
# Check if rate limiting is enabled by default or only via environment variable
rg -n "rateLimitingEnabledKey\|RATE_LIMIT" management/ --type=goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search in handler.go for where the rate limiter is actually applied
rg -n "Middleware\|rateLimitingConfig" management/server/http/handler.go -B2 -A2Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about rate limiting behavior
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) -exec rg -l "rate.limit\|rate-limit" {} \;Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the full auth middleware handler to see if rate limiter is applied
sed -n '60,150p' management/server/http/middleware/auth_middleware.goRepository: netbirdio/netbird
Length of output: 3021
🏁 Script executed:
#!/bin/bash
# Check if rate limiter is actually called in the auth middleware handler
rg -n "rateLimiter\." management/server/http/middleware/auth_middleware.go -C2Repository: netbirdio/netbird
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Check if the Middleware() function from rate_limiter is actually used anywhere
rg -n "\.Middleware\(" management/ --type=goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Verify this by checking what exactly is passed as the 'key' in rate limiter
sed -n '178,190p' management/server/http/middleware/auth_middleware.goRepository: netbirdio/netbird
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Let me understand this better - check all uses of rateLimiter in auth middleware
rg -n "rateLimiter" management/server/http/middleware/auth_middleware.go -B3 -A3Repository: netbirdio/netbird
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Check if the Middleware() function from rate_limiter is ever instantiated or used
rg -n "rate_limiter\.Middleware\|APIRateLimiter.*Middleware" management/ --type=goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Double-check the entire rate_limiter.go to see if Middleware is used
rg -n "Middleware" management/server/http/middleware/rate_limiter.go -C5Repository: netbirdio/netbird
Length of output: 636
Remove unused getClientIP() function and Middleware() method.
The rate limiter in this codebase is token-based (using rateLimiter.Allow(token)), not IP-based. The getClientIP() function and the Middleware() method that depends on it are not actually used anywhere. The unused Middleware() method can be removed, along with getClientIP().
If an IP-based rate limiter is needed in the future, it should integrate with the existing realip package (already used by gRPC) to properly handle proxy deployments.
🤖 Prompt for AI Agents
In `@management/server/http/middleware/rate_limiter.go` around lines 165 - 172,
Remove the unused IP-extraction helper and the unused HTTP middleware: delete
the getClientIP function and the Middleware() method that calls it (both are
unused because the rate limiter is token-based via rateLimiter.Allow(token));
ensure any references to getClientIP or Middleware are removed and clean up any
now-unused imports (e.g., net) or types introduced solely for that method; if
IP-based rate limiting is needed later, note to integrate with the existing
realip package instead of reintroducing getClientIP.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/middleware/rate_limiter_test.go`:
- Around line 13-18: The tests use RateLimiterConfig with RequestsPerMinute=60
which can allow a refill during CI runs and cause flaky "third request denied"
assertions; change RequestsPerMinute to a very low rate (e.g., 1 per minute) in
the NewAPIRateLimiter calls in this test file (references: NewAPIRateLimiter,
RateLimiterConfig, RequestsPerMinute, Burst) so that no token refill can occur
between the quick successive requests—apply the same tweak to the other test
configs mentioned (the blocks around the other NewAPIRateLimiter invocations).
| rl := NewAPIRateLimiter(&RateLimiterConfig{ | ||
| RequestsPerMinute: 60, // 1 per second | ||
| Burst: 2, | ||
| CleanupInterval: time.Minute, | ||
| LimiterTTL: time.Minute, | ||
| }) |
There was a problem hiding this comment.
Reduce time-based flakiness in rate-limiter tests.
With 60 RPM (≈1/sec), a slow CI slice can allow a refill between calls and make “third request denied” assertions flaky. Consider using a much lower rate so no refill happens during the test’s quick succession of requests.
🔧 Suggested tweak (apply to all test configs)
- RequestsPerMinute: 60, // 1 per second
+ RequestsPerMinute: 1, // 1 per minute: avoid refill during testsAlso applies to: 33-38, 67-72, 141-146
🤖 Prompt for AI Agents
In `@management/server/http/middleware/rate_limiter_test.go` around lines 13 - 18,
The tests use RateLimiterConfig with RequestsPerMinute=60 which can allow a
refill during CI runs and cause flaky "third request denied" assertions; change
RequestsPerMinute to a very low rate (e.g., 1 per minute) in the
NewAPIRateLimiter calls in this test file (references: NewAPIRateLimiter,
RateLimiterConfig, RequestsPerMinute, Burst) so that no token refill can occur
between the quick successive requests—apply the same tweak to the other test
configs mentioned (the blocks around the other NewAPIRateLimiter invocations).
* Add version endpoint * Add version endpoint * Add version endpoint * Don't call GitHub endpoints in tests * Fix API definition for invite passwords * Added 422 validation_failed response to all invite-related POST endpoints:
mlsmaycon
left a comment
mlsmaycon
left a comment
There was a problem hiding this comment.
Left a few notes. We need to confirm the request rate is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@management/server/user.go`:
- Around line 1779-1819: The password length check in validatePassword uses
len(password) which counts bytes, not Unicode runes; replace that check to use
utf8.RuneCountInString(password) against minPasswordLength so multi-byte
characters are counted correctly (update the conditional in validatePassword and
import "unicode/utf8" if not already present).
- Around line 1477-1544: The invite flow does case-insensitive checks with
strings.EqualFold but persists raw email, allowing racey duplicates; normalize
the email (e.g., lowercase and trim) on the incoming invite before any lookups
or creation (affecting the checks that call GetAccountUsers,
GetUserInviteByEmail and the UserInviteRecord fields used in SaveUserInvite) and
ensure the DB enforces a unique constraint on (account_id, lower(email)) so
concurrent/variant-case requests cannot create multiple invites; also update any
query implementations (GetUserInviteByEmail) to compare lower(email) to the
normalized value.
♻️ Duplicate comments (1)
management/server/store/sql_store.go (1)
881-907: Consider a deterministic email lookup for scale.
GetUserInviteByEmaildecrypts every invite for an account, which is O(n) and can become expensive for large accounts. A normalized email hash column would keep lookups indexed and avoid full‑table decrypts.
| // Check if user already exists in NetBird DB | ||
| existingUsers, err := am.Store.GetAccountUsers(ctx, store.LockingStrengthNone, accountID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, user := range existingUsers { | ||
| if strings.EqualFold(user.Email, invite.Email) { | ||
| return nil, status.Errorf(status.UserAlreadyExists, "user with this email already exists") | ||
| } | ||
| } | ||
|
|
||
| // Check if invite already exists for this email | ||
| existingInvite, err := am.Store.GetUserInviteByEmail(ctx, store.LockingStrengthNone, accountID, invite.Email) | ||
| if err != nil { | ||
| if sErr, ok := status.FromError(err); !ok || sErr.Type() != status.NotFound { | ||
| return nil, fmt.Errorf("failed to check existing invites: %w", err) | ||
| } | ||
| } | ||
| if existingInvite != nil { | ||
| return nil, status.Errorf(status.AlreadyExists, "invite already exists for this email") | ||
| } | ||
|
|
||
| // Calculate expiration time | ||
| if expiresIn <= 0 { | ||
| expiresIn = types.DefaultInviteExpirationSeconds | ||
| } | ||
| expiresAt := time.Now().UTC().Add(time.Duration(expiresIn) * time.Second) | ||
|
|
||
| // Generate invite token | ||
| inviteID := types.NewInviteID() | ||
| hashedToken, plainToken, err := types.GenerateInviteToken() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate invite token: %w", err) | ||
| } | ||
|
|
||
| // Create the invite record (no user created yet) | ||
| userInvite := &types.UserInviteRecord{ | ||
| ID: inviteID, | ||
| AccountID: accountID, | ||
| Email: invite.Email, | ||
| Name: invite.Name, | ||
| Role: invite.Role, | ||
| AutoGroups: invite.AutoGroups, | ||
| HashedToken: hashedToken, | ||
| ExpiresAt: expiresAt, | ||
| CreatedAt: time.Now().UTC(), | ||
| CreatedBy: initiatorUserID, | ||
| } | ||
|
|
||
| if err := am.Store.SaveUserInvite(ctx, userInvite); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| am.StoreEvent(ctx, initiatorUserID, inviteID, accountID, activity.UserInviteLinkCreated, map[string]any{"email": invite.Email}) | ||
|
|
||
| return &types.UserInvite{ | ||
| UserInfo: &types.UserInfo{ | ||
| ID: inviteID, | ||
| Email: invite.Email, | ||
| Name: invite.Name, | ||
| Role: invite.Role, | ||
| AutoGroups: invite.AutoGroups, | ||
| Status: string(types.UserStatusInvited), | ||
| Issued: types.UserIssuedAPI, | ||
| }, | ||
| InviteLink: plainToken, | ||
| InviteExpiresAt: expiresAt, | ||
| }, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find migration files and schema definitions related to user_invites
find . -type f \( -name "*.sql" -o -name "*migration*" \) | head -20Repository: netbirdio/netbird
Length of output: 756
🏁 Script executed:
# Search for user_invites references in migration files and SQL
find . -type f \( -name "*.sql" -o -name "*migration*.go" \) -exec grep -l "user_invite" {} \;Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Look for database schema constraints on user_invites
rg -n "user_invite" -t sql -t go -A 5 -B 5 | grep -i "unique\|constraint\|index" | head -30Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Search for user_invites table definition
fd -type f -name "*.sql" -o -name "*.go" | xargs rg -l "CREATE TABLE.*user_invite"Repository: netbirdio/netbird
Length of output: 233
🏁 Script executed:
# Look for GetUserInviteByEmail implementation
ast-grep --pattern 'func $_(ctx $_, $$$) GetUserInviteByEmail($$$) {
$$$
}'Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Search for user_invites table definition in migration files
grep -n "user_invite" ./management/server/migration/migration.go | head -20Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Look at the store.sql testdata to understand schema
grep -A 20 "user_invite" ./management/server/testdata/store.sql | head -40Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Search for GetUserInviteByEmail implementation
rg "GetUserInviteByEmail" -n -B 3 -A 10Repository: netbirdio/netbird
Length of output: 4212
🏁 Script executed:
# Look for the Store interface and UserInviteRecord type
rg "type UserInviteRecord" -n -A 10Repository: netbirdio/netbird
Length of output: 1036
🏁 Script executed:
# Search for email normalization in user invite related code
rg "strings.ToLower|strings.TrimSpace" -B 3 -A 3 | grep -i "invite\|email" -A 3 -B 3Repository: netbirdio/netbird
Length of output: 870
🏁 Script executed:
# Look for the SaveUserInvite implementation to understand where email is persisted
rg "SaveUserInvite" -n -B 2 -A 15Repository: netbirdio/netbird
Length of output: 5988
🏁 Script executed:
# Get the complete GetUserInviteByEmail implementation
sed -n '881,920p' management/server/store/sql_store.goRepository: netbirdio/netbird
Length of output: 1797
🏁 Script executed:
# Check CreateUserInvite function signature and implementation start
sed -n '1460,1495p' management/server/user.goRepository: netbirdio/netbird
Length of output: 1473
🏁 Script executed:
# Check if there's any email normalization in the invite flow
rg "CreateUserInvite|AcceptUserInvite" -B 5 -A 2 management/server/user.go | head -50Repository: netbirdio/netbird
Length of output: 1008
🏁 Script executed:
# Check validateUserInvite to see if it does any email validation/normalization
rg "func validateUserInvite" -A 20Repository: netbirdio/netbird
Length of output: 1230
🏁 Script executed:
# Check if email is used in any other comparisons elsewhere in CreateUserInvite/AcceptUserInvite flow
sed -n '1550,1650p' management/server/user.go | grep -i "email" -B 2 -A 2Repository: netbirdio/netbird
Length of output: 503
🏁 Script executed:
# Check if there are any unique constraints defined elsewhere in schema/migration
rg "CreateIndex.*Email|unique.*email" -i -B 2 -A 2Repository: netbirdio/netbird
Length of output: 43
Add case-insensitive unique constraint and normalize invite emails before persisting.
The code uses strings.EqualFold() for case-insensitive lookup in GetUserInviteByEmail(), but emails are stored without normalization. A concurrent request with a different case variant (e.g., "foo@EXAMPLE.COM" vs "foo@example.com") can bypass the in-memory duplicate check and create multiple invites for the same email. Add normalization at line 1477 and enforce a unique constraint on (account_id, lower(email)) in the database to guarantee single active invite per email address, regardless of case or concurrent requests.
🤖 Prompt for AI Agents
In `@management/server/user.go` around lines 1477 - 1544, The invite flow does
case-insensitive checks with strings.EqualFold but persists raw email, allowing
racey duplicates; normalize the email (e.g., lowercase and trim) on the incoming
invite before any lookups or creation (affecting the checks that call
GetAccountUsers, GetUserInviteByEmail and the UserInviteRecord fields used in
SaveUserInvite) and ensure the DB enforces a unique constraint on (account_id,
lower(email)) so concurrent/variant-case requests cannot create multiple
invites; also update any query implementations (GetUserInviteByEmail) to compare
lower(email) to the normalized value.
| const minPasswordLength = 8 | ||
|
|
||
| // validatePassword checks password strength requirements: | ||
| // - Minimum 8 characters | ||
| // - At least 1 digit | ||
| // - At least 1 uppercase letter | ||
| // - At least 1 special character | ||
| func validatePassword(password string) error { | ||
| if len(password) < minPasswordLength { | ||
| return errors.New("password must be at least 8 characters long") | ||
| } | ||
|
|
||
| var hasDigit, hasUpper, hasSpecial bool | ||
| for _, c := range password { | ||
| switch { | ||
| case unicode.IsDigit(c): | ||
| hasDigit = true | ||
| case unicode.IsUpper(c): | ||
| hasUpper = true | ||
| case !unicode.IsLetter(c) && !unicode.IsDigit(c): | ||
| hasSpecial = true | ||
| } | ||
| } | ||
|
|
||
| var missing []string | ||
| if !hasDigit { | ||
| missing = append(missing, "one digit") | ||
| } | ||
| if !hasUpper { | ||
| missing = append(missing, "one uppercase letter") | ||
| } | ||
| if !hasSpecial { | ||
| missing = append(missing, "one special character") | ||
| } | ||
|
|
||
| if len(missing) > 0 { | ||
| return errors.New("password must contain at least " + strings.Join(missing, ", ")) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go len(string) bytes vs unicode characters rune count
💡 Result:
In Go, a string is a read-only sequence of bytes (often UTF-8, but not required). So:
len(s)returns the number of bytes in the string, not “characters”. (go.dev)- A rune is a Unicode code point (
type rune = int32). (go.dev) - To count Unicode code points (runes) in a UTF-8 string, use
utf8.RuneCountInString(s)(orlen([]rune(s))). (documentation.help)
Example:
s := "A⌘" // 'A' = 1 byte, '⌘' = 3 bytes in UTF-8
fmt.Println(len(s)) // 4 bytes
fmt.Println(utf8.RuneCountInString(s)) // 2 runesImportant nuance: “what a user thinks of as one character” (a grapheme cluster, e.g., many emojis or letters+combining marks) can be multiple runes, so rune count still may not match “visible characters.” (go.dev)
Citations:
- 1: https://go.dev/blog/strings?utm_source=openai
- 2: https://go.dev/blog/strings?utm_source=openai
- 3: https://documentation.help/Golang/unicode_utf8.htm?utm_source=openai
- 4: https://go.dev/blog/strings?utm_source=openai
Count runes, not bytes, for the 8‑character password policy.
Line 1787 uses len(password), which counts bytes rather than Unicode characters. Passwords with multi-byte UTF-8 characters (e.g., accented letters, symbols) can pass with fewer than 8 actual characters. Use utf8.RuneCountInString() to enforce the policy correctly.
🛠️ Proposed fix
import (
"context"
"errors"
"fmt"
"strings"
"time"
"unicode"
+ "unicode/utf8"
)
@@
- if len(password) < minPasswordLength {
+ if utf8.RuneCountInString(password) < minPasswordLength {
return errors.New("password must be at least 8 characters long")
}🤖 Prompt for AI Agents
In `@management/server/user.go` around lines 1779 - 1819, The password length
check in validatePassword uses len(password) which counts bytes, not Unicode
runes; replace that check to use utf8.RuneCountInString(password) against
minPasswordLength so multi-byte characters are counted correctly (update the
conditional in validatePassword and import "unicode/utf8" if not already
present).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@management/server/user_invite_test.go`:
- Around line 108-114: Tests reference a non-existent field InviteLink on the
UserInvite result; update all occurrences in
management/server/user_invite_test.go to use InviteToken to match the struct
definition (UserInvite.InviteToken in management/server/types/user_invite.go).
Replace each assertion and any other access of result.InviteLink with
result.InviteToken (e.g., the assertions that check presence and expiry:
result.InviteToken and result.InviteExpiresAt) so the tests compile against the
actual fields.
In `@shared/management/http/api/openapi.yml`:
- Around line 3525-3632: Add a DELETE operation for the existing endpoint
/api/users/invites/{inviteId} to the OpenAPI spec: declare the path parameter
inviteId (in: path, required, type: string), include the same security blocks
(BearerAuth and TokenAuth) and tags as the other invites endpoints, and mirror
response status codes used elsewhere (e.g., 204 or 200 for success, 401
requires_authentication, 403 forbidden, 404 invite not found, 412 precondition
failed - embedded IdP not enabled, 500 internal_error and any validation/409
responses if the handler uses them) so the spec matches the implemented handler
and tests.
♻️ Duplicate comments (3)
management/server/http/handlers/users/invites_handler.go (1)
113-217: Unify invalid-JSON responses between create and regenerate.
createInvitereturns 400 whileregenerateInvitemaps InvalidArgument to 422. Pick one status and apply consistently across both endpoints (and update tests accordingly).🔧 Example (align to InvalidArgument/422)
- if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - util.WriteErrorResponse("couldn't parse JSON request", http.StatusBadRequest, w) - return - } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "couldn't parse JSON request"), w) + return + }management/server/user.go (2)
1477-1486: Normalize email addresses before storage to prevent case-variant duplicates.The code uses
strings.EqualFoldfor case-insensitive comparison but stores the raw email. Concurrent requests with different case variants (e.g., "User@Example.com" vs "user@example.com") could bypass the duplicate check. Normalize the email (lowercase + trim) before any lookups or storage.
1787-1789: Password length check counts bytes instead of Unicode characters.
len(password)returns bytes, not runes. Passwords with multi-byte UTF-8 characters (e.g., accented letters, symbols) could pass validation with fewer than 8 actual characters. Useutf8.RuneCountInString()for correct character counting.
🧹 Nitpick comments (3)
management/server/types/user_invite.go (1)
79-105: Consider constant-time comparison for checksum verification.While the token lookup uses the hashed token (which provides timing-safe comparison at the DB level), the checksum validation in
ValidateInviteTokenuses a direct!=comparison. This is acceptable since this function only validates format/integrity before the hash lookup, but documenting this design choice would be helpful.management/server/user.go (2)
1499-1503: Consider enforcing a minimum expiration time.Per prior discussion, a minimum expiration (e.g., 1 hour) would prevent accidentally creating very short-lived invites. Currently, any positive value is accepted.
Suggested validation
+ const minExpiresIn = 3600 // 1 hour minimum // Calculate expiration time if expiresIn <= 0 { expiresIn = types.DefaultInviteExpirationSeconds + } else if expiresIn < minExpiresIn { + return nil, status.Errorf(status.InvalidArgument, "expiration time must be at least 1 hour") }
1526-1528: Consider wrapping SaveUserInvite in a transaction for consistency.
CreateUserInvitesaves the invite record without a transaction, whileRegenerateUserInviteusesExecuteInTransaction. For consistency and to guard against partial failures in future modifications, consider using a transaction here as well.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/store/sql_store_user_invite_test.go`:
- Around line 79-83: The test currently verifies only HashedToken after calling
store.GetUserInviteByID; add an assertion to verify the ExpiresAt update as well
by comparing retrieved.ExpiresAt to the expected expiration value (the value you
set when updating the invite). Locate the retrieval in the test
(GetUserInviteByID call and variable retrieved) and add a require/ assert (e.g.,
require.Equal or assert.Equal) that retrieved.ExpiresAt matches the updated
ExpiresAt used in the update step.
🧹 Nitpick comments (1)
management/server/store/sql_store_user_invite_test.go (1)
37-45: Consider verifyingHashedTokenpersistence.The test verifies most fields but omits
HashedToken. While other tests implicitly cover this, adding an assertion here would make this test more complete.assert.Equal(t, invite.AutoGroups, retrieved.AutoGroups) assert.Equal(t, invite.CreatedBy, retrieved.CreatedBy) + assert.Equal(t, invite.HashedToken, retrieved.HashedToken)
| // Verify the update | ||
| retrieved, err := store.GetUserInviteByID(ctx, LockingStrengthNone, invite.AccountID, invite.ID) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "new-hashed-token", retrieved.HashedToken) | ||
| }) |
There was a problem hiding this comment.
Add assertion for ExpiresAt update.
The test updates both HashedToken and ExpiresAt (line 74), but only verifies HashedToken. This leaves the expiration update unverified.
// Verify the update
retrieved, err := store.GetUserInviteByID(ctx, LockingStrengthNone, invite.AccountID, invite.ID)
require.NoError(t, err)
assert.Equal(t, "new-hashed-token", retrieved.HashedToken)
+ assert.WithinDuration(t, invite.ExpiresAt, retrieved.ExpiresAt, time.Second)🤖 Prompt for AI Agents
In `@management/server/store/sql_store_user_invite_test.go` around lines 79 - 83,
The test currently verifies only HashedToken after calling
store.GetUserInviteByID; add an assertion to verify the ExpiresAt update as well
by comparing retrieved.ExpiresAt to the expected expiration value (the value you
set when updating the invite). Locate the retrieval in the test
(GetUserInviteByID call and variable retrieved) and add a require/ assert (e.g.,
require.Equal or assert.Equal) that retrieved.ExpiresAt matches the updated
ExpiresAt used in the update step.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shared/management/http/api/openapi.yml`:
- Around line 3664-3727: Add a 429 "Too Many Requests" response to both public
invite endpoints: the GET operation for /api/users/invites/{token} (returns
UserInviteInfo) and the POST operation for /api/users/invites/{token}/accept
(accept invite, returns UserInviteAcceptResponse). Update each operation's
responses block to include a '429' entry—either referencing an existing reusable
response (e.g., components/responses/too_many_requests) or adding a short
description ("Too Many Requests" or rate limit hit) with empty content—so the
OpenAPI spec documents the rate‑limiting behavior implemented in the invite
handlers.
🧹 Nitpick comments (1)
management/server/http/handlers/users/invites_handler.go (1)
105-139: Avoid overwriting persisted invite creation timestamps.Line 137 reassigns
InviteCreatedAtafter the manager has already created the record. This can cause the response to drift from the stored timestamp (especially if the manager uses DB time). Consider only defaulting when it’s zero.♻️ Suggested adjustment
- result.InviteCreatedAt = time.Now().UTC() + if result.InviteCreatedAt.IsZero() { + result.InviteCreatedAt = time.Now().UTC() + }
| get: | ||
| summary: Get invite information | ||
| description: Retrieves public information about an invite. This endpoint is unauthenticated and protected by the token itself. | ||
| tags: [ Users ] | ||
| security: [] | ||
| parameters: | ||
| - in: path | ||
| name: token | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: The invite token | ||
| responses: | ||
| '200': | ||
| description: Invite information | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/UserInviteInfo' | ||
| '400': | ||
| "$ref": "#/components/responses/bad_request" | ||
| '404': | ||
| description: Invite not found or invalid token | ||
| content: { } | ||
| '500': | ||
| "$ref": "#/components/responses/internal_error" | ||
| /api/users/invites/{token}/accept: | ||
| post: | ||
| summary: Accept an invite | ||
| description: Accepts an invite and creates the user with the provided password. This endpoint is unauthenticated and protected by the token itself. | ||
| tags: [ Users ] | ||
| security: [] | ||
| parameters: | ||
| - in: path | ||
| name: token | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: The invite token | ||
| requestBody: | ||
| description: Password to set for the new user | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/UserInviteAcceptRequest' | ||
| responses: | ||
| '200': | ||
| description: Invite accepted successfully | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/UserInviteAcceptResponse' | ||
| '400': | ||
| "$ref": "#/components/responses/bad_request" | ||
| '404': | ||
| description: Invite not found or invalid token | ||
| content: { } | ||
| '412': | ||
| description: Precondition failed - embedded IdP is not enabled or invite expired | ||
| content: { } | ||
| '422': | ||
| "$ref": "#/components/responses/validation_failed" | ||
| '500': |
There was a problem hiding this comment.
Document 429 responses for public invite endpoints.
These routes are rate‑limited, so clients can receive 429s that aren’t described in the spec. Add a 429 response to both public endpoints.
🧾 Proposed OpenAPI update
/api/users/invites/{token}:
get:
responses:
'200':
description: Invite information
content:
application/json:
schema:
$ref: '#/components/schemas/UserInviteInfo'
+ '429':
+ description: Too many requests (rate limited)
+ content: { }
'400':
"$ref": "#/components/responses/bad_request"
...
/api/users/invites/{token}/accept:
post:
responses:
'200':
description: Invite accepted successfully
content:
application/json:
schema:
$ref: '#/components/schemas/UserInviteAcceptResponse'
+ '429':
+ description: Too many requests (rate limited)
+ content: { }
'400':
"$ref": "#/components/responses/bad_request"Based on learnings, these endpoints are rate‑limited in the handler.
📝 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.
| get: | |
| summary: Get invite information | |
| description: Retrieves public information about an invite. This endpoint is unauthenticated and protected by the token itself. | |
| tags: [ Users ] | |
| security: [] | |
| parameters: | |
| - in: path | |
| name: token | |
| required: true | |
| schema: | |
| type: string | |
| description: The invite token | |
| responses: | |
| '200': | |
| description: Invite information | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteInfo' | |
| '400': | |
| "$ref": "#/components/responses/bad_request" | |
| '404': | |
| description: Invite not found or invalid token | |
| content: { } | |
| '500': | |
| "$ref": "#/components/responses/internal_error" | |
| /api/users/invites/{token}/accept: | |
| post: | |
| summary: Accept an invite | |
| description: Accepts an invite and creates the user with the provided password. This endpoint is unauthenticated and protected by the token itself. | |
| tags: [ Users ] | |
| security: [] | |
| parameters: | |
| - in: path | |
| name: token | |
| required: true | |
| schema: | |
| type: string | |
| description: The invite token | |
| requestBody: | |
| description: Password to set for the new user | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteAcceptRequest' | |
| responses: | |
| '200': | |
| description: Invite accepted successfully | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteAcceptResponse' | |
| '400': | |
| "$ref": "#/components/responses/bad_request" | |
| '404': | |
| description: Invite not found or invalid token | |
| content: { } | |
| '412': | |
| description: Precondition failed - embedded IdP is not enabled or invite expired | |
| content: { } | |
| '422': | |
| "$ref": "#/components/responses/validation_failed" | |
| '500': | |
| get: | |
| summary: Get invite information | |
| description: Retrieves public information about an invite. This endpoint is unauthenticated and protected by the token itself. | |
| tags: [ Users ] | |
| security: [] | |
| parameters: | |
| - in: path | |
| name: token | |
| required: true | |
| schema: | |
| type: string | |
| description: The invite token | |
| responses: | |
| '200': | |
| description: Invite information | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteInfo' | |
| '429': | |
| description: Too many requests (rate limited) | |
| content: { } | |
| '400': | |
| "$ref": "#/components/responses/bad_request" | |
| '404': | |
| description: Invite not found or invalid token | |
| content: { } | |
| '500': | |
| "$ref": "#/components/responses/internal_error" | |
| /api/users/invites/{token}/accept: | |
| post: | |
| summary: Accept an invite | |
| description: Accepts an invite and creates the user with the provided password. This endpoint is unauthenticated and protected by the token itself. | |
| tags: [ Users ] | |
| security: [] | |
| parameters: | |
| - in: path | |
| name: token | |
| required: true | |
| schema: | |
| type: string | |
| description: The invite token | |
| requestBody: | |
| description: Password to set for the new user | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteAcceptRequest' | |
| responses: | |
| '200': | |
| description: Invite accepted successfully | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/UserInviteAcceptResponse' | |
| '429': | |
| description: Too many requests (rate limited) | |
| content: { } | |
| '400': | |
| "$ref": "#/components/responses/bad_request" | |
| '404': | |
| description: Invite not found or invalid token | |
| content: { } | |
| '412': | |
| description: Precondition failed - embedded IdP is not enabled or invite expired | |
| content: { } | |
| '422': | |
| "$ref": "#/components/responses/validation_failed" | |
| '500': |
🤖 Prompt for AI Agents
In `@shared/management/http/api/openapi.yml` around lines 3664 - 3727, Add a 429
"Too Many Requests" response to both public invite endpoints: the GET operation
for /api/users/invites/{token} (returns UserInviteInfo) and the POST operation
for /api/users/invites/{token}/accept (accept invite, returns
UserInviteAcceptResponse). Update each operation's responses block to include a
'429' entry—either referencing an existing reusable response (e.g.,
components/responses/too_many_requests) or adding a short description ("Too Many
Requests" or rate limit hit) with empty content—so the OpenAPI spec documents
the rate‑limiting behavior implemented in the invite handlers.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/handlers/users/invites_handler.go`:
- Around line 195-198: The handler currently rejects any non-POST method with
util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w),
which causes registered OPTIONS preflight requests to receive 405; update the
method check to explicitly handle OPTIONS (check r.Method == http.MethodOptions)
by returning 200/204 and setting the appropriate Allow/CORS headers (e.g., set
"Allow: POST, OPTIONS" and any needed CORS headers) before returning, or remove
OPTIONS from the route if you don't want to support it; locate the method check
in invites_handler.go where r.Method is compared and modify it to handle
http.MethodOptions explicitly (and keep the existing util.WriteErrorResponse for
other non-POST methods).
🧹 Nitpick comments (2)
management/server/http/handlers/users/invites_handler.go (2)
21-27: Consider making public invite rate limits configurable.Hardcoding these thresholds makes tuning harder in different deployments; exposing them via config would allow safer adjustments without code changes.
137-138: Avoid clobbering the manager‑provided created timestamp.If the store sets
InviteCreatedAt, overwriting it here can mask the source-of-truth value. Only set it when it’s zero.♻️ Proposed fix
- result.InviteCreatedAt = time.Now().UTC() + if result.InviteCreatedAt.IsZero() { + result.InviteCreatedAt = time.Now().UTC() + }
| if r.Method != http.MethodPost { | ||
| util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) | ||
| return | ||
| } |
There was a problem hiding this comment.
OPTIONS is registered but always rejected here.
This handler returns 405 for any non‑POST request, so registered OPTIONS requests will fail. Either handle OPTIONS explicitly or remove it from the route.
🐛 Suggested fix (handle OPTIONS explicitly)
- if r.Method != http.MethodPost {
+ if r.Method == http.MethodOptions {
+ w.WriteHeader(http.StatusOK)
+ return
+ }
+ if r.Method != http.MethodPost {
util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w)
return
}🤖 Prompt for AI Agents
In `@management/server/http/handlers/users/invites_handler.go` around lines 195 -
198, The handler currently rejects any non-POST method with
util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w),
which causes registered OPTIONS preflight requests to receive 405; update the
method check to explicitly handle OPTIONS (check r.Method == http.MethodOptions)
by returning 200/204 and setting the appropriate Allow/CORS headers (e.g., set
"Allow: POST, OPTIONS" and any needed CORS headers) before returning, or remove
OPTIONS from the route if you don't want to support it; locate the method check
in invites_handler.go where r.Method is compared and modify it to handle
http.MethodOptions explicitly (and keep the existing util.WriteErrorResponse for
other non-POST methods).
2 participants
Describe your changes
Implement an alternative user invite flow where admins can create invite links that users use to set their own passwords, instead of having passwords generated and shared by admins.
New API endpoints:
Key features:
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Security & Storage
Documentation
Activity
Tests
✏️ Tip: You can customize this high-level summary in your review settings.