Skip to content

Potential fix for code scanning alert no. 454: Generic catch clause - #239

Closed
dborgards wants to merge 2 commits into
mainfrom
alert-autofix-454
Closed

dborgards wants to merge 2 commits into
mainfrom
alert-autofix-454

Conversation

@dborgards

Copy link
Copy Markdown
Owner

Potential fix for https://github.com/dborgards/CanKit.Pro/security/code-scanning/454

To fix this, replace the generic catch (Exception ex) in TransmitAddressClaimConfirmedAsync with specific exception catches that represent expected send-path failures.
Best approach without changing intended functionality:

  • Keep the existing failure-to-actor callback behavior.
  • Catch ObjectDisposedException (likely if dependencies are disposed during async send).
  • Catch InvalidOperationException (common operational/runtime misuse from channel/service state).
  • Do not use a generic catch-all for all exceptions.

This change is localized to src/CanKit.Pro.J1939/J1939NodeImpl.cs, in the method TransmitAddressClaimConfirmedAsync around lines 988–994. No new imports are required.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@dborgards
dborgards marked this pull request as ready for review September 29, 2026 19:01
@cursor

cursor Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches J1939 address-claim completion paths; unexpected exceptions from SendConfirmedAsync would no longer complete the pending claim and could still hang ClaimAddressAsync.

Overview
Replaces the broad catch (Exception) around address-claim SendConfirmedAsync in TransmitAddressClaimConfirmedAsync with targeted handlers for ObjectDisposedException, InvalidOperationException, OperationCanceledException, and CanKitException, all routed through a new PostClaimAnnounceTxThrew helper so the actor still runs OnClaimAnnounceTxFailed with mayStillEcho: false. That keeps ClaimAddressAsync from staying stuck in Claiming when the transmit throws instead of returning a confirmation (including dispose-driven cancellation).

Adds CanKit.Core.Exceptions for CanKitException, extends test ScriptedClaimBus with ReleaseKind.Cancel (TaskCanceledException), and a theory test asserting throw/cancel paths fault the claim and return to NotClaimed.

Reviewed by Cursor Bugbot for commit a89f4b1. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T19:15:33.497600Z a89f4b1 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea3e2474f3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.J1939/J1939NodeImpl.cs Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ea3e247. Configure here.

Comment thread src/CanKit.Pro.J1939/J1939NodeImpl.cs Outdated
@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/CanKit.Pro.J1939/J1939NodeImpl.cs 71.42% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

…opping cancellation

The narrowed catch from the code-scanning autofix only handled
ObjectDisposedException and InvalidOperationException. CanBusService.Dispose
cancels a send that is still waiting for its confirmation, and device
adapters raise CanKitException; neither reached OnClaimAnnounceTxFailed, so
ClaimAddressAsync stayed pending in Claiming. Catch OperationCanceledException
and CanKitException as well, through one helper instead of a copy per clause.

The new test fails against the autofix alone (Cancel case: the claim never
completes) and passes with this change.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nr8LK4Saem4HHC6PNZgHJw

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a89f4b165c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.J1939/J1939NodeImpl.cs
@dborgards dborgards closed this Sep 29, 2026

Copy link
Copy Markdown
Owner Author

Closing without merging, as decided by the maintainer. The generic catch (Exception) in TransmitAddressClaimConfirmedAsync is intentional: the send can fault with any exception type (dispose-driven cancellation, CanKitException, driver faults such as TimeoutException), and every one of them has to reach OnClaimAnnounceTxFailed, otherwise ClaimAddressAsync stays pending in Claiming. A typed list cannot cover that open set, which is what the review threads on this PR showed. Code scanning alert 454 therefore has to be dismissed rather than fixed in code. The last commit on this branch (a89f4b1) is not part of main.


Generated by Claude Code

@dborgards
dborgards deleted the alert-autofix-454 branch October 6, 2026 05:23
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.

2 participants