InputFile follow-up #25248

MackinnonBuck · 2020-08-25T18:46:46Z

This PR addresses feedback from the API and security reviews for #24640.

Also moved InputFile from M.A.Components.Web.Extensions to M.A.Components.Forms.

Ask-mode template

Description

This PR addresses feedback from the API and security reviews for #24640.

Customer Impact

If we don't do this, we'll ship APIs in RC1 that we immediately intend to break in RC2. Additionally, not doing it means we won't have the desired security characteristics.

Regression?

No, this is a new feature

Risk

This change reduces risk because it reflects security/API review outcomes.

src/Components/Web.Extensions/src/InputFile/IBrowserFile.cs

src/Components/Web.Extensions/src/InputFile/InputFile.cs

pranavkm · 2020-08-26T21:48:34Z

src/Components/Web.Extensions/src/InputFile/InputFileChangeEventArgs.cs

+        {
+            get
+            {
+                ValidateFiles();


Could we do this in InputFile.NotifyChange? That way the component is responsible for the code and this is a POCO type

@pranavkm How would the developer catch this exception? NotifyChange is initiated from a JS callback.

SteveSandersonMS · 2020-08-28T10:50:18Z

...s/ProtectedBrowserStorage/src/Microsoft.AspNetCore.Components.ProtectedBrowserStorage.csproj

+
+  <PropertyGroup>
+    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
+    <Description>A collection of Blazor components for the web.</Description>


Suggested change

<Description>A collection of Blazor components for the web.</Description>

<Description>Provides functionality for storing protected data using the browser's localStorage and sessionStorage APIs.</Description>

SteveSandersonMS · 2020-08-28T10:51:46Z

src/Components/Web/src/Forms/InputFile/IBrowserFile.cs

        /// Opens the stream for reading the uploaded file.
        /// </summary>
+        /// <param name="maxAllowedSize">
+        /// The maximum size that can be read from the Stream. Defaults to 500kb.


Suggested change

/// The maximum size that can be read from the Stream. Defaults to 500kb.

/// The maximum number of bytes that can be supplied by the Stream. Defaults to 500 KB.

SteveSandersonMS · 2020-08-28T10:52:19Z

src/Components/Web/src/Forms/InputFile/IBrowserFile.cs

+        /// The maximum size that can be read from the Stream. Defaults to 500kb.
+        /// <para>
+        /// Calling <see cref="OpenReadStream(long, CancellationToken)"/>
+        /// will throw if the uploaded file size, as specified by <see cref="Size"/> is larger than


Suggested change

/// will throw if the uploaded file size, as specified by <see cref="Size"/> is larger than

/// will throw if the file's size, as specified by <see cref="Size"/> is larger than

I recommend we avoid using the term "upload" anywhere, since (1) the file hasn't actually been transferred by the time this method runs, and (2) in the wasm case, the file doesn't necessarily leave the user's computer at all.

People have been confused into thinking this is about HTTP uploads, but it's not - it's about making file data readable by .NET code.

SteveSandersonMS · 2020-08-28T10:53:24Z

src/Components/Web/src/Forms/InputFile/IBrowserFile.cs

+        /// <para>
+        /// Calling <see cref="OpenReadStream(long, CancellationToken)"/>
+        /// will throw if the uploaded file size, as specified by <see cref="Size"/> is larger than
+        /// <paramref name="maxAllowedSize"/>. By default, uploading a file larger than 500kb will result in an exception.


Suggested change

/// <paramref name="maxAllowedSize"/>. By default, uploading a file larger than 500kb will result in an exception.

/// <paramref name="maxAllowedSize"/>. By default, if the user supplies a file larger than 500 KB, this method will throw an exception.

Switching to more active voice to clarify who are the actors that supply files and throw exceptions.

SteveSandersonMS · 2020-08-28T10:57:17Z

src/Components/ProtectedBrowserStorage/src/Properties/AssemblyInfo.cs

+using System.Runtime.Versioning;
+
+[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.ProtectedBrowserStorage.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
+[assembly: UnsupportedOSPlatform("browser")]


This is super neat - I love it

SteveSandersonMS · 2020-08-28T11:08:30Z

src/Components/Web/src/Forms/InputFile/IBrowserFile.cs

+        /// When specified, it is important to choose a value based on the largest size the calling code is expected to handle.
+        /// Specifying an arbitrary large value may by a vector for resource exhaustion attacks in particular in Blazor Server.


Suggest avoiding the term "vector" as it's a bit jargony outside security circles. Here's an attempt to phrase it more approachably for beginners, but please feel free to use your own description if you prefer!

Suggested change

/// When specified, it is important to choose a value based on the largest size the calling code is expected to handle.

/// Specifying an arbitrary large value may by a vector for resource exhaustion attacks in particular in Blazor Server.

/// It is valuable to choose a size limit that corresponds to your use case. If you allow excessively large files, this

/// may result in excessive consumption of memory or disk/database space, depending on what your code does

/// with the supplied Stream.

/// </para>

/// <para>

/// For Blazor Server in particular, beware of reading the entire stream into a memory buffer unless you have

/// passed a suitably low size limit, since you will be consuming that memory on the server.

SteveSandersonMS · 2020-08-28T11:09:57Z

src/Components/Web/src/Forms/InputFile/IBrowserFile.cs

+        /// </param>
        /// <param name="cancellationToken">A cancellation token to signal the cancellation of streaming file data.</param>
-        Stream OpenReadStream(CancellationToken cancellationToken = default);
+        /// <exception cref="IOException">Thrown if the Stream</exception>


Suggested change

/// <exception cref="IOException">Thrown if the Stream</exception>

/// <exception cref="IOException">Thrown if the file's length exceeds the <paramref name="maxAllowedSize"/> value.</exception>

SteveSandersonMS · 2020-08-28T11:11:49Z

src/Components/Web/src/Forms/InputFile.cs

+        /// Gets or sets the maximum allowed file size in bytes. Defaults to 15 MB.
+        /// </summary>
+        [Parameter]
+        public long MaxFileSize { get; set; } = 1024 * 1024 * 15;


@pranavkm Are you removing this now the check is done on OpenReadStream?

SteveSandersonMS · 2020-08-28T11:14:31Z

src/Components/Web/src/Forms/InputFile.cs

+        /// Gets or sets the maximum allowed number of files. Defaults to 3.
+        /// </summary>
+        [Parameter]
+        public int MaxAllowedFiles { get; set; } = 3;


3 is an awkward default because a developer might realistically test uploading 2 or 3 files and believe that they support an arbitrary number. Personally I think that either:

We should change this to a much higher value just to catch extreme edge cases, rather than interfering with typical use cases. For example, to 100. Then we reason that typical change-handlers are going to process the files in series, not in parallel, so it's not increasing the total amount of memory used if you're reading files into memory sequentially. It just affects total storage space used if you are dumping these to disk/DB, but that's the same as for any other case where a user can write stuff to disk/DB - they can typically do it arbitrarily many times anyway, unless the developer writes logic to impose limits (which is the same here).

Or we should change it to 1, so developers who intend to support multiple file uploads will certainly hit it in their most basic local verification, and then know they have to pick a value themselves.

SteveSandersonMS · 2020-08-28T11:19:24Z

src/Components/Web/src/Forms/InputFile/BrowserFileExtensions.cs

+    public static class BrowserFileExtensions
+    {
+        /// <summary>
+        /// Converts the current image file to a new one of the specified file type and maximum file dimensions.


Suggested change

/// Converts the current image file to a new one of the specified file type and maximum file dimensions.

/// Attempts to convert the current image file to a new one of the specified image format and maximum dimensions.

/// Caution: there is no guarantee that the file will be converted, or will even be a valid image file at all, either

/// before or after conversion. The conversion is requested within the browser before it is transferred to .NET

/// code. The resulting data should be treated as untrusted.

SteveSandersonMS · 2020-08-28T16:00:33Z

Hey @pranavkm, sorry for the scope creep, but @javiercn and I discussed this earlier today and considered the MaxAllowedFiles parameter in some detail. What would you think about keeping the MaxAllowedFiles parameter, defaulting to 10, and making it raise a validation error if the user selects too many files? The reasoning was:

If there's no limit, then if a naive developer does something like "read all the data from all files into a List<byte[]>" then it's trivial for the user to consume arbitrary amounts of memory.
Having some kind of limit will be desirable for common UX flows for application-specific reasons. In this case developers will commonly have to figure out how to show a validation error if the file count limit is exceeded. If we make this a built-in thing, it's very helpful in that case, and developers who don't want it can trivially raise the limit.

If you want, I'm happy to implement that myself. Or if you want to do it, or if you disagree with the design, those are fine too. If we do this, I'd recommend we don't require the InputFile to be within a form (e.g., have a cascaded edit context). Then:

If no edit context is given, then if the count limit is exceeded, we throw instead of calling the change event
If an edit context is given, then if the count limit is exceeded, we register a validation message instead of calling the change event. The validation message would need to be settable as an optional parameter (e.g., FileCountLimitExceededMessage), for example just like how InputNumber registers a validation message if the number isn't parseable and can be controlled via [Parameter] ParsingErrorMessage.

I know this is quite scope-creepy. If you don't feel this is a good direction for us (or if you just want to punt it to RC2, maybe for someone else to implement) please say so!

SteveSandersonMS · 2020-08-28T16:03:52Z

The alternate approach would be replacing .Files with .GetFiles(maxCount) that throws so the developer can choose to catch it if they want, and then they can use their own technique to show a validation error message.

pranavkm · 2020-08-28T18:04:36Z

src/Components/Web/src/Forms/InputFile/InputFileChangeEventArgs.cs

Can this really happen?

I've been trying to work that out for the last little while, and it seems like no. I thought the user might be able to do some obscure gesture like focusing and pressing delete to clear the file input, but no gesture I've tried so far has that effect. The HTML spec doesn't seem to address this question directly.

Since there's no indication from my research that it's possible, I'll change this case to throw on the grounds that it's not meant to be possible. If somehow it does become possible in the future it would be awkward for us though - either we'd need a breaking change, or our API here would be kind of messy (you'd have to check the file count before accessing the File property).

pranavkm · 2020-08-28T18:05:14Z

src/Components/Web/src/Forms/InputFile/InputFileChangeEventArgs.cs

GetFiles? This feels like a weird name.

Counter offer: GetMultipleFiles

I'm just trying to make it clear that you're only meant to use this if your scenario involves multiple.

That sounds reasonable.

SteveSandersonMS · 2020-08-28T18:16:29Z

Note: the reason I'm going with the GetMultipleFiles-which-may-throw instead of a validation message is that it would just be weird and surprising that the throw-or-not decision would be based on whether you're inside an edit context or not.

SteveSandersonMS

I'm approving everyone else's changes in this PR. @pranavkm, can you also post your approval here if you're happy with my changes?

* Move ProtectedBrowserStorage to it's own package * Mark Components.Web.Extensions as non-shipping until we can move it over * Allow InputFile.OpenReadStreamAsync to specify an expected file size.

pranavkm · 2020-08-28T18:58:37Z

src/Components/Web/src/Forms/InputFile/InputFileChangeEventArgs.cs

+        public IBrowserFile File => _files.Count switch
+        {


ghost · 2020-08-28T19:03:53Z

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

src/Components/Web.Extensions/src/Microsoft.AspNetCore.Components.Web.Extensions.csproj

Pilchie · 2020-08-28T21:22:23Z

Thanks - approved for RC1, pending CI completion.

ghost added the area-blazor Includes: Blazor, Razor Components label Aug 25, 2020

MackinnonBuck mentioned this pull request Aug 25, 2020

InputFile Component #24640

Merged

MackinnonBuck marked this pull request as ready for review August 26, 2020 21:32

MackinnonBuck requested review from a team and SteveSandersonMS as code owners August 26, 2020 21:32

pranavkm reviewed Aug 26, 2020

View reviewed changes

src/Components/Web.Extensions/src/InputFile/IBrowserFile.cs Outdated Show resolved Hide resolved

pranavkm reviewed Aug 26, 2020

View reviewed changes

pranavkm requested a review from a team August 27, 2020 21:09

pranavkm added this to the 5.0.0-rc1 milestone Aug 27, 2020

SteveSandersonMS reviewed Aug 28, 2020

View reviewed changes

pranavkm force-pushed the t-mabuc/input-file-followup branch 2 times, most recently from ea9df71 to 2f92e84 Compare August 28, 2020 14:03

pranavkm reviewed Aug 28, 2020

View reviewed changes

SteveSandersonMS approved these changes Aug 28, 2020

View reviewed changes

MackinnonBuck added 4 commits August 28, 2020 19:45

Changes from API review

a4c3052

Feedback from security review

1cdfdc1

Update IBrowserFile.cs

444174f

Updated documentation.

9350beb

MackinnonBuck and others added 8 commits August 28, 2020 19:46

Moved InputFile to M.A.Components.Forms

b62434a

More changes

b25daf4

* Move ProtectedBrowserStorage to it's own package * Mark Components.Web.Extensions as non-shipping until we can move it over * Allow InputFile.OpenReadStreamAsync to specify an expected file size.

Fix E2E tests

e8bbcd8

CR: Throw if user supplies too many files.

adaf7a1

Another build fix

0cb3d5a

CR: Zero files is not a scenario

9357d8f

Update E2E tests

d4464a2

Update JS binaries after rebase

1abc8a6

SteveSandersonMS force-pushed the t-mabuc/input-file-followup branch from 3e4d040 to 1abc8a6 Compare August 28, 2020 18:48

pranavkm approved these changes Aug 28, 2020

View reviewed changes

SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label Aug 28, 2020

Pilchie reviewed Aug 28, 2020

View reviewed changes

src/Components/Web.Extensions/src/Microsoft.AspNetCore.Components.Web.Extensions.csproj Show resolved Hide resolved

Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Aug 28, 2020

Update test

7558ef1

mkArtakMSFT merged commit c2f9793 into release/5.0 Aug 31, 2020

mkArtakMSFT deleted the t-mabuc/input-file-followup branch August 31, 2020 16:07

mkArtakMSFT mentioned this pull request Aug 31, 2020

FileUpload follow ups #25282

Closed

	<Description>A collection of Blazor components for the web.</Description>
	<Description>Provides functionality for storing protected data using the browser's localStorage and sessionStorage APIs.</Description>

	/// The maximum size that can be read from the Stream. Defaults to 500kb.
	/// The maximum number of bytes that can be supplied by the Stream. Defaults to 500 KB.

	/// will throw if the uploaded file size, as specified by <see cref="Size"/> is larger than
	/// will throw if the file's size, as specified by <see cref="Size"/> is larger than

	/// <paramref name="maxAllowedSize"/>. By default, uploading a file larger than 500kb will result in an exception.
	/// <paramref name="maxAllowedSize"/>. By default, if the user supplies a file larger than 500 KB, this method will throw an exception.

		/// When specified, it is important to choose a value based on the largest size the calling code is expected to handle.
		/// Specifying an arbitrary large value may by a vector for resource exhaustion attacks in particular in Blazor Server.

-        /// When specified, it is important to choose a value based on the largest size the calling code is expected to handle.
-        /// Specifying an arbitrary large value may by a vector for resource exhaustion attacks in particular in Blazor Server.
+        /// It is valuable to choose a size limit that corresponds to your use case. If you allow excessively large files, this
+        /// may result in excessive consumption of memory or disk/database space, depending on what your code does
+        /// with the supplied Stream.
+        /// </para>
+        /// <para>
+        /// For Blazor Server in particular, beware of reading the entire stream into a memory buffer unless you have
+        /// passed a suitably low size limit, since you will be consuming that memory on the server.

	/// <exception cref="IOException">Thrown if the Stream</exception>
	/// <exception cref="IOException">Thrown if the file's length exceeds the <paramref name="maxAllowedSize"/> value.</exception>

-        /// Converts the current image file to a new one of the specified file type and maximum file dimensions.
+        /// Attempts to convert the current image file to a new one of the specified image format and maximum dimensions.
+        /// Caution: there is no guarantee that the file will be converted, or will even be a valid image file at all, either
+        /// before or after conversion. The conversion is requested within the browser before it is transferred to .NET
+        /// code. The resulting data should be treated as untrusted.

InputFile follow-up #25248

InputFile follow-up #25248

Uh oh!

Conversation

MackinnonBuck commented Aug 25, 2020 • edited by SteveSandersonMS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ask-mode template

Description

Customer Impact

Regression?

Risk

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS commented Aug 28, 2020

Uh oh!

SteveSandersonMS commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS commented Aug 28, 2020

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 28, 2020

Uh oh!

Uh oh!

Pilchie commented Aug 28, 2020

Uh oh!

MackinnonBuck commented Aug 25, 2020 •

edited by SteveSandersonMS

Loading

MackinnonBuck Aug 26, 2020 •

edited

Loading

SteveSandersonMS Aug 28, 2020 •

edited

Loading

SteveSandersonMS Aug 28, 2020 •

edited

Loading

SteveSandersonMS Aug 28, 2020 •

edited

Loading

SteveSandersonMS Aug 28, 2020 •

edited

Loading