-
Couldn't load subscription status.
- Fork 421
Enhance HumanReadableName validation
#4171
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.
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
Enhance HumanReadableName validation
#4171
Conversation
Add check that every label of user or domain is not longer than 63. It better alings with validation in `dnssec_prover::rr::Name`.
|
👋 I see @wpaulino was un-assigned. |
d68284c to
bb03a51
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4171 +/- ##
==========================================
+ Coverage 88.78% 88.82% +0.03%
==========================================
Files 180 180
Lines 137066 137123 +57
Branches 137066 137123 +57
==========================================
+ Hits 121694 121793 +99
+ Misses 12552 12519 -33
+ Partials 2820 2811 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
|
|
||
| impl HumanReadableName { | ||
| /// Constructs a new [`HumanReadableName`] from the `user` and `domain` parts. See the | ||
| /// struct-level documentation for more on the requirements on each. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you mention the new filtering in the struct docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
/// The `user` and `domain` parts combined cannot exceed 231 bytes in length;
/// each DNS label within them must be non-empty and no longer than 63 bytes.
bb03a51 to
255a16e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
| if domain.ends_with('.') { | ||
| domain = &domain[..domain.len() - 1]; | ||
| } | ||
| let domain = domain.strip_suffix(".").unwrap_or(domain); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: '.' and "." are different - '.' is the char . whereas "." is the string ".". We should be stripping the char, not the string, as it uses a more efficient algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that what I meant.
I made a followup to clean this and similar instances: #4174.
Add check that every label of user or domain is not longer than 63. It better alings with validation in
dnssec_prover::rr::Name.