server/channels/store/sqlstore/user_access_token_store.go (2)

238-257: Consider selecting only needed columns instead of stripping Token post-fetch.

The implementation fetches the full row (including the secret Token) then blanks it in memory at lines 252-254. While functional, the secret traverses the network from DB to application unnecessarily.

A more secure approach would use a dedicated select without the Token column:

Suggested optimization

 func (s SqlUserAccessTokenStore) GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) {
 	tokens := []*model.UserAccessToken{}

-	query := s.userAccessTokensSelectQuery.
+	query := s.getQueryBuilder().
+		Select(
+			"UserAccessTokens.Id",
+			"UserAccessTokens.UserId",
+			"UserAccessTokens.Description",
+			"UserAccessTokens.IsActive",
+			"UserAccessTokens.ExpiresAt",
+		).
+		From("UserAccessTokens").
 		Where(sq.Gt{"UserAccessTokens.ExpiresAt": 0}).
 		Where(sq.LtOrEq{"UserAccessTokens.ExpiresAt": cutoff}).
 		Where(sq.Eq{"UserAccessTokens.IsActive": true}).
 		OrderBy("UserAccessTokens.ExpiresAt ASC").
 		Limit(uint64(limit))

 	if err := s.GetReplica().SelectBuilder(&tokens, query); err != nil {
 		return nil, errors.Wrap(err, "failed to find expired UserAccessTokens")
 	}

-	for _, token := range tokens {
-		token.Token = ""
-	}
-
 	return tokens, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/channels/store/sqlstore/user_access_token_store.go` around lines 238 -
257, GetExpiredBefore currently fetches full UserAccessTokens rows (via
userAccessTokensSelectQuery) then clears the Token field in memory; instead
modify the query built in GetExpiredBefore to explicitly select only the needed
columns (omitting the Token column) so the secret never travels from DB to app,
e.g., adjust the select builder used by userAccessTokensSelectQuery or create a
new select that excludes "UserAccessTokens.Token" before applying the
Where/OrderBy/Limit and then return the tokens as-is; keep the existing
filtering logic and still set the Token field blank if needed for compatibility.

---

279-282: Silent error swallowing for RowsAffected.

If RowsAffected() fails, the error is silently ignored and deleted defaults to 0. While this doesn't affect the deletion itself, the caller's log message will show 0 deletions even if tokens were deleted.

Consider logging the error for debugging purposes:

Suggested improvement

 	deleted, rErr := res.RowsAffected()
 	if rErr != nil {
+		// Log but don't fail - the deletion succeeded, we just can't report the count
 		deleted = 0
 	}

Or return the error if accurate counts are important for audit/metrics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/channels/store/sqlstore/user_access_token_store.go` around lines 279 -
282, The code swallows the error from res.RowsAffected() causing the caller to
see a misleading deleted=0; update the block around res.RowsAffected() (the call
to RowsAffected and the deleted variable in user_access_token_store.go) to
handle rErr explicitly: if rErr != nil then either log the error with context
(e.g., using the store's logger like s.logger or the existing logging facility)
including the SQL operation and any identifiers, or propagate/return rErr so
callers can distinguish an actual failure from zero rows; ensure the log message
references the operation (RowsAffected on user access token delete) and the err
value instead of silently defaulting deleted to 0.

server/channels/jobs/cleanup_expired_access_tokens/worker.go (1)

77-80: Unused job.Data initialization.

job.Data is initialized but never populated or used before the function returns. This appears to be dead code—consider removing it unless there's a planned use case.

Suggested removal

-	if job.Data == nil {
-		job.Data = make(model.StringMap)
-	}
 	return nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/channels/jobs/cleanup_expired_access_tokens/worker.go` around lines 77
- 80, The nil-check and initialization of job.Data (setting job.Data =
make(model.StringMap)) is dead code because job.Data is never used before the
function returns; remove the conditional block that checks if job.Data == nil
and the allocation (or alternatively, if a use was intended, populate job.Data
where the function processes job) so that the function no longer performs an
unused allocation (locate the code around the early return that sets job.Data
and delete that initialization).

Fix all unresolved CodeRabbit comments on this PR:

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

server/channels/store/storetest/user_access_token_store.go (1)

285-288: Harden cleanup to include the expired fixture too.

If the test exits early before Line 317, the expired token can remain and pollute later runs. Add it to t.Cleanup for deterministic isolation.

♻️ Proposed tweak

 	t.Cleanup(func() {
 		_ = ss.UserAccessToken().Delete(nonExpiring.Id)
+		_ = ss.UserAccessToken().Delete(expired.Id)
 		_ = ss.UserAccessToken().Delete(future.Id)
 	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/channels/store/storetest/user_access_token_store.go` around lines 285
- 288, The cleanup currently calls ss.UserAccessToken().Delete for nonExpiring
and future but misses the expired fixture; update the t.Cleanup closure to also
delete the expired token by calling ss.UserAccessToken().Delete(expired.Id) so
all three fixtures (nonExpiring, future, expired) are removed and tests remain
isolated even on early exit.

Fix all unresolved CodeRabbit comments on this PR:

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

server/channels/store/storetest/user_access_token_store.go (1)

307-315: Strengthen secret-leak assertion across all returned rows.

Currently, Token emptiness is asserted only for the matched expired token. As a tighter regression check, assert row.Token is empty for every row returned by GetExpiredBefore.

✅ Suggested test tightening

 	for _, row := range expiredRows {
+		require.Empty(t, row.Token, "expired token secret should not be returned")
 		if row.Id == expired.Id {
-			require.Empty(t, row.Token, "expired token secret should not be returned")
 			require.Equal(t, expired.ExpiresAt, row.ExpiresAt)
 			found = true
 		}
 		require.NotEqual(t, nonExpiring.Id, row.Id, "non-expiring token must not be returned")
 		require.NotEqual(t, future.Id, row.Id, "future token must not be returned")
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/channels/store/storetest/user_access_token_store.go` around lines 307
- 315, The test currently only asserts row.Token is empty for the matched
expired token; update the loop over expiredRows (the results of
GetExpiredBefore) to assert that every row returned has an empty Token field
(require.Empty(t, row.Token)) while still keeping the existing checks that the
matched expired row has the expected ExpiresAt and that nonExpiring.Id and
future.Id are not present; i.e., inside the for _, row := range expiredRows
loop, add a require.Empty(t, row.Token) at the top to enforce no secret leaks
for any returned row.

Fix all unresolved CodeRabbit comments on this PR:

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