InputFile Component

[MackinnonBuck]

Summary

• Migrated InputFile from https://github.com/SteveSandersonMS/BlazorInputFile to M.A.C.Web.Extensions
• Removed dependency on third-party base64 encoding JS code.
• Added IJSUnmarshalledRuntime to allow RemoteFileListEntryStream to make unmarshalled JS invocations without reflection.
• Reimplemented RemoteFileListEntryStream to use System.IO.Pipelines.
• Added E2E tests

Addresses #12205

[SteveSandersonMS]

This looks really great, @MackinnonBuck! Let us know when you want another round of review.

[SteveSandersonMS]

@javiercn You spoke before about doing a security review on the new file upload feature. I don't know if you had something else in mind, but one thing that occurs to me is reviewing this kind of logic to see if we can see any way it could be misused to make the server allocate arbitrary amounts of memory, or to leak memory, to get into an infinite loop, or anything like that.

Do you think that's something we should do directly within this PR review, or schedule something separate (maybe with Levi giving it a look over)?

[SteveSandersonMS]

Again, this looks great 👍

I see there are still a couple of API changes due (e.g., your OpenFileStream suggestion) and E2E test cases remaining so I'll hold off on marking as approved, but as far as I'm concerned all the big questions are answered now and the rest are just small details. Nice work!

[pranavkm]

Can we call this IBrowserFile? The browser API is called File: https://developer.mozilla.org/en-US/docs/Web/API/File

[pranavkm]

Is there a reason we have an interface and implementation? Do we expect users to implement this contract?

[MackinnonBuck]

I believe properties that can be deserialized from JSON must have public setters - correct me if I'm wrong on this. If that's the case, we probably don't need users to be able to tinker with the various properties, hence the internal FileListEntry and the public IFileListEntry that describes those properties as read-only.

[SteveSandersonMS]

IBrowserFile sounds fine to me. In most cases people won't see the type name in their code anyway since they will receive the InputFileChangeEventArgs then just iterate over its Files collection.

I believe properties that can be deserialized from JSON must have public setters - correct me if I'm wrong on this. If that's the case, we probably don't need users to be able to tinker with the various properties, hence the internal FileListEntry and the public IFileListEntry that describes those properties as read-only.

Yes, I agree. However if we wanted we could swap the "public interface, internal class" pair with a "public abstract class, internal concrete subclass" pair. That has the benefit of letting us add more members in the future non-breakingly.

Not a big deal though. Do either of you feel strongly either way?

[MackinnonBuck]

I'm not partial to keeping the interface - I'll go ahead and swap it for an abstract class.

[MackinnonBuck]

Actually, @SteveSandersonMS, I just want to clarify my understanding of what you had in mind. Is the change to enable us to add more properties to IBrowserFile without forcing our customers to modify their own types implementing IBrowserFile? If so, wouldn't default interface implementations solve this? The challenge with turning IBrowserFile into an abstract class is that we can't define properties in the abstract class as read-only but define them in the internal class as read-write. There are workarounds of course, but that seemed messier to me than using default interface implementation.

[javiercn]

@javiercn I believe the maximum file size scenario is covered already with a bit of custom user code. The user receives an IBrowserFile object which contains a Size property that they can look at before calling OpenReadStream

Yes, but do we stop reading after we reach the Size of the file in the implementation? So if I told you I had 1MB of data and tried to send you 2MB, would we stop at Size when we are pulling data from the stream within CopyToAsync?

[MackinnonBuck]

@javiercn Ah, I see what you mean now - I'll add a check for this!

[javiercn]

@javiercn Ah, I see what you mean now - I'll add a check for this!

Nevermind, I think it's already covered https://github.com/dotnet/aspnetcore/pull/24640/files#diff-288a225ef1a4f388d11bacb6bbb83334R49

[pranavkm]

Feel free to respond to my comments in a follow up PR

[pranavkm]

Could we mark this as [SupportedOSPlatform('browser")]? Previously you were unable to get to this contract from a Blazor server app. We have a way to indicate this is never useful in it.

[MackinnonBuck]

Consider increasing the timeout.

[MackinnonBuck]

API changes are being included as part of #25248.

[hannespreishuber]

looking for deep explanation, why imageFile.OpenReadStream(512000,cancellationToken) is limited?

[SteveSandersonMS]

@hannespreishuber Sorry, I don't understand the question. If you think there's a problem or some unexplained behavior, could you please post a new issue detailing repro steps, what you expect to happen, and what happens instead? Thanks!

[hannespreishuber]

The upload limit of 512KB as parameter size. No bigger files? or do I miss something (as there is no doc on it)

[halter73]

What's stopping you from changing the parameter? Is it being called internally somewhere where maxAllowedSize is not configurable? The doc comments look good to me, but I don't think docs.microsoft.com has been updated yet.

[hannespreishuber]

ah my fault. Read that Doc comment as internal limit