Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opaque redirect handler results in message "Error: 405 Method Not Allowed" in Blazor app #51118

Closed
DamianEdwards opened this issue Oct 3, 2023 · 3 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Oct 3, 2023

Repro app: https://github.com/DamianEdwards/BlazorFormFun

.NET SDK version: 8.0.100-rc.2.23472.8

I have a Blazor app that's built to simulate an online store, listing items from a catalog, with the capability to add items to a cart/basket. The app is using Blazor SSR with streaming rendering, enhanced navigation & form handling. There is no interactive rendering enabled. Calls to load products, add an item to the cart, get the current cart item count, etc. are async and introduce async delay in order to simulate remote call processing. The availability of the cart itself is delayed to simulate only showing cart UX if the cart service can be successfully called.

When running the application and rapidly clicking the "Add to cart" buttons, an error can be shown in the browser. Note it can take dozens of clicks to reproduce the issue. Opening the browser dev tools network window can sometimes reduce the number of clicks required to reproduce the issue.

image

I've tried to reduce the repro but this is as far as I could go while still having the issue occur.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Oct 3, 2023
@SteveSandersonMS
Copy link
Member

This is a really interesting bug because it was hard to track down, and having done so, feels like a pretty fundamental issue with enhanced navigation. So fundamental that I couldn't quite see how other frameworks would avoid the equivalent issue, so I tried to repro it in SvelteKit's use:enhance (progressively-enhanced forms) and was able to do so - they have the exact same bug.

The core thing is that when you use the HTML History APIs, location.href will inevitably have periods of being out of sync with the current page's content. For example:

  • User is at /a and clicks a link to /b
  • JS intercepts this and does history.pushState('/b'), so location.href is now /b
  • Now it starts doing a fetch('/b'), because it plans to introduce that content into the page
  • While this is in flight, the current DOM contents are for /a but location.href === '/b'. Depending on what the user does during this period, bad stuff can happen.
    • For example, maybe they click submit on a <form> that has no action. That form is meant to be submitted to /a but will actually get submitted to /b, causing who knows what issues? It may cause a 405 Method Not Allowed if /b doesn't accept POST, or it might do something even worse if /b does accept POST.

You might think (as I first did) the framework should solve this by changing the order of operations. It could defer the pushState call until it has completed the fetch and has updated the DOM with the new content. Then the URL and content would never get out of sync, right? No, wrong. The user can still click "back" and then the URL will already have updated before JS even gets notified to begin fetching the content. You'd need to fight against the history system by reverting the 'back' click synchronously then replaying it later after the fetch, which is just going to lead to worse pain. Trying to stop the URL and content getting out of sync is a losing battle.

Possible solutions

I'm not convinced there is any solution for the general version of this problem, which is that code might assume the current URL corresponds to content in the page, but that's not always true.

In the special case of this problem where the user submits a <form> with no action, we could solve it in various ways:

  • Update the enhanced nav system to keep track of the latest "confirmed" URL (i.e., the one corresponding to the latest enhanced page load that actually completed and hence matches the current DOM state). Then use that instead of form.action.
    • However I think this would be a bad move, because how do we know we've really tracked this accurately? Lots of things could cause the document to update. It would be a recipe for bugs unless we could be certain that we always accurately do this tracking everywhere. Plus in error cases (e.g., we partially updated the DOM after an enhanced nav, but a JS error stopped us from updating the whole page), I don't know what we'd even do.
  • Change the server-side rendering logic so that <form> always has an action. That is, if the developer didn't specify any action, we would emit action="currentRequestUrl". Then we can be sure that actionless forms always post to the URL that rendered them, which is exactly what they are meant to do. It no longer matters whether location.href is temporarily out of sync with the DOM content.

My suggestion is we take the second option there, since AFAIK we are in a good position to do that. SSR already detects when it is rendering a form so that it can insert the _handler. It wouldn't be much different to also check whether we rendered an action attribute for it, and if not, to render one with the current URL.

@SteveSandersonMS SteveSandersonMS added the bug This issue describes a behavior which is not expected - a bug. label Oct 4, 2023
@SteveSandersonMS SteveSandersonMS added this to the 8.0.0 milestone Oct 4, 2023
@DamianEdwards
Copy link
Member Author

Agree with your suggested change and IIRC it matches what we did in Web Forms and MVC. I'll try working around the issue by manually setting the form action to the current URL server side when the form is rendered until a framework fix lands.

@javiercn
Copy link
Member

javiercn commented Oct 4, 2023

Change the server-side rendering logic so that

always has an action. That is, if the developer didn't specify any action, we would emit action="currentRequestUrl". Then we can be sure that actionless forms always post to the URL that rendered them, which is exactly what they are meant to do. It no longer matters whether location.href is temporarily out of sync with the DOM content.

We are doing:
#51130

DamianEdwards added a commit to dotnet/aspire that referenced this issue Oct 4, 2023
Workaround for dotnet/aspnetcore#51118. Can remove once that issue's fix flows to builds we're using.
DamianEdwards added a commit to dotnet/aspire that referenced this issue Oct 4, 2023
Workaround for dotnet/aspnetcore#51118. Can remove once that issue's fix flows to builds we're using.
mkArtakMSFT pushed a commit that referenced this issue Oct 11, 2023
This change updates the server rendering logic to always emit an absolute URL for a form action when none is specified explicitly.

## Description

When as part of rendering a form, the developer does not explicitly add an `action` attribute, the framework will automatically generate one with the value of the current request URL.

Fixes #51118

## Customer Impact

When enhanced navigation is active, the URL might change while an update to the page is in progress. In such situations, if the user clicks a button and submits a form, the form might incorrectly post to the wrong URL, as the page URL might have already been updated. 

Given that when the URL updates on the document is not a behavior, we have control over, we have to account for this, and to prevent it, we make sure that we always generate forms with an action attribute, which is unambiguous.

## Regression?

- [ ] Yes
- [X] No

[If yes, specify the version the behavior has regressed from]

## Risk

- [ ] High
- [ ] Medium
- [X] Low

The fix is to detect forms without an `action` attribute and emit it to the current URL. This is correct in all cases and is unambiguous.

## Verification

- [x] Manual (required)
- [X] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A

----

## When servicing release/2.1

- [ ] Make necessary changes in eng/PatchConfig.props
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

4 participants