Closed Bug 1119401 Opened 9 years ago Closed 3 years ago

Images are shared with wrong extension

Categories

(Firefox for Android Graveyard :: General, defect, P5)

51 Branch
ARM
Android
defect

Tracking

(fennec+, firefox56 affected)

RESOLVED INCOMPLETE
Tracking Status
fennec + ---
firefox56 --- affected

People

(Reporter: mirh, Assigned: kimsaehun)

References

()

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141229214612

Steps to reproduce:

Open an image with an "odd" URL, like https://pbs.twimg.com/media/B60OF0vCIAEiLtd.jpg:large

Share it with other applications


Actual results:

I believe image is sent to apps with "jpg:large" extension, which cannot be handled by any of the 5 I tried


Expected results:

Jpg should be the right extension
I tried sharing https://pbs.twimg.com/media/B62ErB9CUAA-zMV.jpg:large to Google Plus, the first application I tried and it shared fine for me in the compose window.
Mentor: rnewman, liuche
OS: Windows 7 → Android
Hardware: x86_64 → All
Whiteboard: [good second bug][lang=java]
I don't use G+, though the program says it cannot display image preview (works when I remove :large)

I even just tried to share with messages app and it only shows the URL. When I remove :large a proper MMS is created
Severity: normal → minor
Hardware: All → ARM
Hello.
AFAIK `:large` is a "browser action". Apps are not supposed to handle it. The browser model is. So if I were to pass a file, I would remove all action requests from the URI. File paths should end with the file extension(if present). All query/action requests ought to be stripped from the URI. 

Possible solution: (IMO) URI should be terminated before `:` or `?` after extension or file-name.

BTW I'd like to work on it.
NI to respond to mentor request in comment 3
Flags: needinfo?(rnewman)
Flags: needinfo?(liuche)
I'm afraid Chenxia's gonna have to tackle the meat of this one; I'm totally underwater. Sorry, Chenxia!
Flags: needinfo?(rnewman)
Mirh: what application were you trying to share to? I tried to repro this as well, but couldn't find an app where the image didn't share - the only one with an image preview was G+ though (and the preview of the post displayed the image).

Abhishek: We don't want to lose the data that :large conveys because the link will eventually be displayed by a browser anyways, so I don't think truncating is the right approach. Colons(:) are valid url component in other links (port, authentication), so our Share functionality shouldn't modify the url.
Flags: needinfo?(mirh)
Flags: needinfo?(liuche)
Yeah, indeed I believe what was said in #3 could fix the problem. 

And I tried to share to whatsapp, telegram, photo editor, meme generator, viber and textsecure (practically everything that could accept images). 

And every one refused the content. 
I'm with kitkat if it can help.
Flags: needinfo?(mirh)
(In reply to mirh from comment #7)
> Yeah, indeed I believe what was said in #3 could fix the problem. 
> 
> And I tried to share to whatsapp, telegram, photo editor, meme generator,
> viber and textsecure (practically everything that could accept images). 
> 
> And every one refused the content. 
> I'm with kitkat if it can help.
Yay, one voice of assent! BTW the STR link in comment#0 is dead. Link in comment #1 is working.
Guess this one's still unconfirmed. BTW what is the repository for Firefox for Android? (I'm pretty sure its NOT mozilla-central)

