avm1: Partially implement FileReference (Part of #307) #5086

hatal175 · 2021-08-17T11:58:10Z

I've implemented mostly just the browse function which opens a file dialog. The download and upload have security implications, so I'm not sure how to take them into consideration (if the framework is implemented at all).

For the dialog, I've added the rfd crate. It allows for asynchronous file dialogs which the tiny dialog crate don't have, and are part of the spec according the docs.

midgleyc · 2021-08-17T15:42:02Z

The download and upload have security implications, so I'm not sure how to take them into consideration (if the framework is implemented at all).

At the moment, I think we ignore the security framework (e.g. on LoadVars or XML).

I think that this doesn't matter so much: download is effectively a GET of a given URL, and upload a POST: if a user can do these inside Flash, they can also do them outside Flash, so I don't find it terribly concerning if Ruffle just does these ignoring the sandboxing.

hatal175 · 2021-08-17T17:05:14Z

If you consider it ok, I can implement the other ones as well for the pull request.

…

On Tue, Aug 17, 2021, 18:42 Chris Midgley ***@***.***> wrote: The download and upload have security implications, so I'm not sure how to take them into consideration (if the framework is implemented at all). At the moment, I think we ignore the security framework (e.g. on LoadVars or XML). I think that this doesn't matter so much: download is effectively a GET of a given URL, and upload a POST: if a user can do these inside Flash, they can also do them outside Flash, so I don't find it terribly concerning if Ruffle just does these ignoring the sandboxing. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5086 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYQXXPEPHPAAQNJ46JRQFTT5J7NLANCNFSM5CJWOB3A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

midgleyc · 2021-08-17T17:12:06Z

I think that would be nice :)

In terms of SWFs in the wild using this: I know of one web-based management interface that uses upload (first parameter only). I don't know of anything using download, but I wouldn't be surprised to see it.

midgleyc

Comments on the desktop file selector also apply to the web one. A lot of the code is shared: can it be moved up a level somehow?

core/src/avm1/globals.rs

core/src/loader.rs

desktop/src/ui.rs

kmeisthax · 2021-08-17T22:31:10Z

Regarding the security checks: our intention on web is to not open up any new web attack surface, so on the web we don't need to run the same security checks for download or upload since we're not bothering with policy files or anything. browse is equivalent to populating a file upload input on web and is perfectly fine.

On desktop the story changes a little. Flash movies on desktop inherently don't come with an origin. The original security model for local file access was web-only or filesystem-only, controlled with some movie metadata. This is a restriction we absolutely don't implement right now. When we implement download or upload we may want to consider changing ExternalNavigatorBackend so that it can be locked one way or the other.

hatal175 · 2021-08-20T20:31:00Z

Comments on the desktop file selector also apply to the web one. A lot of the code is shared: can it be moved up a level somehow?

Well, technically yes, but no :) I could put in core but there are no other implementations of obviously ui code there... if other people also think so, I can move it.

hatal175 · 2021-08-21T07:17:11Z

I don't think I'm going to implement the download/upload pair in this pull request. After an answer to the duplication of the ui code, I'll rebase this and consider this complete for now.

midgleyc · 2021-08-21T07:22:16Z

You have a leftover log::warn!("{}", extensions[0]); in desktop/src/ui.rs, other than that looks good to me.

core/src/avm1/object/file_reference.rs

adrian17 · 2021-09-05T12:43:28Z

For the record, the web popup looks like this:

adrian17 · 2021-09-05T12:48:47Z

I think the PR is good. One question from reading the docs:

Only one browse() session can be performed at a time (because only one dialog box can be displayed at a time).

Do we have a guard against that? What would happen if we called browse twice?
I don't think it's an issue on web (rfd shows a popup that makes it impossible to trigger new AS click events, right?), but not sure about desktop. If it's impossible on desktop too, then it's cool.

Also notes for the future:

