From 474608544480ec8815fbbb8590e034f3f42fa066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sat, 7 Sep 2024 16:02:29 +0200 Subject: [PATCH] Block postbacks for 5s after redirects The page does not unload right after we perform a navigation, and allows the user to hit the submit button a second time. In the past, we used to disable all postbacks after a redirect, but that also blocks the page after a file is returned. There probably isn't a 100% reliable way to detect if the location change is a navigation or a file return. With this patch, we will block the page again, but only for a limited time (5 seconds). We also only only block the postback queue, which means that only postbacks with Concurrency=Deny or Queue will be affected. Standard file returns as provided by DotVVM are excluded from this, but it should also work acceptably with custom file returns (some buttons will not work for 5 seconds after the file return) --- .../Hosting/DotvvmRequestContextExtensions.cs | 8 +- .../Framework/Hosting/HttpRedirectService.cs | 7 +- .../Resources/Scripts/postback/redirect.ts | 17 +++- .../Resources/Scripts/tests/eventArgs.test.ts | 6 +- .../Scripts/utils/magic-navigator.ts | 7 +- .../Resources/Scripts/utils/promise.ts | 1 + .../DefaultViewModelSerializer.cs | 3 +- .../RedirectPostbackConcurrencyViewModel.cs | 65 +++++++++++++ .../RedirectPostbackConcurrency.dothtml | 33 +++++++ .../Abstractions/SamplesRouteUrls.designer.cs | 1 + .../Tests/Tests/Feature/RedirectTests.cs | 94 ++++++++++++++++++- 11 files changed, 227 insertions(+), 15 deletions(-) create mode 100644 src/Samples/Common/ViewModels/FeatureSamples/Redirect/RedirectPostbackConcurrencyViewModel.cs create mode 100644 src/Samples/Common/Views/FeatureSamples/Redirect/RedirectPostbackConcurrency.dothtml diff --git a/src/Framework/Framework/Hosting/DotvvmRequestContextExtensions.cs b/src/Framework/Framework/Hosting/DotvvmRequestContextExtensions.cs index 913347ffb7..8bc46a5b7a 100644 --- a/src/Framework/Framework/Hosting/DotvvmRequestContextExtensions.cs +++ b/src/Framework/Framework/Hosting/DotvvmRequestContextExtensions.cs @@ -141,8 +141,8 @@ public static void RedirectToRoutePermanent(this IDotvvmRequestContext context, context.RedirectToUrlPermanent(url, replaceInHistory, allowSpaRedirect); } - public static void SetRedirectResponse(this IDotvvmRequestContext context, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false) => - context.Configuration.ServiceProvider.GetRequiredService().WriteRedirectResponse(context.HttpContext, url, statusCode, replaceInHistory, allowSpaRedirect); + public static void SetRedirectResponse(this IDotvvmRequestContext context, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false, string? downloadName = null) => + context.Configuration.ServiceProvider.GetRequiredService().WriteRedirectResponse(context.HttpContext, url, statusCode, replaceInHistory, allowSpaRedirect, downloadName); internal static Task SetCachedViewModelMissingResponse(this IDotvvmRequestContext context) { @@ -262,8 +262,10 @@ public static async Task ReturnFileAsync(this IDotvvmRequestContext context, Str AttachmentDispositionType = attachmentDispositionType ?? "attachment" }; + var downloadAttribute = attachmentDispositionType == "inline" ? null : fileName; + var generatedFileId = await returnedFileStorage.StoreFileAsync(stream, metadata).ConfigureAwait(false); - context.SetRedirectResponse(context.TranslateVirtualPath("~/dotvvmReturnedFile?id=" + generatedFileId)); + context.SetRedirectResponse(context.TranslateVirtualPath("~/dotvvmReturnedFile?id=" + generatedFileId), downloadName: downloadAttribute); throw new DotvvmInterruptRequestExecutionException(InterruptReason.ReturnFile, fileName); } diff --git a/src/Framework/Framework/Hosting/HttpRedirectService.cs b/src/Framework/Framework/Hosting/HttpRedirectService.cs index d1aef7f12f..c29281b6c7 100644 --- a/src/Framework/Framework/Hosting/HttpRedirectService.cs +++ b/src/Framework/Framework/Hosting/HttpRedirectService.cs @@ -25,14 +25,13 @@ namespace DotVVM.Framework.Hosting { public interface IHttpRedirectService { - void WriteRedirectResponse(IHttpContext httpContext, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false); + void WriteRedirectResponse(IHttpContext httpContext, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false, string? downloadName = null); } public class DefaultHttpRedirectService: IHttpRedirectService { - public void WriteRedirectResponse(IHttpContext httpContext, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false) + public void WriteRedirectResponse(IHttpContext httpContext, string url, int statusCode = (int)HttpStatusCode.Redirect, bool replaceInHistory = false, bool allowSpaRedirect = false, string? downloadName = null) { - if (DotvvmRequestContext.DetermineRequestType(httpContext) is DotvvmRequestType.Navigate) { httpContext.Response.Headers["Location"] = url; @@ -43,7 +42,7 @@ public void WriteRedirectResponse(IHttpContext httpContext, string url, int stat httpContext.Response.StatusCode = 200; httpContext.Response.ContentType = "application/json"; httpContext.Response - .WriteAsync(DefaultViewModelSerializer.GenerateRedirectActionResponse(url, replaceInHistory, allowSpaRedirect)) + .WriteAsync(DefaultViewModelSerializer.GenerateRedirectActionResponse(url, replaceInHistory, allowSpaRedirect, downloadName)) .GetAwaiter().GetResult(); // ^ we just wait for this Task. Redirect API never was async and the response size is small enough that we can't quite safely wait for the result // .GetAwaiter().GetResult() preserves stack traces across async calls, thus I like it more that .Wait() diff --git a/src/Framework/Framework/Resources/Scripts/postback/redirect.ts b/src/Framework/Framework/Resources/Scripts/postback/redirect.ts index bad3b21184..fb28132976 100644 --- a/src/Framework/Framework/Resources/Scripts/postback/redirect.ts +++ b/src/Framework/Framework/Resources/Scripts/postback/redirect.ts @@ -1,17 +1,23 @@ import * as events from '../events'; import * as magicNavigator from '../utils/magic-navigator' import { handleSpaNavigationCore } from "../spa/spa"; +import { delay } from '../utils/promise'; + export function performRedirect(url: string, replace: boolean, allowSpa: boolean): Promise { if (replace) { location.replace(url); - return Promise.resolve(); } else if (compileConstants.isSpa && allowSpa) { return handleSpaNavigationCore(url); } else { magicNavigator.navigate(url); - return Promise.resolve(); } + + // When performing redirect, we pretend that the request takes additional X second to avoid + // double submit with Postback.Concurrency=Deny or Queue. + // We do not want to block the page forever, as the redirect might just return a file (or HTTP 204/205), + // and the page will continue to live. + return delay(5_000); } export async function handleRedirect(options: PostbackOptions, resultObject: any, response: Response, replace: boolean = false): Promise { @@ -28,7 +34,12 @@ export async function handleRedirect(options: PostbackOptions, resultObject: any } events.redirect.trigger(redirectArgs); - await performRedirect(url, replace, resultObject.allowSpa); + const downloadFileName = resultObject.download + if (downloadFileName != null) { + magicNavigator.navigate(url, downloadFileName) + } else { + await performRedirect(url, replace, resultObject.allowSpa); + } return redirectArgs; } diff --git a/src/Framework/Framework/Resources/Scripts/tests/eventArgs.test.ts b/src/Framework/Framework/Resources/Scripts/tests/eventArgs.test.ts index 097fe5c478..d6c29aa268 100644 --- a/src/Framework/Framework/Resources/Scripts/tests/eventArgs.test.ts +++ b/src/Framework/Framework/Resources/Scripts/tests/eventArgs.test.ts @@ -174,7 +174,8 @@ const fetchDefinitions = { postbackRedirect: async (url: string, init: RequestInit) => { return { action: "redirect", - url: "/newUrl" + url: "/newUrl", + download: "some-file" // say it's a file, so that DotVVM does not block postback queue after the test } as any; }, @@ -213,7 +214,8 @@ const fetchDefinitions = { return { action: "redirect", url: "/newUrl", - allowSpa: true + allowSpa: true, + download: "some-file" } as any; }, spaNavigateError: async (url: string, init: RequestInit) => { diff --git a/src/Framework/Framework/Resources/Scripts/utils/magic-navigator.ts b/src/Framework/Framework/Resources/Scripts/utils/magic-navigator.ts index 60e3486ab1..b97ca9dc46 100644 --- a/src/Framework/Framework/Resources/Scripts/utils/magic-navigator.ts +++ b/src/Framework/Framework/Resources/Scripts/utils/magic-navigator.ts @@ -1,10 +1,15 @@ let fakeAnchor: HTMLAnchorElement | undefined; -export function navigate(url: string) { +export function navigate(url: string, downloadName: string | null | undefined = null) { if (!fakeAnchor) { fakeAnchor = document.createElement("a"); fakeAnchor.style.display = "none"; document.body.appendChild(fakeAnchor); } + if (downloadName == null) { + fakeAnchor.removeAttribute("download"); + } else { + fakeAnchor.download = downloadName + } fakeAnchor.href = url; fakeAnchor.click(); } diff --git a/src/Framework/Framework/Resources/Scripts/utils/promise.ts b/src/Framework/Framework/Resources/Scripts/utils/promise.ts index a860c54669..c702c2f86e 100644 --- a/src/Framework/Framework/Resources/Scripts/utils/promise.ts +++ b/src/Framework/Framework/Resources/Scripts/utils/promise.ts @@ -1,2 +1,3 @@ /** Runs the callback in the next event loop cycle */ export const defer = (callback: () => T) => Promise.resolve().then(callback) +export const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)) diff --git a/src/Framework/Framework/ViewModel/Serialization/DefaultViewModelSerializer.cs b/src/Framework/Framework/ViewModel/Serialization/DefaultViewModelSerializer.cs index eb35dc99f4..db51e3f283 100644 --- a/src/Framework/Framework/ViewModel/Serialization/DefaultViewModelSerializer.cs +++ b/src/Framework/Framework/ViewModel/Serialization/DefaultViewModelSerializer.cs @@ -258,7 +258,7 @@ public JObject BuildResourcesJson(IDotvvmRequestContext context, Func /// Serializes the redirect action. /// - public static string GenerateRedirectActionResponse(string url, bool replace, bool allowSpa) + public static string GenerateRedirectActionResponse(string url, bool replace, bool allowSpa, string? downloadName) { // create result object var result = new JObject(); @@ -266,6 +266,7 @@ public static string GenerateRedirectActionResponse(string url, bool replace, bo result["action"] = "redirect"; if (replace) result["replace"] = true; if (allowSpa) result["allowSpa"] = true; + if (downloadName is object) result["download"] = downloadName; return result.ToString(Formatting.None); } diff --git a/src/Samples/Common/ViewModels/FeatureSamples/Redirect/RedirectPostbackConcurrencyViewModel.cs b/src/Samples/Common/ViewModels/FeatureSamples/Redirect/RedirectPostbackConcurrencyViewModel.cs new file mode 100644 index 0000000000..75f53ba0b1 --- /dev/null +++ b/src/Samples/Common/ViewModels/FeatureSamples/Redirect/RedirectPostbackConcurrencyViewModel.cs @@ -0,0 +1,65 @@ +using System.Diagnostics.Metrics; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using DotVVM.Core.Storage; +using DotVVM.Framework.ViewModel; + +namespace DotVVM.Samples.BasicSamples.ViewModels.FeatureSamples.Redirect +{ + class RedirectPostbackConcurrencyViewModel : DotvvmViewModelBase + { + public static int GlobalCounter = 0; + private readonly IReturnedFileStorage returnedFileStorage; + + [Bind(Direction.ServerToClient)] + public int Counter { get; set; } = GlobalCounter; + + public int MiniCounter { get; set; } = 0; + + [FromQuery("empty")] + public bool IsEmptyPage { get; set; } = false; + [FromQuery("loadDelay")] + public int LoadDelay { get; set; } = 0; + + public RedirectPostbackConcurrencyViewModel(IReturnedFileStorage returnedFileStorage) + { + this.returnedFileStorage = returnedFileStorage; + } + public override async Task Init() + { + await Task.Delay(LoadDelay); // delay to enable user to click DelayIncrement button between it succeeding and loading the next page + await base.Init(); + } + + public async Task DelayIncrement() + { + await Task.Delay(1000); + + Interlocked.Increment(ref GlobalCounter); + + Context.RedirectToRoute(Context.Route.RouteName, query: new { empty = true, loadDelay = 2000 }); + } + + public async Task GetFileStandard() + { + await Context.ReturnFileAsync("test file"u8.ToArray(), "test.txt", "text/plain"); + } + + public async Task GetFileCustom() + { + var metadata = new ReturnedFileMetadata() + { + FileName = "test_custom.txt", + MimeType = "text/plain", + AttachmentDispositionType = "attachment" + }; + + var stream = new MemoryStream("test custom file"u8.ToArray()); + var generatedFileId = await returnedFileStorage.StoreFileAsync(stream, metadata).ConfigureAwait(false); + + var url = Context.TranslateVirtualPath("~/dotvvmReturnedFile?id=" + generatedFileId); + Context.RedirectToUrl(url); + } + } +} diff --git a/src/Samples/Common/Views/FeatureSamples/Redirect/RedirectPostbackConcurrency.dothtml b/src/Samples/Common/Views/FeatureSamples/Redirect/RedirectPostbackConcurrency.dothtml new file mode 100644 index 0000000000..f9838fcdc6 --- /dev/null +++ b/src/Samples/Common/Views/FeatureSamples/Redirect/RedirectPostbackConcurrency.dothtml @@ -0,0 +1,33 @@ +@viewModel DotVVM.Samples.BasicSamples.ViewModels.FeatureSamples.Redirect.RedirectPostbackConcurrencyViewModel, DotVVM.Samples.Common + + + Hello from DotVVM! + + +
+ Back +
+
+

