From 9232a185ed1b0ab8c94f830df39e5ce7f24c1ebc Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 20 Mar 2024 14:50:36 +0000 Subject: [PATCH] Avoid antiforgery issues (#13) --- .../DefaultSmartComponentsBuilder.cs | 13 +++++++++ .../ISmartComponentsBuilder.cs | 4 ++- ...tComboBoxEndpointRouteBuilderExtensions.cs | 19 +++++-------- ...rtComponentsServiceCollectionExtensions.cs | 27 ++++++++++++------- .../typescript/SmartComboBox.ts | 18 ++++++++----- .../typescript/SmartPaste.ts | 20 +++++++++----- .../typescript/SmartTextArea/SmartTextArea.ts | 21 +++++++++------ test/testassets/TestBlazorApp/Program.cs | 3 ++- test/testassets/TestMvcApp/Program.cs | 3 ++- 9 files changed, 82 insertions(+), 46 deletions(-) diff --git a/src/SmartComponents.AspNetCore/DefaultSmartComponentsBuilder.cs b/src/SmartComponents.AspNetCore/DefaultSmartComponentsBuilder.cs index b814fc6..607b98b 100644 --- a/src/SmartComponents.AspNetCore/DefaultSmartComponentsBuilder.cs +++ b/src/SmartComponents.AspNetCore/DefaultSmartComponentsBuilder.cs @@ -21,4 +21,17 @@ public ISmartComponentsBuilder WithInferenceBackend(string? name) where T : c return this; } + + public ISmartComponentsBuilder WithAntiforgeryValidation() + { + services.AddSingleton(); + return this; + } + + internal static bool HasEnabledAntiForgeryValidation(IServiceProvider services) + { + return services.GetService() is not null; + } + + internal sealed class SmartComponentsAntiforgeryValidation { } } diff --git a/src/SmartComponents.AspNetCore/ISmartComponentsBuilder.cs b/src/SmartComponents.AspNetCore/ISmartComponentsBuilder.cs index 0b8b284..4486137 100644 --- a/src/SmartComponents.AspNetCore/ISmartComponentsBuilder.cs +++ b/src/SmartComponents.AspNetCore/ISmartComponentsBuilder.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using SmartComponents.StaticAssets.Inference; @@ -8,4 +8,6 @@ namespace Microsoft.AspNetCore.Builder; public interface ISmartComponentsBuilder { public ISmartComponentsBuilder WithInferenceBackend(string? name = null) where T : class, IInferenceBackend; + + public ISmartComponentsBuilder WithAntiforgeryValidation(); } diff --git a/src/SmartComponents.AspNetCore/SmartComboBox/SmartComboBoxEndpointRouteBuilderExtensions.cs b/src/SmartComponents.AspNetCore/SmartComboBox/SmartComboBoxEndpointRouteBuilderExtensions.cs index 072ad9a..e8f0c8d 100644 --- a/src/SmartComponents.AspNetCore/SmartComboBox/SmartComboBoxEndpointRouteBuilderExtensions.cs +++ b/src/SmartComponents.AspNetCore/SmartComboBox/SmartComboBoxEndpointRouteBuilderExtensions.cs @@ -19,21 +19,16 @@ public static IEndpointRouteBuilder MapSmartComboBox(this IEndpointRouteBuilder private static IEndpointRouteBuilder MapSmartComboBoxCore(this IEndpointRouteBuilder builder, string url, Func>> suggestions) { + var validateAntiforgery = DefaultSmartComponentsBuilder.HasEnabledAntiForgeryValidation(builder.ServiceProvider); + var endpoint = builder.MapPost(url, async (HttpContext httpContext, [FromServices] IAntiforgery antiforgery) => { -#if NET8_0_OR_GREATER - // We use DisableAntiforgery and validate manually so that it works whether - // or not you have UseAntiforgery middleware in the pipeline. Without doing that, - // people will get errors like https://stackoverflow.com/questions/61829324 - // - // On .NET 6, we can't enforce antiforgery at all because there's no way for Blazor - // Server or WebAssembly to get/set a token arbitrarily during interactive rendering - // (e.g., may have to set a cookie). This is not really a problem since these endpoints - // don't mutate any state anyway so the protection is not really required - we only do - // it on .NET 8+ because there's no reason not to. - await antiforgery.ValidateRequestAsync(httpContext); -#endif + // See comment in SmartComponentsServiceCollectionExtensions to explain the antiforgery handling + if (validateAntiforgery) + { + await antiforgery.ValidateRequestAsync(httpContext); + } // Can't use [FromForm] on net6.0 var form = httpContext.Request.Form; diff --git a/src/SmartComponents.AspNetCore/SmartComponentsServiceCollectionExtensions.cs b/src/SmartComponents.AspNetCore/SmartComponentsServiceCollectionExtensions.cs index d109a24..9258818 100644 --- a/src/SmartComponents.AspNetCore/SmartComponentsServiceCollectionExtensions.cs +++ b/src/SmartComponents.AspNetCore/SmartComponentsServiceCollectionExtensions.cs @@ -36,16 +36,21 @@ public Action Configure(Action next) = { next(builder); + var validateAntiforgery = DefaultSmartComponentsBuilder.HasEnabledAntiForgeryValidation(builder.ApplicationServices); + builder.UseEndpoints(app => { var smartPasteEndpoint = app.MapPost("/_smartcomponents/smartpaste", async ([FromServices] IInferenceBackend inference, HttpContext httpContext, [FromServices] IAntiforgery antiforgery, [FromServices] SmartPasteInference smartPasteInference) => { -#if NET8_0_OR_GREATER - // We use DisableAntiforgery and validate manually so that it works whether - // or not you have UseAntiforgery middleware in the pipeline. Without doing that, - // people will get errors like https://stackoverflow.com/questions/61829324 - await antiforgery.ValidateRequestAsync(httpContext); -#endif + // The rules about whether antiforgery are enabled by default vary across different + // ASP.NET Core versions. To make it consistent, we disable the default enablement on + // .NET 8, and manually validate it if you've opted in. + // Also note that antiforgery handling has issues (https://github.com/dotnet/aspnetcore/issues/54533) + // so until that's resolved we need this to be off by default. + if (validateAntiforgery) + { + await antiforgery.ValidateRequestAsync(httpContext); + } // Can't use [FromForm] on net6.0 if (!httpContext.Request.Form.TryGetValue("dataJson", out var dataJson)) @@ -59,10 +64,11 @@ public Action Configure(Action next) = var smartTextAreaEndpoint = app.MapPost("/_smartcomponents/smarttextarea", async ([FromServices] IInferenceBackend inference, HttpContext httpContext, [FromServices] IAntiforgery antiforgery, [FromServices] SmartTextAreaInference smartTextAreaInference) => { -#if NET8_0_OR_GREATER - // See above for why we validate antiforgery manually - await antiforgery.ValidateRequestAsync(httpContext); -#endif + if (validateAntiforgery) + { + // See above for why we validate antiforgery manually + await antiforgery.ValidateRequestAsync(httpContext); + } // Can't use [FromForm] on net6.0 var form = httpContext.Request.Form; @@ -79,6 +85,7 @@ public Action Configure(Action next) = }); #if NET8_0_OR_GREATER + // These APIs only exist on .NET 8+. It wasn't enabled by default in prior versions. smartPasteEndpoint.DisableAntiforgery(); smartTextAreaEndpoint.DisableAntiforgery(); #endif diff --git a/src/SmartComponents.StaticAssets/typescript/SmartComboBox.ts b/src/SmartComponents.StaticAssets/typescript/SmartComboBox.ts index 92150f5..ad5b5e9 100644 --- a/src/SmartComponents.StaticAssets/typescript/SmartComboBox.ts +++ b/src/SmartComponents.StaticAssets/typescript/SmartComboBox.ts @@ -70,18 +70,24 @@ class SmartComboBox extends HTMLElement { async _requestSuggestions() { this.currentAbortController = new AbortController(); + const body = { + inputValue: this.inputElem.value, + maxResults: this.getAttribute('data-max-suggestions'), + similarityThreshold: this.getAttribute('data-similarity-threshold'), + }; + + const antiforgeryName = this.getAttribute('data-antiforgery-name'); + if (antiforgeryName) { + body[antiforgeryName] = this.getAttribute('data-antiforgery-value'); + } + let response: Response; const requestInit: RequestInit = { method: 'post', headers: { 'content-type': 'application/x-www-form-urlencoded', }, - body: new URLSearchParams({ - [this.getAttribute('data-antiforgery-name')]: this.getAttribute('data-antiforgery-value'), - inputValue: this.inputElem.value, - maxResults: this.getAttribute('data-max-suggestions'), - similarityThreshold: this.getAttribute('data-similarity-threshold'), - }), + body: new URLSearchParams(body), signal: this.currentAbortController.signal, }; diff --git a/src/SmartComponents.StaticAssets/typescript/SmartPaste.ts b/src/SmartComponents.StaticAssets/typescript/SmartPaste.ts index 6ccae60..de7669d 100644 --- a/src/SmartComponents.StaticAssets/typescript/SmartPaste.ts +++ b/src/SmartComponents.StaticAssets/typescript/SmartPaste.ts @@ -215,6 +215,18 @@ function inferFieldDescription(form: HTMLFormElement, element: HTMLElement): str async function getSmartPasteResponse(button: HTMLButtonElement, formConfig, clipboardContents): Promise { const formFields = formConfig.map(entry => restrictProperties(entry, ['identifier', 'description', 'allowedValues', 'type'])); + const body = { + dataJson: JSON.stringify({ + formFields, + clipboardContents, + }) + }; + + const antiforgeryName = button.getAttribute('data-antiforgery-name'); + if (antiforgeryName) { + body[antiforgeryName] = button.getAttribute('data-antiforgery-value'); + } + // We rely on the URL being pathbase-relative for Blazor, or a ~/... URL that would already // be resolved on the server for MVC const url = button.getAttribute('data-url'); @@ -223,13 +235,7 @@ async function getSmartPasteResponse(button: HTMLButtonElement, formConfig, clip headers: { 'content-type': 'application/x-www-form-urlencoded', }, - body: new URLSearchParams({ - [button.getAttribute('data-antiforgery-name')]: button.getAttribute('data-antiforgery-value'), - dataJson: JSON.stringify({ - formFields, - clipboardContents, - }) - }) + body: new URLSearchParams(body) }); } diff --git a/src/SmartComponents.StaticAssets/typescript/SmartTextArea/SmartTextArea.ts b/src/SmartComponents.StaticAssets/typescript/SmartTextArea/SmartTextArea.ts index fcf95cd..b29e0be 100644 --- a/src/SmartComponents.StaticAssets/typescript/SmartTextArea/SmartTextArea.ts +++ b/src/SmartComponents.StaticAssets/typescript/SmartTextArea/SmartTextArea.ts @@ -117,19 +117,24 @@ export class SmartTextArea extends HTMLElement { cursorPosition: this.textArea.selectionStart, }; + const body = { + // TODO: Limit the amount of text we send, e.g., to 100 characters before and after the cursor + textBefore: snapshot.textAreaValue.substring(0, snapshot.cursorPosition), + textAfter: snapshot.textAreaValue.substring(snapshot.cursorPosition), + config: this.getAttribute('data-config'), + }; + + const antiforgeryName = this.getAttribute('data-antiforgery-name'); + if (antiforgeryName) { + body[antiforgeryName] = this.getAttribute('data-antiforgery-value'); + } + const requestInit: RequestInit = { method: 'post', headers: { 'content-type': 'application/x-www-form-urlencoded', }, - body: new URLSearchParams({ - [this.getAttribute('data-antiforgery-name')]: this.getAttribute('data-antiforgery-value'), - - // TODO: Limit the amount of text we send, e.g., to 100 characters before and after the cursor - textBefore: snapshot.textAreaValue.substring(0, snapshot.cursorPosition), - textAfter: snapshot.textAreaValue.substring(snapshot.cursorPosition), - config: this.getAttribute('data-config'), - }), + body: new URLSearchParams(body), signal: snapshot.abortSignal, }; diff --git a/test/testassets/TestBlazorApp/Program.cs b/test/testassets/TestBlazorApp/Program.cs index 3899bb9..6e3a9db 100644 --- a/test/testassets/TestBlazorApp/Program.cs +++ b/test/testassets/TestBlazorApp/Program.cs @@ -22,7 +22,8 @@ private static void Main(string[] args) .AddInteractiveWebAssemblyComponents(); builder.Services.AddScoped(); builder.Services.AddSmartComponents() - .WithInferenceBackend(); + .WithInferenceBackend() + .WithAntiforgeryValidation(); // This doesn't benefit most apps, but we'll validate it works in E2E tests var app = builder.Build(); diff --git a/test/testassets/TestMvcApp/Program.cs b/test/testassets/TestMvcApp/Program.cs index 9157102..97c3447 100644 --- a/test/testassets/TestMvcApp/Program.cs +++ b/test/testassets/TestMvcApp/Program.cs @@ -19,7 +19,8 @@ private static void Main(string[] args) builder.Services.AddControllersWithViews(); builder.Services.AddScoped(); builder.Services.AddSmartComponents() - .WithInferenceBackend(); + .WithInferenceBackend() + .WithAntiforgeryValidation(); // This doesn't benefit most apps, but we'll validate it works in E2E tests var app = builder.Build();