Because of new functionality added to Flash Player, when publishing to Flash Player 10, you can have only one of the following operations active at one time: FileReference.browse(), FileReference.upload(), FileReference.download(). Otherwise, Flash Player throws a runtime error (code 2174). Use FileReference.cancel() to stop an operation in progress.

We'll probably need a guard against that, when we get upload/download.

In Flash Player 10 and later, you can call this method successfully only in response to a user event (for example, in an event handler for a mouse click or keypress event). Otherwise, calling this method results in Flash Player throwing an Error.

Ideally we should implement this at some point, but it's probably not a big immediate risk - since FP blocked this behavior, I don't think people bothered to even try to trigger this without user event.

adrian17

(I'd appreciate if kmeisthax looked at the async portion too, but I think it's mergeable as-is)

Herschel · 2021-09-11T20:38:18Z

Since this uses rfd, can we also use rfd for the initial Choose SWF dialog on desktop and remove tinyfiledialog? It feels odd to have two separate file dialog crates.

Herschel

Thank you!

Herschel · 2021-09-11T21:03:27Z

core/src/avm1/value.rs

+impl<'gc> From<u64> for Value<'gc> {
+    fn from(value: u64) -> Self {
+        Value::Number(value as f64)
+    }
+}
+


I get nervous about these kind of conversions, because u64 -> f64 can be lossy and from::From by convention should be a lossless conversion. But we already have From<usize>, so this is fine for now.

Herschel · 2021-09-11T21:19:09Z

desktop/src/ui.rs

