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

[Blazor] Add support for antiforgery #49108

Merged
merged 10 commits into from
Jul 5, 2023
Merged

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jun 30, 2023

  • Adds a new IAntiforgeryMetadata interface to describe the antiforgery requirement.
  • Adds a new RequireAntiforgeryToken attribute to require antiforgery.
  • Adds RequireAntiforgeryToken to all razor component endpoints.
  • Adds a new AntiforgeryTokenStateProvider service that retrieves and renders the antiforgery token for the app.
  • Adds a new AntiforgeryToken component that renders the request antiforgery token as a hidden field.
  • Makes EditForm automatically render the AntiforgeryToken when inside of a form binding context.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Jun 30, 2023
@javiercn javiercn force-pushed the javiercn/blazor-antiforgery branch 2 times, most recently from 60a7bf9 to 6ad2c32 Compare July 3, 2023 10:28
@javiercn javiercn marked this pull request as ready for review July 3, 2023 10:38
@javiercn javiercn requested a review from a team as a code owner July 3, 2023 10:38
@@ -175,6 +175,11 @@ public void AddAttribute(int sequence, string name)
throw new InvalidOperationException($"Valueless attributes may only be added immediately after frames of type {RenderTreeFrameType.Element}");
}

if (TrackNamedEventHandlers && string.Equals(name, "@onsubmit:name", StringComparison.Ordinal))
{
_entries.AppendAttribute(sequence, name, "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that people would write <... @onsubmit:name> with no value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do no value, but more commonly you would do @onsubmit:name="" which gets translated to true, which is what this code avoids. Once the directive for the razor compiler is in, this will just be SetEventHandlerName

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

The only remaining thing is an idea that, since AntiforgeryStateProvider only uses PersistentComponentState as an internal implementation detail, it would be ideal not to couple the public API to that. For example, AntiforgeryStateProvider itself could be reduced to an abstract base class or interface that only describes the public API, and then we could have a shared-source DefaultAntiforgeryStateProvider that uses PersistentComponentState and is used in WebAssembly and in Endpoints.

I don't want to hold up your PR with this though. If you feel inclined to do that, that's great. If not, would you be OK with me doing that as a follow-up afterwards? The point is just to keep the public API as lean as it can be, and give us flexibility to change the set of services this depends on over time.

@javiercn
Copy link
Member Author

javiercn commented Jul 3, 2023

If not, would you be OK with me doing that as a follow-up afterwards? The point is just to keep the public API as lean as it can be, and give us flexibility to change the set of services this depends on over time.

That's absolutely fine, I was trying to avoid the need for an additional abstraction, but your point is fair and I don't feel strongly about it. I'll update the PR to change it.

@javiercn javiercn force-pushed the javiercn/blazor-antiforgery branch from 47475e2 to b69673c Compare July 4, 2023 06:41
@javiercn javiercn force-pushed the javiercn/blazor-antiforgery branch from fb3cf3a to bf09a17 Compare July 4, 2023 15:34
@javiercn javiercn merged commit 78bec18 into main Jul 5, 2023
@javiercn javiercn deleted the javiercn/blazor-antiforgery branch July 5, 2023 10:24
@ghost ghost added this to the 8.0-preview7 milestone Jul 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants