Conversation
… (related to issue #26 )
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method)) | ||
| { | ||
| if (Action == LoginCallbackAction) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, the fix is to ensure that the decision to execute OnLoginCallbackAsync() is not driven directly by user-controlled data such as a query parameter. Instead, the page should infer that it’s in the callback phase based on trusted server-side state, or be wired up via routing (separate callback endpoint) rather than a query flag. At minimum, the query parameter should not be the sole gate; if it’s not actually needed, it should be removed.
The simplest fix that preserves existing visible behavior is to remove the query-parameter-based Action gate entirely. This page is already meant to be reached via an external login callback: OnInitializedAsync always calls SignInManager.GetExternalLoginInfoAsync(), and the comment at lines 101–103 states “We should only reach this page via the login callback”. So instead of checking Action == LoginCallbackAction, we can directly proceed with OnLoginCallbackAsync() on GET requests; if this endpoint is hit via any other HTTP method, we still redirect to login. This removes the tainted comparison altogether while maintaining the intended flow.
Concretely:
- In
ExternalLogin.razor, remove theActionproperty annotated with[SupplyParameterFromQuery], as it is no longer needed. - In
OnInitializedAsync, replace the innerif (Action == LoginCallbackAction)with a direct call toOnLoginCallbackAsync()whenever the request is a GET, preserving the existing redirect for non-GET requests. - No new imports are required; we only modify logic in the shown snippet.
These changes keep functionality aligned with the original intent (“this page is only for the callback”) while eliminating the user-controlled bypass.
| @@ -66,9 +66,6 @@ | ||
| [SupplyParameterFromQuery] | ||
| private string? ReturnUrl { get; set; } | ||
|
|
||
| [SupplyParameterFromQuery] | ||
| private string? Action { get; set; } | ||
|
|
||
| private string? ProviderDisplayName => externalLoginInfo?.ProviderDisplayName; | ||
|
|
||
| protected override async Task OnInitializedAsync() | ||
| @@ -92,16 +89,13 @@ | ||
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method)) | ||
| { | ||
| if (Action == LoginCallbackAction) | ||
| { | ||
| await OnLoginCallbackAsync(); | ||
| return; | ||
| } | ||
|
|
||
| // We should only reach this page via the login callback, so redirect back to | ||
| // the login page if we get here some other way. | ||
| RedirectManager.RedirectTo("Account/Login"); | ||
| await OnLoginCallbackAsync(); | ||
| return; | ||
| } | ||
|
|
||
| // We should only reach this page via the login callback, so redirect back to | ||
| // the login page if we get here some other way. | ||
| RedirectManager.RedirectTo("Account/Login"); | ||
| } | ||
|
|
||
| private async Task OnLoginCallbackAsync() |
|
|
||
| showRemoveButton = passwordHash is not null || currentLogins.Count > 1; | ||
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, the fix is to ensure that sensitive account or authentication actions are not guarded solely by user-controlled values (such as query parameters). Instead, the decision to run such code should depend on internal server-side state or on data that has been validated as part of a secure protocol (for example, checking the presence and validity of an external login authentication result, or using a strongly-typed route/handler instead of a free-form Action query parameter).
For this concrete case, the simplest and least intrusive fix is to stop using the user-provided Action parameter to decide whether to run OnGetLinkLoginCallbackAsync(). If the intended behavior is that this page should always process the external login callback on a GET request (which is typically the case in an external login-linking flow), we can simplify the condition to check only the HTTP method and drop the Action query parameter altogether. This removes the user-controlled factor while preserving functionality: authenticated users making a GET request to this page will have the callback handler executed as before. Concretely:
- In
ExternalLogins.razor, remove theActionproperty annotated with[SupplyParameterFromQuery], since it is no longer needed. - In
OnInitializedAsync, change the conditional from:to:if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction) { await OnGetLinkLoginCallbackAsync(); }
if (HttpMethods.IsGet(HttpContext.Request.Method)) { await OnGetLinkLoginCallbackAsync(); }
No new imports or helper methods are required, and the rest of the page logic remains unchanged.
| @@ -83,8 +83,6 @@ | ||
| [SupplyParameterFromForm] | ||
| private string? ProviderKey { get; set; } | ||
|
|
||
| [SupplyParameterFromQuery] | ||
| private string? Action { get; set; } | ||
|
|
||
| protected override async Task OnInitializedAsync() | ||
| { | ||
| @@ -108,7 +106,7 @@ | ||
|
|
||
| showRemoveButton = passwordHash is not null || currentLogins.Count > 1; | ||
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction) | ||
| if (HttpMethods.IsGet(HttpContext.Request.Method)) | ||
| { | ||
| await OnGetLinkLoginCallbackAsync(); | ||
| } |
|
|
||
| showRemoveButton = passwordHash is not null || currentLogins.Count > 1; | ||
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, the problem is that a sensitive method (OnGetLinkLoginCallbackAsync) is conditionally executed based on a query parameter (Action) that the user controls. To fix this, we should remove that user-controlled decision and instead rely on server-side routing or internal state to determine when the callback should run. In this context, the simplest safe change—without altering the observable functionality of the page—is to stop using the Action query parameter as a guard and use only the HTTP method check (or an equivalent non-user-controlled signal). This preserves the idea that the callback only runs on GET, while removing the tainted input from the condition.
Concretely, in samples/BitBlazor.Sample/BitBlazor.Sample/Components/Account/Pages/Manage/ExternalLogins.razor:
- Leave the
Actionproperty and its[SupplyParameterFromQuery]attribute untouched (removing them could be a behavioral change if other code or links rely on it). - Change the condition on line 111 from
if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction)toif (HttpMethods.IsGet(HttpContext.Request.Method)). This ensures the sensitive callback is no longer gated by user-controlled data; instead, it runs whenever the page is initialized via GET and the user is valid. - No new imports or additional methods are strictly needed;
HttpMethodsis already in use, and the logic fits within the existingOnInitializedAsyncmethod.
This minimal change removes the tainted input from the sensitive control flow while keeping the rest of the behaviour (GET-only, user must exist) intact.
| @@ -108,7 +108,7 @@ | ||
|
|
||
| showRemoveButton = passwordHash is not null || currentLogins.Count > 1; | ||
|
|
||
| if (HttpMethods.IsGet(HttpContext.Request.Method) && Action == LinkLoginCallbackAction) | ||
| if (HttpMethods.IsGet(HttpContext.Request.Method)) | ||
| { | ||
| await OnGetLinkLoginCallbackAsync(); | ||
| } |
This pull request introduces improved validation styling and logic for form components in the BitBlazor library. It adds dynamic CSS class assignment for validation states, ensures correct cleanup of event handlers via
IDisposable, and updates tests to verify the new validation behaviors. These changes provide immediate and clear feedback to users about the validity of form fields.Validation logic and styling improvements:
is-invalid,just-validate-success-field) to input fields inBitFormComponentBase<T>, updating them in response to validation state changes. This includes subscribing toOnValidationStateChangedand providing helper methods for adding the appropriate classes. (src/BitBlazor/Form/BitFormComponentBase.cs) [1] [2] [3]just-validate-error-labelCSS class for consistency with new validation styles. (src/BitBlazor/Form/BitFormComponentBase.cs)AddValidationCssClassmethod to the CSS class computation for all relevant form input components, including Checkbox, SelectField, and Toggle, ensuring validation classes are reflected in the rendered HTML. (src/BitBlazor/Form/BitInputFieldBase.cs,src/BitBlazor/Form/Checkbox/BitCheckbox.razor,src/BitBlazor/Form/Checkbox/BitCheckbox.razor.cs,src/BitBlazor/Form/SelectField/BitSelectField.razor,src/BitBlazor/Form/SelectField/BitSelectField.razor.cs,src/BitBlazor/Form/Toggle/BitToggle.razor,src/BitBlazor/Form/Toggle/BitToggle.razor.cs) [1] [2] [3] [4] [5] [6] [7]Resource cleanup and lifecycle management:
IDisposablepattern inBitFormComponentBase<T>to detach event handlers and prevent memory leaks. (src/BitBlazor/Form/BitFormComponentBase.cs) [1] [2]Testing enhancements:
tests/BitBlazor.Test/Form/Checkbox/BitCheckboxTest.Rendering.razor,tests/BitBlazor.Test/Form/Datepicker/BitDatepickerTest.Rendering.razor,tests/BitBlazor.Test/Form/NumberField/BitNumberFieldTest.Rendering.razor) [1] [2] [3] [4] [5]