Skip to content

Batch User Model Loading Queries. 32% speedup #2361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
olirice wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Batch User Model Loading Queries. 32% speedup #2361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 63 additions & 3 deletions internal/models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"crypto/sha256"
"database/sql"
"encoding/base64"
"encoding/json"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -604,15 +605,74 @@ func CountOtherUsers(tx *storage.Connection, id uuid.UUID) (int, error) {
}

func findUser(tx *storage.Connection, query string, args ...interface{}) (*User, error) {
obj := &User{}
if err := tx.Eager().Q().Where(query, args...).First(obj); err != nil {
// Single query with JSON aggregation - embed by VALUE not pointer
type userWithJSON struct {
User
IdentitiesJSON []byte `db:"identities_json"`
FactorsJSON []byte `db:"factors_json"`
}

var result userWithJSON

sqlQuery := `
select
u.id, u.aud, u.role, u.email, u.is_sso_user,
u.encrypted_password, u.email_confirmed_at, u.invited_at,
u.phone, u.phone_confirmed_at,
u.confirmation_token, u.confirmation_sent_at, u.confirmed_at,
u.recovery_token, u.recovery_sent_at,
u.email_change_token_current, u.email_change_token_new, u.email_change,
u.email_change_sent_at, u.email_change_confirm_status,
u.phone_change_token, u.phone_change, u.phone_change_sent_at,
u.reauthentication_token, u.reauthentication_sent_at,
u.last_sign_in_at,
u.raw_app_meta_data, u.raw_user_meta_data,
u.created_at, u.updated_at, u.banned_until, u.deleted_at, u.is_anonymous,
u.instance_id,
coalesce((select json_agg(json_build_object(
'identity_id', i.id,
'id', i.provider_id,
'user_id', i.user_id,
'identity_data', i.identity_data,
'provider', i.provider,
'last_sign_in_at', i.last_sign_in_at,
'created_at', i.created_at,
'updated_at', i.updated_at,
'email', i.email
)) from ` + Identity{}.TableName() + ` i where i.user_id = u.id), '[]') as identities_json,
coalesce((select json_agg(json_build_object(
'id', f.id,
'user_id', f.user_id,
'created_at', f.created_at,
'updated_at', f.updated_at,
'status', f.status,
'friendly_name', f.friendly_name,
'factor_type', f.factor_type,
'secret', f.secret,
Comment on lines +644 to +651
Copy link
Contributor

@depthfirst-app depthfirst-app bot Jan 31, 2026

Choose a reason for hiding this comment

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

🔴 Severity: CRITICAL

MFA Secret Exposure - The TOTP secret field is included in JSON aggregation, bypassing the json:"-" protection in the Factor struct. This exposes the shared secret used to generate TOTP codes. The original Eager loading respected this tag and excluded secrets. Remove this line and web_authn_credential (line 654) to prevent exposure in API responses, logs, or caches.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Remove the 'secret' field (line 651) and 'web_authn_credential' field (line 654) from the JSON aggregation query. These fields are marked with json:"-" tags in the Factor struct to prevent exposure in API responses. Including them in the SQL JSON aggregation bypasses this protection and exposes sensitive MFA secrets that could be logged, cached, or transmitted in API responses. The corrected query should only include non-sensitive Factor fields that are safe to serialize.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
'id', f.id,
'user_id', f.user_id,
'created_at', f.created_at,
'updated_at', f.updated_at,
'status', f.status,
'friendly_name', f.friendly_name,
'factor_type', f.factor_type,
'secret', f.secret,
'id', f.id,
'user_id', f.user_id,
'created_at', f.created_at,
'updated_at', f.updated_at,
'status', f.status,
'friendly_name', f.friendly_name,
'factor_type', f.factor_type,
'phone', f.phone,
'last_challenged_at', f.last_challenged_at
)) from ` + Factor{}.TableName() + ` f where f.user_id = u.id), '[]') as factors_json

Copy link
Contributor Author

@olirice olirice Jan 31, 2026

Choose a reason for hiding this comment

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

this was already being loaded when we used *. Its not a change in behavior. I'm not clear on the implications of removing it but feel free

'phone', f.phone,
'last_challenged_at', f.last_challenged_at,
'web_authn_credential', f.web_authn_credential
)) from ` + Factor{}.TableName() + ` f where f.user_id = u.id), '[]') as factors_json
from ` + User{}.TableName() + ` u
where ` + query
Copy link
Contributor

@depthfirst-app depthfirst-app bot Jan 31, 2026

Choose a reason for hiding this comment

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

🟠 Severity: HIGH

SQL Injection Risk via String Concatenation: The query parameter is concatenated directly into the SQL string before being passed to RawQuery. While current callers use hardcoded strings, this pattern is dangerous because it breaks the parameterization contract. The old code used tx.Eager().Q().Where(query, args...) which properly handled parameterization through the ORM. Future developers might pass dynamically constructed queries, creating SQL injection vectors.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: This SQL injection risk requires an architectural change to the findUser function. Consider one of these approaches:

  1. Use parameterized WHERE clause builder: Instead of accepting a raw query string, accept structured parameters (e.g., field names and operators) and build the WHERE clause programmatically with proper escaping.

  2. Implement query validation/whitelisting: Add a whitelist of allowed query patterns at the start of findUser to ensure only safe, hardcoded queries are accepted. Reject any query that doesn't match the whitelist.

  3. Revert to ORM-based approach: Consider reverting to the original tx.Eager().Q().Where(query, args...) pattern which properly handles parameterization through the ORM, accepting the performance trade-off for better security.

  4. Create specific finder methods: Instead of a generic findUser helper, create specific methods (findUserByEmail, findUserByPhone, etc.) that construct their own safe SQL queries, eliminating the need to pass query strings as parameters.

The current implementation breaks the parameterization contract by concatenating user-provided strings directly into SQL, even though current callers use hardcoded strings. This creates a dangerous pattern that future developers might misuse.

Copy link
Contributor Author

@olirice olirice Jan 31, 2026

Choose a reason for hiding this comment

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

yeah, i don't love passing raw sql around, but that seems to be the pattern. any suggestions would be great. This could easily be a pair of uuids instead but I don't want to change the functions interface in this PR


if err := tx.RawQuery(sqlQuery, args...).First(&result); err != nil {
if errors.Cause(err) == sql.ErrNoRows {
return nil, UserNotFoundError{}
}
return nil, errors.Wrap(err, "error finding user")
}

return obj, nil
// Deserialize identities and factors from JSON
if err := json.Unmarshal(result.IdentitiesJSON, &result.User.Identities); err != nil {
return nil, errors.Wrap(err, "error unmarshaling identities")
}

if err := json.Unmarshal(result.FactorsJSON, &result.User.Factors); err != nil {
return nil, errors.Wrap(err, "error unmarshaling factors")
}

return &result.User, nil
}

// FindUserByEmailAndAudience finds a user with the matching email and audience.
Expand Down