(In reply to Chenxia Liu [:liuche] from comment #6)
> Mirh: what application were you trying to share to? I tried to repro this as
> well, but couldn't find an app where the image didn't share - the only one
> with an image preview was G+ though (and the preview of the post displayed
> the image).
> 
> Abhishek: We don't want to lose the data that :large conveys because the
> link will eventually be displayed by a browser anyways, so I don't think
> truncating is the right approach. Colons(:) are valid url component in other
> links (port, authentication), so our Share functionality shouldn't modify
> the url.
What information is there to convey? ":large" is an action, not attribute. If someone wants to look at an image in a browser at full resolution, they could simply zoom. Moreover, ":large" is not even a standard action (NOT working on IE-Windows phone, Chromium-GNU Linux, Chrome-Windows 8.1{Certain file types like PNG}), nor is it documented as a standard. Also, passing attributes/queries in URIs to apps is a potential security bomb.
Flags: needinfo?(rnewman)
Flags: needinfo?(mirh)
Flags: needinfo?(liuche)
I don't actually know much about url extensions - wesj, you did some stuff with filetype extensions, do you have any thoughts on this?

(Also clearing rnewman because he said he's too busy to mentor atm.)
Flags: needinfo?(wjohnston)
Flags: needinfo?(rnewman)
Flags: needinfo?(liuche)
> What information is there to convey? ":large" is an action, not attribute.
> If someone wants to look at an image in a browser at full resolution, they
> could simply zoom. Moreover, ":large" is not even a standard action (NOT
> working on IE-Windows phone, Chromium-GNU Linux, Chrome-Windows 8.1{Certain
> file types like PNG}), nor is it documented as a standard. Also, passing
> attributes/queries in URIs to apps is a potential security bomb.

My hesitation about this is the implementation - all we're doing is sharing a link via an Android intent to other apps, and it looks like they aren't handling the link extension properly. Does that mean Firefox should modify the content a user is sharing, because other apps aren't doing the right thing? I'm not sure that's correct, but I'll do a little more research into this.

In the meantime, there are other bugs that are more well-defined, if you want to take a look at them! For instance, bug 924009, or if you're looking for something a little more challenging, bug 1084293.

> BTW what is the repository for Firefox
> for Android? (I'm pretty sure its NOT mozilla-central)

The repository is in fact mozilla-central - the mobile code is under the mobile/android directories.

If you don't have a development environment set up, there's a helpful getting started wiki that you can follow: https://wiki.mozilla.org/Mobile/Fennec/Android
Flags: needinfo?(babhishek21)
(In reply to Chenxia Liu [:liuche] from comment #10)
> > What information is there to convey? ":large" is an action, not attribute.
> > If someone wants to look at an image in a browser at full resolution, they
> > could simply zoom. Moreover, ":large" is not even a standard action (NOT
> > working on IE-Windows phone, Chromium-GNU Linux, Chrome-Windows 8.1{Certain
> > file types like PNG}), nor is it documented as a standard. Also, passing
> > attributes/queries in URIs to apps is a potential security bomb.
> 
> My hesitation about this is the implementation - all we're doing is sharing
> a link via an Android intent to other apps, and it looks like they aren't
> handling the link extension properly. Does that mean Firefox should modify
> the content a user is sharing, because other apps aren't doing the right
> thing? I'm not sure that's correct, but I'll do a little more research into
> this.
> 
> In the meantime, there are other bugs that are more well-defined, if you
> want to take a look at them! For instance, bug 924009, or if you're looking
> for something a little more challenging, bug 1084293.
Oh. I'm in no hurry. :P But thank you for the links. 
> > BTW what is the repository for Firefox
> > for Android? (I'm pretty sure its NOT mozilla-central)
> 
> The repository is in fact mozilla-central - the mobile code is under the
> mobile/android directories.
Oh! I did not know that. 
> If you don't have a development environment set up, there's a helpful
> getting started wiki that you can follow:
> https://wiki.mozilla.org/Mobile/Fennec/Android
I have an environment suitable for desktop testing only. So will take a look here.
Flags: needinfo?(babhishek21)
(In reply to Chenxia Liu [:liuche] from comment #10)
> My hesitation about this is the implementation - all we're doing is sharing
> a link via an Android intent to other apps, and it looks like they aren't
> handling the link extension properly.

Yeah. My worry is that this would break in some cases. i.e. the app would try to download the link and get a different image than the one we expected. We actually download and share the downloaded image as well though, so for most apps it probably doesn't matter.

I can walk you through this a bit though. We send the link info the Java here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#819

That includes both the url and a mimetype (hardcoded to image/* in order to avoid things like this exact problem). That object is made into a PromptListItem in Java here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptListItem.java#119

and where we also make an intent, that is eventually handed to an action provider:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptListAdapter.java#79

which hands it to an ActivityProvider:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java#361

so that intent is probably the one causing misfires. If we wanted, we could alter what we pass to the ActivityChooserModel to NOT include a url at all. Just a mimetype. I'm not sure what will happen if we then send a url to it in the end though.
Flags: needinfo?(wjohnston)
Still a thing in latest 51.0a1, Marshmallow + Whatsapp
Flags: needinfo?(mirh)
Severity: minor → trivial
Version: 37 Branch → 51 Branch
Tested using:
Device: One A2001 (Android 6.0.1)
Build: Firefox for Android 52.0a1 (2016-10-02)

1. Open https://pbs.twimg.com/media/B62ErB9CUAA-zMV.jpg:large
2. Long tap on it and choose share image
-gmail: the image is uploaded with "x.jpg:large" extension
-whatsup: "the file format is not supported" message is displayed

tried the same steps with chrome:
-gmail: the image is uploaded with "x.jpg" extension
-whatsup: no error message is displayed
Wesley,
Any update on this?

I think truncating url after file extension should do the job. As mentioned in comment #15, sharing image with chrome is not causing any problem. So, our browser should send modified url to the app.

If I'm right, I would like to work on this.
Flags: needinfo?(wjohnston2000)
Flags: needinfo?(liuche)
Although one problem is of course that if you remove the ":large" bit, Twitter servers you a lower resolution image.
(In reply to Jan Henning [:JanH] from comment #17)
> servers

That should be "serves" of course.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Hello, I would like to work on this as my second bug.

I did some work based on previous comments and here is what I did:

As per comment #12, I looked at "/mozilla-central/mobile/android/chrome/content/browser.js" and I did as comment #16 stated by truncating the url by modifying the "src" variable.

878:        let src = aTarget.src;
879:        return {
880:          title: src,
881:          uri: src,
882:          type: "image/*",
883:        };

Once I read over on how to properly submit a patch, I will do so.
Comment on attachment 8895108 [details]
Bug 1119401 - Truancate image URIs after file extension.

Bug metadata a little outdated, here.
Flags: needinfo?(wjohnston2000)
Flags: needinfo?(liuche)
Attachment #8895108 - Flags: review?(s.kaspari)
Attachment #8895108 - Flags: review?(rnewman)
Attachment #8895108 - Flags: review?(liuche)
Mentor: liuche, rnewman
Oh, oops. Sorry about that. Thanks for the fix!
tracking-fennec: ? → +
Priority: -- → P2
Assignee: nobody → kimsaehun
Status: NEW → ASSIGNED
Attachment #8895108 - Flags: review?(topwu.tw)
Attachment #8895108 - Flags: review?(s.kaspari)
Attachment #8895108 - Flags: review?(cnevinchen)
Comment on attachment 8895108 [details]
Bug 1119401 - Truancate image URIs after file extension.

https://reviewboard.mozilla.org/r/166218/#review178836

::: mobile/android/chrome/content/browser.js:882
(Diff revision 1)
>          let props = imageCache.findEntryProperties(aTarget.currentURI, doc);
>          let src = aTarget.src;
> +        // trim URI after file extension
> +        let indexOfExtension = src.lastIndexOf(".");
> +        if (indexOfExtension != -1) {
> +          let alphaRegex = /\.[a-zA-Z]*/;

IIUC, we have an assumption that image file extensions only contain alphabet characters, right?

The wiki link list a lot of image formats and some of they do have extension contains not only alphabet but also digit, `CD5` for example. Not sure do we have to take care of this case because it seems rarely seen.

https://en.wikipedia.org/wiki/Image_file_formats
Attachment #8895108 - Flags: review?(topwu.tw) → review+
Oooh, I definitely was under the assumption that file extensions only contain alphabet characters.

Should I fix the code to account for digits in the extension and submit a new patch or wait for further input?
(In reply to kimsaehun from comment #24)
> Oooh, I definitely was under the assumption that file extensions only
> contain alphabet characters.
> 
> Should I fix the code to account for digits in the extension and submit a
> new patch or wait for further input?

I think a new patch considering digits would be great!
So over this weekend, I messed around a bit too much with mercurial and ended up deleting and re-downloading the mozilla-central repository. Although I didn't encounter any problems yesterday, I am currently getting an "Error running mach: ['--log-no-times', 'artifact', 'install']" error when running the `./mach build` command.

Not sure what went wrong in the span of 24 hours but I'll most likely end up submitting the new patch tomorrow. =(
Comment on attachment 8895108 [details]
Bug 1119401 - Truancate image URIs after file extension.

https://reviewboard.mozilla.org/r/166218/#review179866
Attachment #8895108 - Flags: review?(cnevinchen) → review+
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: P2 → P5
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: