refactor: consolidate duplicate random string generators#104
refactor: consolidate duplicate random string generators#104ManthanSiroya wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new ChangesRandom String Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@internal/service/auth_service.go`:
- Around line 121-124: The error returned from GenerateRandomString calls at
lines 121-124, 420-423, and 553-556 are being returned directly to handlers,
which exposes raw RNG/OS failures to clients through ErrorResponse. Instead of
returning the error directly from GenerateRandomString, wrap it with a custom
service-level error (such as ErrTokenGenerationFailed) at all three locations.
This ensures handlers receive stable, sanitized error messages that do not
expose internal implementation details to clients.
In `@internal/utils/random.go`:
- Around line 9-16: The GenerateRandomString function does not validate the
length parameter before passing it to make(), which will panic if a negative
value is provided instead of returning an error as expected from an exported
function. Add input validation at the start of GenerateRandomString to check
that length is a valid positive integer, and return an appropriate error if the
validation fails before attempting to allocate the byte slice.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e36fb0f4-20a9-4a19-80d1-5c0aa6a2d92e
📒 Files selected for processing (5)
internal/service/auth_service.gointernal/service/oauth_provider_service.gointernal/service/token_helper.gointernal/service/token_service_test.gointernal/utils/random.go
|
@ManthanSiroya Password reset tokens, email verification tokens, and the temp OAuth password used to be 44 characters (base64 of 32 random bytes, untruncated). Now they're 32 chars because the new shared utils.GenerateRandomString truncates the base64 output to length. If there's a VARCHAR(44) column or anything else downstream that assumes the old length, this could cause issues. Can you double check the DB schema ? The truncation itself reduces entropy a bit more than intended. In utils.GenerateRandomString, we generate length random bytes, base64-encode them (~4/3 * length chars), then cut it down to length chars. So a "32-character" token actually only has the randomness of ~24 bytes (192 bits), not 32 bytes (256 bits). since this function is now used everywhere (passwords, OAuth secrets, auth codes), might be worth generating enough bytes upfront so the final string actually has length chars worth of full entropy. |
|
|
You're right. The shared utility unintentionally changed token behavior by truncating the base64 output, which silently reduced token length and entropy. I'll update the implementation to preserve the original behavior (44-char output for 32 random bytes) so downstream assumptions and randomness characteristics remain unchanged. |
|
@ManthanSiroya Centralizing our random string generation and adding defensive error handling to our cryptographic operations is a great step forward for cleaning up technical debt! However, we have a blocker regarding how entropy and string limits are handled. The new utility mixes up byte length with character length. This causes our core authentication tokens (resets, registration verifications) to silently shrink from 44 down to 32 characters, which will conflict with database requirements and validation layers. Furthermore, truncating strings down this way strips out real cryptographic entropy. |
|
Hi @roshankumar0036singh, Good catch — you were right about the byte-length vs character-length issue. I’ve already updated the shared utility to preserve the original behavior (no truncation), so token length/entropy remain unchanged and existing downstream expectations (44-char tokens for 32 random bytes) are preserved. |
|
@ManthanSiroya did you commit the push ? |
|
Hi @roshankumar0036singh ,I Addressed the random string generation concern and pushed the fix. Updated the shared |
|
@ManthanSiroya i cant see your updated commit ? |
|
@ManthanSiroya Thanks for the update, but changing the test to expect 44 characters instead of 32 just hides the symptom. The length parameter must dictate the final character length of the string returned, not the input bytes before Base64 expansion. If returned strings are longer than our database column sizes, production writes will crash. Please add encoded[:length] slicing to utils.GenerateRandomString to guarantee strict character counts, and update the test to assert for exactly 32 |
|
Hi @roshankumar0036singh ,I will do this and the another PR issue by tommorrow as I have to go now. |
|
I wanted to share an update regarding this PR. I’ve spent 5–6+ hours working through the refactor, review feedback, testing, and follow-up fixes around the shared random string utility and related behavior changes. At this point, I’m unable to continue working on the remaining review items for this PR. I’ve already addressed the earlier requested fixes and pushed my latest changes. If the completed work is acceptable, I’d appreciate consideration for merge and recognition of the effort already contributed. For the remaining requested improvements/comments, I kindly request creating separate follow-up issues so they can be handled independently later. Thanks for the detailed reviews and guidance throughout the process. |
2 participants
Summary
This PR consolidates duplicate random string generation logic into a single shared utility function to reduce duplication and improve consistency.
Previously, the repository had two separate implementations:
TokenService.GenerateRandomString()intoken_helper.go(ignored errors)generateRandomString()inoauth_provider_service.go(returned errors)This caused inconsistent behavior and increased maintenance overhead.
Changes Made
Added shared utility
GenerateRandomString()function in theutilspackage for centralized random string generation.Removed duplicate implementations
Removed duplicate random string generation logic from:
token_helper.gooauth_provider_service.goUpdated callers
Test updates
Benefits
Fixes #66
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit
Release Notes