What the developer wrote
Authored by Nicolas Dorier
Small refactor
What changed, and why it matters
This commit refactors authorization checks in two wallet-related controller actions. In one case, the 'clear' command previously required both server-admin role AND store-level wallet-settings permission; now it only requires a server-level policy. In another case, a new switch statement replaces nested if-checks but appears functionally equivalent. The change could be a security fix if the old combined check was overly restrictive, or a security weakening if the new single policy is less protective than the old two-factor check. Without vendor context, the security relevance is uncertain.
Review the definition and enforcement of Policies.CanModifyServerSettings to confirm it is at least as restrictive as the previous combination of Roles.ServerAdmin plus WalletPolicies.CanManageWalletSettings. If the new policy is weaker, revert or add the additional checks. Otherwise, treat as a benign refactor and add tests covering authorization for the 'clear' command.
Security signals we found
Authorization logic refactor with semantic change for 'clear' command
Removal of explicit server-admin role check for 'clear' command
Removal of store-level WalletPolicies.CanManageWalletSettings check for 'clear' command
Replacement with Policies.CanModifyServerSettings for 'clear' command
Functional-equivalent refactor for sign/analyze-psbt/schedule checks
Evidence from the diff
In UIWalletsController.cs, two authorization blocks are refactored. The first (around line 1042) replaces three separate if/or authorization calls with a switch expression selecting the required policy, then a single AuthorizeAsync call. Behavior appears equivalent: sign/analyze-psbt require WalletPolicies.CanSignWalletTransactions, schedule requires Policies.CanManagePayouts, and other commands skip the check. The second block (around line 1797) changes the ‘clear’ command authorization from ‘User.IsInRole(Roles.ServerAdmin) && AuthorizeAsync(… WalletPolicies.CanManageWalletSettings)’ to just ‘Policies.CanModifyServerSettings’. This is a semantic change: it removes the explicit server-admin role check and the store-level wallet-settings policy, replacing them with a presumably server-level policy. Whether this is a hardening, bug fix, or privilege-lowering change depends on how Policies.CanModifyServerSettings is defined and whether it implies the previous requirements.
Changed components
BTCPayServer/Plugins/Wallets/Controllers/UIWalletsController.csWallet 'clear' command authorizationWallet send command authorization (sign/analyze-psbt/schedule)Inspect captured patch +16 / −10
diff --git a/BTCPayServer/Plugins/Wallets/Controllers/UIWalletsController.cs b/BTCPayServer/Plugins/Wallets/Controllers/UIWalletsController.cs
index f5c1ed1..41b1655 100644
--- a/BTCPayServer/Plugins/Wallets/Controllers/UIWalletsController.cs
+++ b/BTCPayServer/Plugins/Wallets/Controllers/UIWalletsController.cs
@@ -12,6 +12,7 @@ using BTCPayServer.Abstractions.Constants;
using BTCPayServer.Abstractions.Extensions;
using BTCPayServer.Abstractions.Models;
using BTCPayServer.BIP78.Sender;
+using BTCPayServer.Blazor;
using BTCPayServer.Client;
using BTCPayServer.Client.Models;
using BTCPayServer.Data;
@@ -1042,9 +1043,15 @@ namespace BTCPayServer.Controllers
WalletId walletId, WalletSendModel vm, string command = "", CancellationToken cancellation = default,
string? bip21 = "")
{
- if (command == "sign" && !(await authorizationService.AuthorizeAsync(User, walletId.StoreId, WalletPolicies.CanSignWalletTransactions)).Succeeded ||
- command == "analyze-psbt" && !(await authorizationService.AuthorizeAsync(User, walletId.StoreId, WalletPolicies.CanSignWalletTransactions)).Succeeded ||
- command == "schedule" && !(await authorizationService.AuthorizeAsync(User, walletId.StoreId, Policies.CanManagePayouts)).Succeeded)
+ var required = command switch
+ {
+ "sign" => WalletPolicies.CanSignWalletTransactions,
+ "analyze-psbt" => WalletPolicies.CanSignWalletTransactions,
+ "schedule" => Policies.CanManagePayouts,
+ _ => null
+ };
+
+ if (required is not null && !(await authorizationService.AuthorizeAsync(User, walletId.StoreId, required)).Succeeded)
return Forbid();
var store = await Repository.FindStore(walletId.StoreId);
if (store == null)
@@ -1797,15 +1804,14 @@ namespace BTCPayServer.Controllers
if (command is not ("cpfp" or "prune" or "clear"))
return NotFound();
- var authorized = command switch
+ var required = command switch
{
- "cpfp" => (await authorizationService.AuthorizeAsync(User, walletId.StoreId, WalletPolicies.CanCreateWalletTransactions)).Succeeded,
- "prune" => (await authorizationService.AuthorizeAsync(User, walletId.StoreId, WalletPolicies.CanManageWalletSettings)).Succeeded,
- "clear" => User.IsInRole(Roles.ServerAdmin) &&
- (await authorizationService.AuthorizeAsync(User, walletId.StoreId, WalletPolicies.CanManageWalletSettings)).Succeeded,
- _ => false
+ "cpfp" => WalletPolicies.CanCreateWalletTransactions,
+ "prune" => WalletPolicies.CanManageWalletSettings,
+ "clear" => Policies.CanModifyServerSettings,
+ _ => null
};
- if (!authorized)
+ if (required is null || !(await authorizationService.AuthorizeAsync(User, walletId.StoreId, required)).Succeeded)
return Forbid();
var derivationScheme = GetDerivationSchemeSettings(walletId);
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.