Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented Oct 2, 2025

Summary by CodeRabbit

  • New Features

    • OTP QR payloads now include the algorithm parameter for non-SHA1 options (e.g., SHA256, SHA512), improving compatibility with authenticator apps.
  • Tests

    • Expanded test coverage for TOTP/HOTP across SHA256/SHA512, 8-digit codes, custom period and parameters, and default handling (omitting defaults in URIs, HOTP counter defaulting to 1).

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

📝 Walkthrough

Walkthrough

Adds conditional logic in OneTimePassword URI generation to include an algorithm query parameter when the algorithm is not SHA1. Expands unit tests to cover TOTP/HOTP variations, defaults suppression, custom parameters, and default counter behavior, asserting the resulting otpauth URIs.

Changes

Cohort / File(s) Summary
OTP payload generation
QRCoder/PayloadGenerator/OneTimePassword.cs
In ProcessCommonFields, conditionally appends &algorithm=... when AuthAlgorithm != SHA1; flow branches before digits processing. No other logic changes.
Unit tests for OTP URIs
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs
Adds tests for TOTP/HOTP with SHA256/SHA512, 8 digits, custom period, all custom params, HOTP default counter, and omission of default period/digits/algorithm in output.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant OTP as OneTimePassword
  participant PCF as ProcessCommonFields

  C->>OTP: Generate otpauth URI
  OTP->>PCF: Build base URI and common fields
  alt Algorithm != SHA1
    PCF-->>PCF: Append "&algorithm=<ALG>"
  else Algorithm == SHA1
    PCF-->>PCF: Skip algorithm parameter
  end
  PCF-->>OTP: Return common fields
  OTP-->>C: Return final otpauth URI
  note over OTP: Digits/period/counter appended per type and defaults
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely describes the primary change of the pull request—the addition of SHA256 and SHA512 algorithm support for One Time Password generation—so it accurately reflects the main content of the changeset.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_otp

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90df7dd and 1debc1b.

📒 Files selected for processing (2)
  • QRCoder/PayloadGenerator/OneTimePassword.cs (1 hunks)
  • QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/PayloadGenerator/OneTimePassword.cs (3)
QRCoder/PayloadGenerator/BezahlCode.cs (1)
  • ToString (251-332)
QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
  • ToString (51-70)
QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
  • ToString (42-50)
QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (1)
QRCoder/PayloadGenerator/OneTimePassword.cs (3)
  • PayloadGenerator (6-210)
  • OneTimePassword (11-209)
  • ToString (99-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (7)
QRCoder/PayloadGenerator/OneTimePassword.cs (1)

199-202: LGTM! Clean conditional parameter inclusion.

The algorithm parameter is correctly omitted when using the default SHA1, following the same pattern as the period and digits conditionals. The enum's ToString() produces the correct format for the URI.

QRCoderTests/PayloadGeneratorTests/OneTimePasswordTests.cs (6)

67-109: Excellent coverage for SHA256/SHA512 algorithms.

These tests thoroughly validate the new algorithm parameter behavior for both TOTP and HOTP with SHA256 and SHA512, ensuring the parameter is correctly included in the URI.


111-139: Good coverage for custom digits parameter.

These tests verify that non-default digit values are correctly included in the OTP URI for both TOTP and HOTP.


141-153: LGTM! Validates custom period parameter.

This test confirms that non-default period values are correctly appended to the TOTP URI.


155-186: Comprehensive integration tests for multiple custom parameters.

These tests validate that combinations of non-default parameters (algorithm, digits, period/counter) work correctly together and maintain proper parameter ordering in the URI.


188-201: Correctly validates default counter behavior.

This test confirms that HOTP always includes the counter parameter, defaulting to 1 when not explicitly set, which is the correct behavior per the OTP specification.


203-243: Essential tests for default value suppression.

These tests are critical for validating that default values (period=30, digits=6, algorithm=SHA1) are correctly omitted from the URI, keeping it clean and following OTP URI best practices.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Shane32 Shane32 merged commit e8aa494 into master Oct 2, 2025
12 checks passed
@Shane32 Shane32 deleted the fix_otp branch October 2, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OneTimePassword payload doesn't seem to respect Algorithm, Digits, or Period

3 participants