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

Warn on MaximumReceiveMessageSize #27014

Merged
merged 8 commits into from Sep 27, 2022
Merged

Warn on MaximumReceiveMessageSize #27014

merged 8 commits into from Sep 27, 2022

Conversation

guardrex
Copy link
Collaborator

Fixes #27013

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 16, 2022

Let's hold off on review. We're discussing this on the PU issue. We might do something different ... an INCLUDE used in the Blazor doc and the SignalR doc, for example. We might do this, but we might only do it in the SignalR doc because the Blazor doc already links to the SignalR doc for the configuration details.

@guardrex guardrex marked this pull request as draft September 16, 2022 10:29
@guardrex guardrex self-assigned this Sep 16, 2022
@guardrex
Copy link
Collaborator Author

guardrex commented Sep 17, 2022

The PU issue was closed right in the middle of the discussion on this.

Let's go with Rick's Gambit™ ... i.e. ... Call for review! 😆

Brennan, which way would you like to go?

@guardrex guardrex marked this pull request as ready for review September 17, 2022 23:12
@BrennanConroy
Copy link
Member

  • I can place it in the Description in the table at ...

Sure, similar to how ApplicationMaxBufferSize in a later table mentions memory usage can increase if the setting is changed.

@guardrex
Copy link
Collaborator Author

@BrennanConroy ... Added a remark to the table. How's it look?

Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

LGTM, will leave the final sign-off for @BrennanConroy

aspnetcore/blazor/fundamentals/signalr.md Outdated Show resolved Hide resolved
aspnetcore/blazor/security/server/threat-mitigation.md Outdated Show resolved Hide resolved
Co-authored-by: Tanay Parikh <TanayParikh@users.noreply.github.com>
@guardrex guardrex closed this Sep 20, 2022
@guardrex guardrex reopened this Sep 20, 2022
Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd await @BrennanConroy's signoff.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 20, 2022

I will ... and btw ... I've talked myself out of using the explicit byte conventions for abbreviations, as you recommended, on the issue that I opened ...

#27056

I wish MS/.NET foundation would adopt the standard. What I can do is place a remark on it in the Blazor Fundamentals topic and just use "KB"/"MB"/"GB" everywhere.

I'm going to perform a universal update on that per that issue, so let's not sweat it on this PR. UPDATE: I fixed it on this PR and will address it elsewhere on another PR.

@guardrex
Copy link
Collaborator Author

@BrennanConroy ... Are you OOF? No rush ... just checking.

@guardrex
Copy link
Collaborator Author

@BrennanConroy ... Ok ... ready for another 👁️.

@guardrex guardrex merged commit 1eb1c97 into main Sep 27, 2022
@guardrex guardrex deleted the guardrex-patch-1 branch September 27, 2022 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn on MaximumReceiveMessageSize size (DOS risk)
3 participants