PM-27771: Improve TOTP parsing #6119
Conversation
david-livefront
requested review from
a team and
brian-livefront
as code owners
|
Claude finished @david-livefront's task —— View job Code Review Complete ✅SummaryThis PR significantly improves TOTP parsing logic with better validation, clearer architecture, and enhanced edge case handling. The refactoring moves TOTP utilities to a more appropriate location in the Key Improvements 👍
Issues Identified🟡 Medium Priority
📝 Minor Issues
🌱 Future Improvements
Security Considerations ✓
Test Coverage 📊Codecov shows 80% patch coverage with 9 lines missing coverage. The missing coverage appears to be in edge case branches. Consider adding the suggested test cases above to improve coverage. Architecture Compliance ✓
RecommendationApprove with minor suggestions. The PR delivers substantial improvements to TOTP parsing robustness and code quality. The identified issues are minor and can be addressed in follow-up improvements if desired. The one question to clarify is whether RFC 4648-strict padding validation is needed for Base32 strings. Todo List
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ui/src/main/kotlin/com/bitwarden/ui/platform/util/TotpUriUtils.kt
Outdated
Show resolved
Hide resolved
ui/src/main/kotlin/com/bitwarden/ui/platform/util/TotpUriUtils.kt
Outdated
Show resolved
Hide resolved
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "1234567890qwertyuiop" |
There was a problem hiding this comment.
🎨 Test improvement opportunity: The test secret "1234567890qwertyuiop" contains invalid Base32 characters (0, 1, 8, 9, and lowercase vowels). Consider using a more explicit test case that clearly shows Base32 validation:
// More explicit examples:
// Invalid: contains '0', '1', '8', '9'
every { getQueryParameter("secret") } returns "0189"
// Or invalid: contains lowercase vowels outside Base32
every { getQueryParameter("secret") } returns "invalid!"This makes the test intent clearer: we're validating Base32 alphabet, not just rejecting arbitrary strings.
| fun `getTotpDataOrNull with invalid secret returns null`() { | ||
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "1234567890qwertyuiop" | ||
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Missing test coverage: Consider adding a test case for an empty secret string after trimming (e.g., " "). This would verify the behavior when getQueryParameter("secret") returns whitespace-only strings.
@Test
fun `getTotpDataOrNull with whitespace-only secret returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns " "
}
assertNull(uri.getTotpDataOrNull())
}
| fun `getTotpDataOrNull with invalid digits returns null`() { | ||
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "secret" | ||
| every { getQueryParameter("digits") } returns "55" | ||
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Test edge cases: Good coverage for out-of-range digits (55 > 10). Consider also testing the lower boundary (0 or negative) to ensure comprehensive validation:
@Test
fun `getTotpDataOrNull with zero digits returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "secret"
every { getQueryParameter("digits") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}
| fun `getTotpDataOrNull with invalid period returns null`() { | ||
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "secret" | ||
| every { getQueryParameter("digits") } returns "5" | ||
| every { getQueryParameter("period") } returns "-1" | ||
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Test edge cases: Consider also testing period = 0 explicitly, since the validation is period < 1:
@Test
fun `getTotpDataOrNull with zero period returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "secret"
every { getQueryParameter("digits") } returns "6"
every { getQueryParameter("period") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}
| return if (digits == null) { | ||
| // If no value was provided, then we'll default to 6. | ||
| 6 | ||
| } else if (digits < 1 || digits > 10) { |
There was a problem hiding this comment.
ℹ️ Magic number rationale: The upper bound of 10 digits is well-chosen. According to RFC 6238 (TOTP specification), most implementations support 6-8 digits, and 10 is a reasonable maximum. This validation will prevent potential issues with excessive digit counts while allowing flexibility.
| private fun Uri.getAlgorithm(): TotpData.CryptoHashAlgorithm? { | ||
| val algorithm = this | ||
| .getQueryParameter(PARAM_NAME_ALGORITHM) | ||
| ?.trim() | ||
| ?.lowercase() | ||
| return if (algorithm == null) { | ||
| // If no value was provided, then we'll default to SHA_1. | ||
| TotpData.CryptoHashAlgorithm.SHA_1 | ||
| } else { | ||
| // If the value is unidentifiable, then it's invalid. | ||
| // If it's identifiable, then we return the valid value. | ||
| // We specifically do not use a `let` here, since we do not want to map an unidentified | ||
| // value to the default value. | ||
| TotpData.CryptoHashAlgorithm.parse(value = algorithm) | ||
| } |
There was a problem hiding this comment.
👍 Excellent improvement: The refactoring of getAlgorithm() correctly distinguishes between:
- Missing algorithm → default to SHA_1 ✓
- Invalid algorithm → return null (reject the entire URI) ✓
The comment explaining why let is intentionally avoided is particularly helpful. This prevents the subtle bug where an invalid algorithm like "sha999" would fall through to the default instead of being rejected.
| val secret = this.getSecret() ?: return null | ||
| val digits = this.getDigits() ?: return null | ||
| val period = this.getPeriod() ?: return null | ||
| val algorithm = this.getAlgorithm() ?: return null |
There was a problem hiding this comment.
👍 Better validation structure: Extracting validation into separate methods with early returns makes the code much more maintainable and the validation logic clearer. Each parameter now has explicit validation rules that are easy to test independently.
|
Great job! No new security vulnerabilities introduced in this pull request |
david-livefront
force-pushed
the
PM-27771-improve-totp-parsing
branch
3 times, most recently
from
5643594 to
dd9e13e
Compare
| /** | ||
| * Checks if a string is using base32 digits. | ||
| */ | ||
| private fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
There was a problem hiding this comment.
🟡 Important - Input Validation: Base32 padding validation could be stricter
The current regex ^[A-Za-z2-7]+=*$ accepts any number of = padding characters. According to RFC 4648, Base32 padding should only result in 0, 1, 3, 4, or 6 padding characters (to make the length a multiple of 8).
Current behavior: AAAA======== would be accepted (8 padding chars - invalid)
Expected: Only specific padding lengths should be valid
Security consideration: Lenient validation may accept malformed Base32 strings that fail during TOTP generation, potentially causing runtime issues.
Stricter validation example (if needed)
private fun String.isBase32(): Boolean {
// Check Base32 alphabet
if (!matches("^[A-Za-z2-7]+=*$".toRegex())) return false
// Validate padding length (0, 1, 3, 4, or 6 according to RFC 4648)
val paddingCount = count { it == '=' }
return paddingCount in setOf(0, 1, 3, 4, 6)
}Question: Is this lenient validation intentional for compatibility with various TOTP providers, or should we enforce RFC 4648 compliance more strictly?
Impact: Medium - Could accept invalid Base32 strings that fail at TOTP generation time
| this | ||
| .getQueryParameter(PARAM_NAME_SECRET) | ||
| ?.trim() | ||
| ?.takeIf { it.isBase32() } |
There was a problem hiding this comment.
🟢 Suggestion - Edge Case: Empty string handling after trim
The current implementation trims whitespace and validates Base32, but an empty string after trimming would match the regex ^[A-Za-z2-7]+=*$ (since * means zero or more).
Suggestion: Explicitly reject empty secrets for clarity:
private fun Uri.getSecret(): String? =
this
.getQueryParameter(PARAM_NAME_SECRET)
?.trim()
?.takeIf { it.isNotEmpty() && it.isBase32() }Impact: Low - Edge case that's unlikely but worth handling explicitly
| // We specifically do not use a `let` here, since we do not want to map an unidentified | ||
| // value to the default value. | ||
| TotpData.CryptoHashAlgorithm.parse(value = algorithm) | ||
| } |
There was a problem hiding this comment.
👍 Excellent improvement: Proper algorithm validation
This is a significant improvement over the old implementation! The refactored logic correctly distinguishes between:
- Missing algorithm → default to SHA_1 ✓
- Invalid algorithm → return null and reject the entire URI ✓
The comment explaining why let is intentionally avoided is particularly valuable. This prevents the subtle bug where invalid algorithms like "sha999" would incorrectly fall through to the default.
Old behavior (from previous code):
algorithm = TotpData.CryptoHashAlgorithm.parse(value = ...) ?: TotpData.CryptoHashAlgorithm.SHA_1This would accept invalid algorithms and default them to SHA_1.
New behavior: Properly rejects invalid algorithms while defaulting only when none is specified.
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "1234567890qwertyuiop" |
There was a problem hiding this comment.
🟢 Test improvement: More explicit invalid Base32 test case
The test secret "1234567890qwertyuiop" contains characters outside the Base32 alphabet (0, 1, 8, 9, and lowercase vowels e/i/o/u). Consider using a clearer example that explicitly demonstrates Base32 validation:
// More explicit examples:
every { getQueryParameter("secret") } returns "0189" // Invalid: contains 0, 1, 8, 9
// or
every { getQueryParameter("secret") } returns "INVALID!" // Invalid: contains special charsThis makes the test intent clearer: we're validating the Base32 alphabet specifically.
Impact: Low - Test clarity improvement
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Missing test coverage for whitespace-only secret
Consider adding a test case for whitespace-only secret strings (e.g., " "). This would verify behavior when getQueryParameter("secret") returns only whitespace.
@Test
fun `getTotpDataOrNull with whitespace-only secret returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns " "
}
assertNull(uri.getTotpDataOrNull())
}Impact: Low - Additional edge case coverage
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Test lower boundary for digits validation
Good coverage for the upper boundary (55 > 10). Consider also testing the lower boundary:
@Test
fun `getTotpDataOrNull with zero digits returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}Impact: Low - Comprehensive boundary testing
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Test zero period explicitly
Since the validation is period < 1, consider also testing period = 0 explicitly:
@Test
fun `getTotpDataOrNull with zero period returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "6"
every { getQueryParameter("period") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}Impact: Low - Comprehensive boundary testing
david-livefront
force-pushed
the
PM-27771-improve-totp-parsing
branch
from
dd9e13e to
55e62b5
Compare
| val regex = ("^[A-Z2-7]+=*$").toRegex() | ||
| return regex.matches(this) | ||
| } | ||
| fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
There was a problem hiding this comment.
🟡 Base32 validation: Empty string edge case
The current regex ^[A-Za-z2-7]+=*$ will match an empty string (since * means zero or more). While the getSecret() function in TotpUriUtils trims the input, an empty string after trimming would still pass this validation.
Recommendation: Consider adding an explicit length check or use + instead of * for non-empty validation:
Option 1: Explicit non-empty check
fun String.isBase32(): Boolean =
isNotEmpty() && "^[A-Za-z2-7]+=*$".toRegex().matches(this)Option 2: Require at least one Base32 character
fun String.isBase32(): Boolean =
"^[A-Za-z2-7]+[A-Za-z2-7=]*$".toRegex().matches(this)Impact: Low - Edge case that's unlikely but worth handling explicitly for robustness
| val regex = ("^[A-Z2-7]+=*$").toRegex() | ||
| return regex.matches(this) | ||
| } | ||
| fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
There was a problem hiding this comment.
🟡 Base32 validation: Padding compliance
According to RFC 4648 Section 6, Base32 padding with = should only result in specific padding lengths to make the total length a multiple of 8. Valid padding counts are: 0, 1, 3, 4, or 6 characters.
Current behavior: AAAA======== (8 padding chars) would be accepted
RFC 4648 compliant: Only 0, 1, 3, 4, or 6 = characters should be valid
Question: Is lenient validation intentional for compatibility with various TOTP providers, or should we enforce stricter RFC 4648 compliance?
Stricter RFC 4648-compliant validation
fun String.isBase32(): Boolean {
// Check Base32 alphabet
if (!matches("^[A-Za-z2-7]+=*$".toRegex())) return false
// Validate padding length per RFC 4648
val paddingCount = count { it == '=' }
return paddingCount in setOf(0, 1, 3, 4, 6)
}Impact: Medium - Lenient validation may accept malformed Base32 strings that fail during actual TOTP generation
Note: RFC 3548 (referenced in TotpData KDoc) states "padding...should be omitted", so many TOTP URIs may not have padding at all. The question is whether we should reject invalid padding patterns.
| // We specifically do not use a `let` here, since we do not want to map an unidentified | ||
| // value to the default value. | ||
| TotpData.CryptoHashAlgorithm.parse(value = algorithm) | ||
| } |
There was a problem hiding this comment.
👍 Excellent algorithm validation logic
This is a significant improvement! The refactored code correctly distinguishes between:
- Missing algorithm → default to SHA_1 ✓
- Invalid algorithm → return null and reject the entire URI ✓
The comment explaining why let is intentionally avoided is particularly valuable. This prevents the subtle bug where invalid algorithms like "sha999" would incorrectly fall through to the default.
Old behavior: Would accept invalid algorithms and default them to SHA_1
New behavior: Properly rejects invalid algorithms while defaulting only when none is specified
| return if (digits == null) { | ||
| // If no value was provided, then we'll default to 6. | ||
| 6 | ||
| } else if (digits < 1 || digits > 10) { |
There was a problem hiding this comment.
ℹ️ Validation bounds rationale
The upper bound of 10 digits is well-chosen. According to RFC 6238 (TOTP specification), most implementations support 6-8 digits, and 10 is a reasonable maximum that prevents potential UI/display issues while allowing flexibility.
The lower bound of 1 prevents zero or negative digit counts which would be meaningless for TOTP codes.
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "1234567890qwertyuiop" |
There was a problem hiding this comment.
🎨 Test clarity: More explicit Base32 validation example
The test secret "1234567890qwertyuiop" contains multiple character types outside Base32:
- Invalid digits:
0,1,8,9(Base32 uses 2-7, not 0-1 or 8-9) - Invalid letters:
e,i,o,u(Base32 alphabet excludes vowels to avoid confusion)
Consider a clearer example that explicitly demonstrates what's being validated:
every { getQueryParameter("secret") } returns "0189" // Invalid: contains 0, 1, 8, 9or
every { getQueryParameter("secret") } returns "INVALID!" // Invalid: special charactersThis makes the test intent more obvious: we're specifically validating the Base32 alphabet.
Impact: Low - Test clarity improvement
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Test coverage: Whitespace-only secret
Consider adding a test case for whitespace-only secret strings (e.g., " "). This would verify behavior when getQueryParameter("secret") returns only whitespace, which gets trimmed to an empty string.
Suggested test case
@Test
fun `getTotpDataOrNull with whitespace-only secret returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns " "
}
assertNull(uri.getTotpDataOrNull())
}Impact: Low - Additional edge case coverage
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Test coverage: Lower boundary for digits
Good coverage for the upper boundary (55 > 10). Consider also testing the lower boundary to ensure comprehensive validation:
Suggested test case
@Test
fun `getTotpDataOrNull with zero digits returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}This would test the digits < 1 validation in TotpUriUtils.kt:75.
Impact: Low - Comprehensive boundary testing
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Test coverage: Zero period boundary
Since the validation is period < 1 (line 93 in TotpUriUtils.kt), consider explicitly testing period = 0:
Suggested test case
@Test
fun `getTotpDataOrNull with zero period returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "6"
every { getQueryParameter("period") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}The current test uses -1 which is good, but testing 0 explicitly would provide complete boundary coverage.
Impact: Low - Comprehensive boundary testing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6119 +/- ##
=======================================
Coverage 84.90% 84.90%
=======================================
Files 724 724
Lines 52780 52798 +18
Branches 7672 7679 +7
=======================================
+ Hits 44811 44829 +18
Misses 5277 5277
Partials 2692 2692 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
david-livefront
force-pushed
the
PM-27771-improve-totp-parsing
branch
from
55e62b5 to
a43f021
Compare
| val regex = ("^[A-Z2-7]+=*$").toRegex() | ||
| return regex.matches(this) | ||
| } | ||
| fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
There was a problem hiding this comment.
🟡 Base32 validation: Empty string edge case
The current regex ^[A-Za-z2-7]+=*$ will match an empty string (since * means zero or more). While getSecret() in TotpUriUtils trims the input, an empty string would still pass this validation.
Recommendation: The current implementation in TotpUriUtils.kt:109 handles this correctly by checking it.isNotEmpty() && it.isBase32(), so this is already protected. However, for the isBase32() extension function itself to be more robust for future use, consider:
| fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) | |
| fun String.isBase32(): Boolean = isNotEmpty() && "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
This makes the function self-contained and safe to use anywhere without requiring callers to remember the empty string check.
| val regex = ("^[A-Z2-7]+=*$").toRegex() | ||
| return regex.matches(this) | ||
| } | ||
| fun String.isBase32(): Boolean = "^[A-Za-z2-7]+=*$".toRegex().matches(this) |
There was a problem hiding this comment.
🟡 Base32 validation: RFC 4648 padding compliance
According to RFC 4648 Section 6, Base32 padding with = should only result in specific padding lengths to make the total length a multiple of 8. Valid padding counts are: 0, 1, 3, 4, or 6 characters.
Current behavior: AAAA======== (8 padding chars) would be accepted
RFC 4648 compliant: Only 0, 1, 3, 4, or 6 = characters should be valid
Question for review: Is lenient validation intentional for compatibility with various TOTP providers, or should we enforce stricter RFC 4648 compliance?
Stricter RFC 4648-compliant validation (if desired)
fun String.isBase32(): Boolean {
// Check Base32 alphabet
if (!matches("^[A-Za-z2-7]+=*$".toRegex())) return false
// Validate padding length per RFC 4648
val paddingCount = count { it == '=' }
return paddingCount in setOf(0, 1, 3, 4, 6)
}Note: RFC 3548 (referenced in TotpData KDoc at line 14) states "padding...should be omitted", so many TOTP URIs may not have padding at all. The question is whether we should reject invalid padding patterns when padding is present.
Impact: Medium - Lenient validation may accept malformed Base32 strings that could fail during actual TOTP generation, though most providers omit padding entirely.
| private fun Uri.getPeriod(): Int? { | ||
| val period = this.getQueryParameter(PARAM_NAME_PERIOD)?.trim()?.toIntOrNull() | ||
| return if (period == null) { | ||
| // If no value was provided, then we'll default to 30. |
There was a problem hiding this comment.
📝 Documentation correction
The comment says "default to 6" but this is the getPeriod() function which correctly defaults to 30 seconds (not the getDigits() function which defaults to 6).
| // If no value was provided, then we'll default to 30. | |
| // If no value was provided, then we'll default to 30. |
| private fun Uri.getAlgorithm(): TotpData.CryptoHashAlgorithm? { | ||
| val algorithm = this | ||
| .getQueryParameter(PARAM_NAME_ALGORITHM) | ||
| ?.trim() | ||
| ?.lowercase() | ||
| return if (algorithm == null) { | ||
| // If no value was provided, then we'll default to SHA_1. | ||
| TotpData.CryptoHashAlgorithm.SHA_1 | ||
| } else { | ||
| // If the value is unidentifiable, then it's invalid. | ||
| // If it's identifiable, then we return the valid value. | ||
| // We specifically do not use a `let` here, since we do not want to map an unidentified | ||
| // value to the default value. | ||
| TotpData.CryptoHashAlgorithm.parse(value = algorithm) | ||
| } |
There was a problem hiding this comment.
👍 Excellent algorithm validation logic
This is a significant improvement over the previous implementation! The refactored code correctly distinguishes between:
- Missing algorithm → default to SHA_1 ✓
- Invalid algorithm → return null and reject the entire URI ✓
The comment at lines 60-62 explaining why let is intentionally avoided is particularly valuable. This prevents the subtle bug where invalid algorithms like "sha999" would incorrectly fall through to the default.
Previous behavior: Would accept invalid algorithms and default them to SHA_1
New behavior: Properly rejects invalid algorithms while defaulting only when none is specified
This makes the validation much more robust and prevents malformed URIs from being accepted.
| /** | ||
| * Attempts to extract the digits from the given totp [Uri]. | ||
| */ | ||
| @Suppress("MagicNumber") | ||
| private fun Uri.getDigits(): Int? { | ||
| val digits = this.getQueryParameter(PARAM_NAME_DIGITS)?.trim()?.toIntOrNull() | ||
| return if (digits == null) { | ||
| // If no value was provided, then we'll default to 6. | ||
| 6 | ||
| } else if (digits < 1 || digits > 10) { | ||
| // If the value is less than 1 or greater than 10, then it's invalid. | ||
| null | ||
| } else { | ||
| // If the value is valid, then we'll return it. | ||
| digits | ||
| } |
There was a problem hiding this comment.
ℹ️ Validation bounds rationale
The validation bounds are well-chosen:
- Upper bound of 10 digits: According to RFC 6238 (TOTP specification), most implementations support 6-8 digits. The upper limit of 10 is a reasonable maximum that prevents potential UI/display issues while allowing flexibility for edge cases.
- Lower bound of 1 digit: Prevents zero or negative digit counts which would be meaningless for TOTP codes.
This strikes a good balance between flexibility and preventing malformed TOTP URIs.
| /** | ||
| * Checks if the given [Uri] contains valid data for a TOTP. The [TotpData] will be returned when | ||
| * the correct data is present or `null` if data is invalid or missing. | ||
| */ | ||
| fun Uri.getTotpDataOrNull(): TotpData? { | ||
| // Must be a "otpauth" scheme | ||
| if (!this.scheme.equals(other = TOTP_SCHEME_NAME, ignoreCase = true)) return null | ||
| // Must be a "totp" host | ||
| if (!this.host.equals(other = TOTP_HOST_NAME, ignoreCase = true)) return null | ||
| val secret = this.getSecret() ?: return null | ||
| val digits = this.getDigits() ?: return null | ||
| val period = this.getPeriod() ?: return null | ||
| val algorithm = this.getAlgorithm() ?: return null | ||
| val segments = this.pathSegments?.firstOrNull()?.split(":") | ||
| val segmentCount = segments?.size ?: 0 | ||
| return TotpData( | ||
| uri = this.toString(), | ||
| issuer = this.getQueryParameter(PARAM_NAME_ISSUER) | ||
| ?: segments?.firstOrNull()?.trim()?.takeIf { segmentCount > 1 }, | ||
| accountName = if (segmentCount > 1) { | ||
| segments?.getOrNull(index = 1)?.trim() | ||
| } else { | ||
| segments?.firstOrNull()?.trim() | ||
| }, | ||
| secret = secret, | ||
| digits = digits, | ||
| period = period, | ||
| algorithm = algorithm, | ||
| ) | ||
| } |
There was a problem hiding this comment.
👍 Improved validation architecture
The refactoring of validation into separate extension functions (getSecret(), getDigits(), getPeriod(), getAlgorithm()) is excellent! This approach provides:
✓ Clear separation of concerns - Each parameter has its own validation logic
✓ Better maintainability - Easy to modify validation rules for individual parameters
✓ Improved testability - Each validation rule can be tested independently
✓ Early returns - Invalid data is rejected immediately with clear null returns
✓ Self-documenting code - The structure makes it obvious when defaults are applied vs. when validation fails
This is a significant architectural improvement over the previous monolithic parsing approach.
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "1234567890qwertyuiop" |
There was a problem hiding this comment.
🎨 Test clarity: More explicit Base32 validation example
The test secret "1234567890qwertyuiop" contains multiple character types outside Base32:
- Invalid digits:
0,1,8,9(Base32 uses 2-7, not 0-1 or 8-9) - Invalid letters:
e,i,o,u(lowercase vowels - Base32 alphabet is A-Z excluding vowels to avoid confusion)
Consider using a clearer example that explicitly demonstrates what's being validated:
every { getQueryParameter("secret") } returns "0189" // Invalid: contains 0, 1, 8, 9or
every { getQueryParameter("secret") } returns "INVALID!" // Invalid: special charactersThis would make the test intent more obvious: we're specifically validating the Base32 alphabet.
Impact: Low - Test works correctly, this is just a clarity improvement.
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Whitespace-only secret test coverage
Consider adding a test case for whitespace-only secret strings (e.g., " "). This would verify behavior when getQueryParameter("secret") returns only whitespace, which gets trimmed to an empty string.
Suggested test case
@Test
fun `getTotpDataOrNull with whitespace-only secret returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns " "
}
assertNull(uri.getTotpDataOrNull())
}This would test the isNotEmpty() check at line 109 in TotpUriUtils.kt after trimming.
Impact: Low - Additional edge case coverage for robustness.
| @Test | ||
| fun `getTotpDataOrNull with invalid digits returns null`() { | ||
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "secret" | ||
| every { getQueryParameter("digits") } returns "11" | ||
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Lower boundary test for digits
Good coverage for the upper boundary (11 > 10). Consider also testing the lower boundary to ensure comprehensive validation:
Suggested test case
@Test
fun `getTotpDataOrNull with zero digits returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}This would test the digits < 1 validation at line 75 in TotpUriUtils.kt.
Impact: Low - Comprehensive boundary testing for robustness.
| @Test | ||
| fun `getTotpDataOrNull with invalid period returns null`() { | ||
| val uri = mockk<Uri> { | ||
| every { scheme } returns "otpauth" | ||
| every { host } returns "totp" | ||
| every { getQueryParameter("secret") } returns "secret" | ||
| every { getQueryParameter("digits") } returns "5" | ||
| every { getQueryParameter("period") } returns "0" | ||
| } | ||
|
|
||
| assertNull(uri.getTotpDataOrNull()) | ||
| } |
There was a problem hiding this comment.
🌱 Future improvement: Zero period boundary test
Since the validation is period < 1 (line 93 in TotpUriUtils.kt), consider explicitly testing period = 0:
Suggested test case
@Test
fun `getTotpDataOrNull with zero period returns null`() {
val uri = mockk<Uri> {
every { scheme } returns "otpauth"
every { host } returns "totp"
every { getQueryParameter("secret") } returns "JBSWY3DPEHPK3PXP" // Valid Base32
every { getQueryParameter("digits") } returns "6"
every { getQueryParameter("period") } returns "0"
}
assertNull(uri.getTotpDataOrNull())
}The current test uses an empty string for period which is also good, but testing "0" explicitly would provide complete boundary coverage.
Impact: Low - Comprehensive boundary testing for robustness.
|
Thanks @SaintPatrck |
🎟️ Tracking
PM-27771
📔 Objective
This PR improves the TOTP parsing logic in the
TotpUriUtilsto handle more edge cases than before.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes