Skip to content

fix: Handle run-test-payment failures safely - #153

Open
ValeraFinebits wants to merge 3 commits into
masterfrom
fix/Handle-run-test-payment-failures-safely
Open

ValeraFinebits wants to merge 3 commits into
masterfrom
fix/Handle-run-test-payment-failures-safely

Conversation

@ValeraFinebits

Copy link
Copy Markdown
Owner

POST /plugins/payjoin/run-test-payment threw on a malformed body and let any
failure from the payjoin flow escape the action. BTCPay reacts to an exception
escaping plugin code by disabling the plugin and stopping the host, so a bad
request or a transient RPC error took the whole dev server down. The endpoint is
[CheatModeRoute], so this only hit cheat-mode servers, which is where the
manual test plan runs.

  • A null body returns 400 instead of throwing ArgumentNullException.
  • The flow moved to RunTestPaymentCoreAsync, wrapped in a catch that logs and
    reports. A genuine cancellation is still re-thrown.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a safety boundary around cheat-mode test payments to prevent plugin or host shutdowns.

Changes:

  • Returns 400 for null request bodies.
  • Logs unexpected failures while preserving cancellation behavior.
  • Updates null-body test coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
UIPayJoinController.cs Adds validation and exception handling.
UIPayJoinControllerTests.cs Verifies null bodies return 400.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread BTCPayServer.Plugins.Payjoin/Controllers/UIPayJoinController.cs
Comment thread BTCPayServer.Plugins.Payjoin/Controllers/UIPayJoinController.cs Outdated
@ValeraFinebits
ValeraFinebits marked this pull request as ready for review August 19, 2026 19:47
@ValeraFinebits
ValeraFinebits requested a review from chavic August 19, 2026 19:47

@chavic chavic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes for the missing regression coverage.

The implementation passes locally. However, the committed tests do not cover the exception boundary this PR adds. Please add cases that verify:

  • An ordinary exception returns a structured failure response.
  • Cancellation of the actual request propagates.
  • An OperationCanceledException without a cancelled request token returns a failure response.

I tested these cases locally. All 273 tests passed with the current implementation. I then deliberately broke each behavior in an isolated copy. The 270 existing unit tests still passed; the added cases detected each regression.

Please assert the failure response without requiring raw exception text. That leaves room to return a generic client message while logging the details.

@ValeraFinebits

Copy link
Copy Markdown
Owner Author

Requesting changes for the missing regression coverage.

The implementation passes locally. However, the committed tests do not cover the exception boundary this PR adds. Please add cases that verify:

  • An ordinary exception returns a structured failure response.
  • Cancellation of the actual request propagates.
  • An OperationCanceledException without a cancelled request token returns a failure response.

I tested these cases locally. All 273 tests passed with the current implementation. I then deliberately broke each behavior in an isolated copy. The 270 existing unit tests still passed; the added cases detected each regression.

Please assert the failure response without requiring raw exception text. That leaves room to return a generic client message while logging the details.

@chavic Done.

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.

3 participants