fix(md5crypt): apply the default iterations for the sun variant - #353
Conversation
The documented default of 34000 iterations was never applied. The only place it was set compared the uint32 iterations against an IterationsMin of 0, which can never be true, so the Sun variant always defaulted to 0 additional rounds. The hasher now tracks whether iterations were explicitly configured and applies IterationsDefault when they were not, following the same approach as sha1crypt. An explicit WithIterations(0) is still honoured. The unreachable check in Digest.defaults is removed, and the WithIterations documentation now reflects the rounds parameter and the actual maximum. Digests produced by the Sun variant with default options will now include rounds=34000. Existing digests are unaffected.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughMD5crypt now preserves explicitly configured iteration counts, including zero, and applies the default count only when iterations were not set. Regression tests cover Sun and standard variant encoding and password verification. ChangesMD5crypt iteration handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Omitted Sun iterations now use the 34,000 rounds setting, while explicit zero and standard-variant behavior are preserved. The available repository evidence shows no concrete merge-blocking regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 82.28% 82.77% +0.48%
==========================================
Files 49 49
Lines 1716 1724 +8
==========================================
+ Hits 1412 1427 +15
+ Misses 304 297 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
|
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
1 participant
The documented default of 34000 iterations was never applied. The only place it was set compared the uint32 iterations against an IterationsMin of 0, which can never be true, so the Sun variant always defaulted to 0 additional rounds.
The hasher now tracks whether iterations were explicitly configured and applies IterationsDefault when they were not, following the same approach as sha1crypt. An explicit WithIterations(0) is still honoured. The unreachable check in Digest.defaults is removed, and the WithIterations documentation now reflects the rounds parameter and the actual maximum.
Digests produced by the Sun variant with default options will now include rounds=34000. Existing digests are unaffected.
Summary by CodeRabbit