Conversation
Server generated tokens have a high entropy due to being random cryptographically secure strings, they do not suffer from the same pitfalls which human-made low entropy passwords do. This makes using password-hashing API's on them which might get more expensive in the future overkill and unnecessary.
Also, 32 chars of case-sensitive alphanumeric has plenti of entropy
There was a problem hiding this comment.
Pull Request Overview
This PR refactors token and secret handling to avoid unnecessary hashing on high entropy, server‑generated strings. Key changes include renaming session-related methods to better indicate token usage, updating secret/secret property names to secretHash across the codebase, and switching to a unified hashing utility and token length defined in AuthConstants.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Common/Utils/AuthUtils.cs | Renamed TryGetUserSession to TryGetUserSessionToken for clarity |
| Common/Services/Session/SessionService.cs | Updated session creation and retrieval to use CreateSessionResult and consistent token handling |
| Common/Services/BatchUpdate/BatchUpdateService.cs | Refactored token update mechanism with the new token length and hashing logic |
| Common/OpenShockDb/* | Replaced Secret properties with SecretHash to align with the new security approach |
| API/Controller/* and Authentication Handlers | Updated methods to use token-based session and API token authentication |
| // Only hash new tokens, old ones are 64 chars long | ||
| if (sessionToken.Length == AuthConstants.GeneratedTokenLength) | ||
| { | ||
| sessionToken = HashingUtils.HashToken(sessionToken); | ||
| } |
There was a problem hiding this comment.
[nitpick] The conditional token hashing logic here is duplicated in other parts of the codebase. Consider extracting a shared utility method for normalizing tokens to ensure consistent behavior.
| // Only hash new tokens, old ones are 64 chars long | |
| if (sessionToken.Length == AuthConstants.GeneratedTokenLength) | |
| { | |
| sessionToken = HashingUtils.HashToken(sessionToken); | |
| } | |
| // Normalize the session token | |
| sessionToken = NormalizeToken(sessionToken); |
| { | ||
| _sessionLastUsed.Enqueue(sessionKey, lastUsed); | ||
| // Only hash new tokens, old ones are 64 chars long | ||
| if (sessionToken.Length == AuthConstants.GeneratedTokenLength) |
There was a problem hiding this comment.
[nitpick] Token normalization by checking the token length is repeated here; centralizing this logic in a utility method would reduce maintenance risk and ensure consistency.
Reason for changing secret hashing:
Server generated tokens have a high entropy due to being random cryptographically secure strings, they do not suffer from the same pitfalls which human-made low entropy passwords do.
This makes using password-hashing API's on them which might get more expensive in the future overkill and unnecessary.
see:
https://security.stackexchange.com/questions/232753/is-single-iteration-sha-256-safe-enough-for-hashing-128bit-random-number-api-key
https://stackoverflow.com/questions/76189626/which-hashing-algorithm-to-use-for-api-key