Redirect and postback concurrency test

+

+ Testing Concurrency=Deny / Concurrency=Queue with redirect and file returns. +

+

First, we have a set of buttons incrementing a static variable, each takes about 2sec and redirects to a blank page afterwards

+

GlobalCounter =

+

MiniCounter(Concurrency=Deny) =

+ +

+ Increment (Concurrency=Default) + Increment (Concurrency=Deny) + Increment (Concurrency=Queue) +

+ +

We also test that returning files does not block the page forever,

+ +

+ Standard return file + Custom return file (will have delay before page works) +

+
+ + diff --git a/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs b/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs index ff8f0ccc1b..83323d2379 100644 --- a/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs +++ b/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs @@ -327,6 +327,7 @@ public partial class SamplesRouteUrls public const string FeatureSamples_Redirect_RedirectionHelpers_PageC = "FeatureSamples/Redirect/RedirectionHelpers_PageC"; public const string FeatureSamples_Redirect_RedirectionHelpers_PageD = "FeatureSamples/Redirect/RedirectionHelpers_PageD"; public const string FeatureSamples_Redirect_RedirectionHelpers_PageE = "FeatureSamples/Redirect/RedirectionHelpers_PageE"; + public const string FeatureSamples_Redirect_RedirectPostbackConcurrency = "FeatureSamples/Redirect/RedirectPostbackConcurrency"; public const string FeatureSamples_Redirect_Redirect_StaticCommand = "FeatureSamples/Redirect/Redirect_StaticCommand"; public const string FeatureSamples_RenderSettingsModeServer_RenderSettingModeServerProperty = "FeatureSamples/RenderSettingsModeServer/RenderSettingModeServerProperty"; public const string FeatureSamples_RenderSettingsModeServer_RepeaterCollectionExchange = "FeatureSamples/RenderSettingsModeServer/RepeaterCollectionExchange"; diff --git a/src/Samples/Tests/Tests/Feature/RedirectTests.cs b/src/Samples/Tests/Tests/Feature/RedirectTests.cs index 70af6f5cc2..ef9b12b62e 100644 --- a/src/Samples/Tests/Tests/Feature/RedirectTests.cs +++ b/src/Samples/Tests/Tests/Feature/RedirectTests.cs @@ -1,7 +1,11 @@ using System; using System.Linq; +using System.Threading; using DotVVM.Samples.Tests.Base; using DotVVM.Testing.Abstractions; +using OpenQA.Selenium; +using Riganti.Selenium.Core; +using Riganti.Selenium.Core.Abstractions; using Riganti.Selenium.DotVVM; using Xunit; using Xunit.Abstractions; @@ -69,6 +73,94 @@ public void Feature_Redirect_RedirectionHelpers() Assert.Matches($"{SamplesRouteUrls.FeatureSamples_Redirect_RedirectionHelpers_PageE}/1221\\?x=a", currentUrl.LocalPath + currentUrl.Query); }); } - + + bool TryClick(IElementWrapper element) + { + if (element is null) return false; + try + { + element.Click(); + return true; + } + catch (StaleElementReferenceException) + { + return false; + } + } + + [Fact] + public void Feature_Redirect_RedirectPostbackConcurrency() + { + RunInAllBrowsers(browser => { + browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_Redirect_RedirectPostbackConcurrency); + + int globalCounter() => int.Parse(browser.First("counter", SelectByDataUi).GetText()); + + var initialCounter = globalCounter(); + for (int i = 0; i < 20; i++) + { + TryClick(browser.FirstOrDefault("inc-default", SelectByDataUi)); + Thread.Sleep(1); + } + browser.WaitFor(() => Assert.Contains("empty=true", browser.CurrentUrl, StringComparison.OrdinalIgnoreCase), 7000, "Redirect did not happen"); + browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_Redirect_RedirectPostbackConcurrency); + + // must increment at least 20 times, otherwise delays are too short + Assert.Equal(globalCounter(), initialCounter + 20); + + initialCounter = globalCounter(); + var clickCount = 0; + while (TryClick(browser.FirstOrDefault("inc-deny", SelectByDataUi))) + { + clickCount++; + Thread.Sleep(1); + } + Assert.InRange(clickCount, 3, int.MaxValue); + browser.WaitFor(() => Assert.Contains("empty=true", browser.CurrentUrl, StringComparison.OrdinalIgnoreCase), timeout: 500, "Redirect did not happen"); + + browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_Redirect_RedirectPostbackConcurrency); + Assert.Equal(globalCounter(), initialCounter + 1); // only one click was allowed + + initialCounter = globalCounter(); + clickCount = 0; + while (TryClick(browser.FirstOrDefault("inc-queue", SelectByDataUi))) + { + clickCount++; + Thread.Sleep(1); + } + + Assert.InRange(clickCount, 3, int.MaxValue); + browser.WaitFor(() => Assert.Contains("empty=true", browser.CurrentUrl, StringComparison.OrdinalIgnoreCase), timeout: 500, "Redirect did not happen"); + + browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_Redirect_RedirectPostbackConcurrency); + Assert.Equal(globalCounter(), initialCounter + 1); // only one click was allowed + }); + } + + [Fact] + public void Feature_Redirect_RedirectPostbackConcurrencyFileReturn() + { + RunInAllBrowsers(browser => { + browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_Redirect_RedirectPostbackConcurrency); + + void increment(int timeout) + { + browser.WaitFor(() => { + var original = int.Parse(browser.First("minicounter", SelectByDataUi).GetText()); + browser.First("minicounter", SelectByDataUi).Click(); + AssertUI.TextEquals(browser.First("minicounter", SelectByDataUi), (original + 1).ToString()); + }, timeout, "Could not increment minicounter in given timeout (postback queue is blocked)"); + } + + increment(3000); + + browser.First("file-std", SelectByDataUi).Click(); + increment(3000); + + browser.First("file-custom", SelectByDataUi).Click(); + // longer timeout, because DotVVM blocks postback queue for 5s after redirects to debounce any further requests + increment(15000); + }); + } } }