+
+impl DesktopFileDialogResult {
+    pub fn new(handle: Option<FileHandle>) -> Self {
+        let md = handle.as_ref().and_then(|x| metadata(x.path()).ok());


style nit: Let's import fs::self and then do fs::metadata (makes it a little clearer that it's the standard metadata fn and not our own fn.)

Herschel · 2021-09-11T21:25:19Z

web/src/ui.rs

+    #[cfg(not(target_arch = "wasm32"))]
+    fn creation_time(&self) -> Option<DateTime<Utc>> {
+        unreachable!();
+    }


Let's make two separate impl blocks and #[cfg(not(target_arch = "wasm32"))] on the impl block, instead of doing #[cfg] on each method.

There are functions that should be in both configs and functions that should be in just one. Since I can't do two trait implemenrations, it means I have to duplicate the functions that are in both... advice?

Herschel · 2021-09-11T21:40:45Z

Also, looks like there's an undocumented FileReference.convertToPPT and deleteConvertedPPT, at least in the desktop player(?):

ASSetPropFlags(flash.net.FileReference.prototype, null, 6, 1);
for(var k in flash.net.FileReference.prototype) {
	trace(k);
}

No clue what they do, but we should at least mention these in a comment and/or possibly stub them out.

hatal175 · 2021-09-12T10:13:17Z

I've fixed most of the issues you've raised except the cfg thing.

I've removed tinyfiledialogs from the license - I hope that's ok.
~~rfd can't set file dialog title for now - I have a pending pull request:~~ Add set_title to FileDialog PolyMeilex/rfd#23. Pull request was merged and title was readded.
I chose to add a comment since there's no real mention of those functions anywhere. It's probably used internally by flash somewhere so no real reason to stub.
Also updated rfd crate while I'm at it.

PolyMeilex · 2021-09-12T21:33:46Z

For the record, the web popup looks like this:

I intentionally designed it in a way that is easy to customize, so you don't have to stick to that in case you don't like it, you can just provide CSS styles for IDs that RFD is using rfd-overlay,rfd-card,rfd-input,rfd-button

Default style can be used as a reference: https://github.com/PolyMeilex/rfd/blob/master/src/backend/wasm/style.css

midgleyc suggested changes Aug 17, 2021

View reviewed changes

core/src/avm1/globals.rs Outdated Show resolved Hide resolved

core/src/loader.rs Outdated Show resolved Hide resolved

desktop/src/ui.rs Outdated Show resolved Hide resolved

desktop/src/ui.rs Show resolved Hide resolved

hatal175 force-pushed the filereference branch 2 times, most recently from deb1592 to 563b6a8 Compare August 22, 2021 03:55

adrian17 reviewed Aug 30, 2021

View reviewed changes

core/src/avm1/object/file_reference.rs Outdated Show resolved Hide resolved

hatal175 force-pushed the filereference branch from 563b6a8 to a587fc1 Compare August 31, 2021 16:00

adrian17 approved these changes Sep 5, 2021

View reviewed changes

Herschel requested changes Sep 11, 2021

View reviewed changes

hatal175 force-pushed the filereference branch from a587fc1 to a53ca44 Compare September 12, 2021 10:10

hatal175 force-pushed the filereference branch from a53ca44 to a8c0b4d Compare September 12, 2021 10:23

hatal175 force-pushed the filereference branch from a8c0b4d to 017daa4 Compare September 12, 2021 22:19

hatal175 requested a review from Herschel September 14, 2021 15:35

hatal175 force-pushed the filereference branch from 017daa4 to 8ee8200 Compare September 18, 2021 23:09

avm1: Partially implement FileReference (Part of ruffle-rs#307)

bde69ab

hatal175 force-pushed the filereference branch from 8ee8200 to bde69ab Compare September 18, 2021 23:29

midgleyc mentioned this pull request Feb 13, 2022

desktop: swap out tinyfiledialogs for rfd #6265

Merged

hatal175 closed this Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avm1: Partially implement FileReference (Part of #307) #5086

avm1: Partially implement FileReference (Part of #307) #5086

hatal175 commented Aug 17, 2021

midgleyc commented Aug 17, 2021

hatal175 commented Aug 17, 2021 via email

midgleyc commented Aug 17, 2021

midgleyc left a comment

kmeisthax commented Aug 17, 2021

hatal175 commented Aug 20, 2021 •

edited

hatal175 commented Aug 21, 2021

midgleyc commented Aug 21, 2021

adrian17 commented Sep 5, 2021

adrian17 commented Sep 5, 2021 •

edited

adrian17 left a comment •

edited

Herschel commented Sep 11, 2021

Herschel left a comment

Herschel Sep 11, 2021

Herschel Sep 11, 2021

Herschel Sep 11, 2021

hatal175 Sep 12, 2021

Herschel commented Sep 11, 2021

hatal175 commented Sep 12, 2021 •

edited

PolyMeilex commented Sep 12, 2021

avm1: Partially implement FileReference (Part of #307) #5086

avm1: Partially implement FileReference (Part of #307) #5086

Conversation

hatal175 commented Aug 17, 2021

midgleyc commented Aug 17, 2021

hatal175 commented Aug 17, 2021 via email

midgleyc commented Aug 17, 2021

midgleyc left a comment

Choose a reason for hiding this comment

kmeisthax commented Aug 17, 2021

hatal175 commented Aug 20, 2021 • edited

hatal175 commented Aug 21, 2021

midgleyc commented Aug 21, 2021

adrian17 commented Sep 5, 2021

adrian17 commented Sep 5, 2021 • edited

adrian17 left a comment • edited

Choose a reason for hiding this comment

Herschel commented Sep 11, 2021

Herschel left a comment

Choose a reason for hiding this comment

Herschel Sep 11, 2021

Choose a reason for hiding this comment

Herschel Sep 11, 2021

Choose a reason for hiding this comment

Herschel Sep 11, 2021

Choose a reason for hiding this comment

hatal175 Sep 12, 2021

Choose a reason for hiding this comment

Herschel commented Sep 11, 2021

hatal175 commented Sep 12, 2021 • edited

PolyMeilex commented Sep 12, 2021

hatal175 commented Aug 20, 2021 •

edited

adrian17 commented Sep 5, 2021 •

edited

adrian17 left a comment •

edited

hatal175 commented Sep 12, 2021 •

edited