Skip to content

fix: clear the S2955 bug and suppress the deliberate 3DES vulnerabilities#193

Open
blehnen wants to merge 1 commit into
masterfrom
sonar-vuln-bug-suppress
Open

fix: clear the S2955 bug and suppress the deliberate 3DES vulnerabilities#193
blehnen wants to merge 1 commit into
masterfrom
sonar-vuln-bug-suppress

Conversation

@blehnen

@blehnen blehnen commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Track 1 of the badge-reduction effort. Takes the public SonarCloud badge to 0 bugs and 0 vulnerabilities.

Bug — S2955 (Guard.NotNull<T>)

value == null on an unconstrained generic T trips S2955. Replaced with ReferenceEquals(value, null):

  • Behavior-identical — value types box to a non-null reference (so a value-typed T is never "null", as today), and a null Nullable<T> boxes to a null reference (so it still throws). Only genuine null references are caught.
  • Avoids the == operator the rule flags, without a where T : class constraint — that constraint cascaded across every unconstrained-generic caller when it was tried in Sonar Bucket C: reliability bugs + dispose-pattern (S1751/S4586/S3881) #189, so it was reverted then.

Vulnerabilities — S5547 ×2 (TripleDesMessageInterceptor)

Both flag the TripleDES.Create() calls as a weak cipher. This type is already [Obsolete] and retained only so existing 3DES-encrypted messages remain decryptable (new code uses AesMessageInterceptor). Removing 3DES would break decryption of previously-encrypted payloads, so the finding is accepted with justification in code via method-level [SuppressMessage] pointing at the deliberate retention.

Verification

  • Core project builds clean (0/0).
  • TripleDESInterceptorTest 3/3 pass (encrypt/decrypt roundtrip, which also exercises the NotNull input guard).

Note: SonarCloud honors inline [SuppressMessage] for its Roslyn-analyzer rules. If the two S5547 findings don't clear on the next master scan, the fallback is a one-click "Accept" in the SonarCloud UI — but no code change should be needed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of null values in generic scenarios to keep validation behavior consistent and reliable.
    • Continued support for decrypting and encrypting existing legacy-protected messages, helping preserve compatibility with older data.

…ties

- Guard.NotNull<T>: replace `value == null` (S2955 on an unconstrained
  generic) with ReferenceEquals(value, null). Behavior-identical — value
  types box to a non-null reference, a null Nullable<T> boxes to null —
  but avoids the == operator that trips the rule. No `where T : class`
  constraint (which cascaded across all callers when tried in #189).

- TripleDesMessageInterceptor: SuppressMessage the two S5547 "strong
  cipher" vulnerabilities. 3DES is intentionally retained (the type is
  already [Obsolete]) so existing 3DES-encrypted messages stay
  decryptable; new code uses AesMessageInterceptor. Removing it is not
  an option, so the finding is accepted with justification in code.

Takes the SonarCloud badge to 0 bugs and 0 vulnerabilities.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2b9a7567-3af4-42ee-98d9-207fcafce87f

📥 Commits

Reviewing files that changed from the base of the PR and between 0359d22 and c8a8dba.

📒 Files selected for processing (2)
  • Source/DotNetWorkQueue/Interceptors/TripleDesMessageInterceptor.cs
  • Source/DotNetWorkQueue/netfx/System/Guard.cs

📝 Walkthrough

Walkthrough

This PR adds SuppressMessage attributes to suppress the S5547 security analyzer rule on the TripleDesMessageInterceptor's encryption/decryption methods, justified by backward compatibility needs, and updates Guard.NotNull to use ReferenceEquals instead of the equality operator for null checking.

Changes

Analyzer suppression and Guard null-check update

Layer / File(s) Summary
Suppress S5547 for legacy 3DES cipher usage
Source/DotNetWorkQueue/Interceptors/TripleDesMessageInterceptor.cs
Adds a CodeAnalysis using directive and SuppressMessage attributes with justifications on MessageToBytes and BytesToMessage, noting 3DES retention for backward compatibility and pointing to AesMessageInterceptor for new code.
Update Guard.NotNull null-check logic
Source/DotNetWorkQueue/netfx/System/Guard.cs
Changes the null comparison in Guard.NotNull<T> from == null to ReferenceEquals(value, null), with comments explaining behavior for unconstrained generic types.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Possibly related PRs

  • blehnen/DotNetWorkQueue#182: Both PRs modify the legacy TripleDesMessageInterceptor, with the retrieved PR deprecating it while this PR suppresses the S5547 analyzer rule on the same methods.

Poem

A rabbit hops through code so neat,
Suppressing warnings, no defeat 🐇
ReferenceEquals now stands tall,
Null checks safer for one and all,
3DES rests, its job still done,
While AES waits for newer fun! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the two main changes: fixing S2955 in Guard.NotNull and suppressing intentional 3DES-related findings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

@sonarqubecloud

sonarqubecloud Bot commented Jul 9, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.40%. Comparing base (f27a40a) to head (c8a8dba).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #193   +/-   ##
=======================================
  Coverage   87.40%   87.40%           
=======================================
  Files        1023     1023           
  Lines       33646    33646           
  Branches     2833     2833           
=======================================
+ Hits        29408    29409    +1     
+ Misses       3358     3357    -1     
  Partials      880      880           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant