Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098263002:
[Offline Page]Making mhtml file recognized as openable by Chrome (Closed)
Description[Offline Page]Making mhtml file recognized as openable by Chrome.
There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file.
This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes.
First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager).
Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender).
The last filter added [mimeType=”*/*”], enables ShareIt open the file.
Deleting either of the intent filter will cause at least one scheme not working properly.
Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753.
BUG=622139
Committed: https://crrev.com/6d63d246b893f5033749391030e8a36172356c3a
Cr-Commit-Position: refs/heads/master@{#408242}
Patch Set 1 #Patch Set 2 : Enable open with mimetype and extension #Patch Set 3 : Enable open file from ShareIt #
Total comments: 6
Patch Set 4 : Add support to mht and eml #
Total comments: 8
Patch Set 5 : Make schemes explicit #Messages
Total messages: 34 (16 generated)
Description was changed from ========== [Offline Page]Making mhtml file recognized as archiveable BUG=622139 ========== to ========== [Offline Page]Making mhtml file recognized as archiveable BUG=622139 ==========
Description was changed from ========== [Offline Page]Making mhtml file recognized as archiveable BUG=622139 ========== to ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. BUG=622139 ==========
weiran@google.com changed reviewers: + dewittj@chromium.org, fgorski@chromium.org
ptal
https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:193: <intent-filter> nit: I don't think this file should have tabs in it. https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:200: <data android:pathPattern=".*\\.mhtml"/> also maybe include .mht and .eml? https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:202: <intent-filter> duplicate section?
https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:193: <intent-filter> On 2016/07/25 20:13:26, dewittj wrote: > nit: I don't think this file should have tabs in it. Done. https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:200: <data android:pathPattern=".*\\.mhtml"/> On 2016/07/25 20:13:26, dewittj wrote: > also maybe include .mht and .eml? I added .mht and .eml. Not sure if .eml is actually supported. I tried both change file extension from .mhtml to .eml and download an sample .eml file, and opened them. There is no problem with .mht files. But instead of opening .eml file, Chrome would trigger download. Do you know what might cause the issue? I have the same problem opening .mhtml file from Android's storage manager. https://codereview.chromium.org/2098263002/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:202: <intent-filter> On 2016/07/25 20:13:26, dewittj wrote: > duplicate section? These two sections differ in the line <data android:mimeType="*/*"/>. Xender would only work with the second filter while ShareIt would only work with the third one.
Description was changed from ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. BUG=622139 ========== to ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753. BUG=622139 ==========
fgorski@chromium.org changed reviewers: + tedchoc@chromium.org
lgtm on my end, Ted, could you help us, by either reviewing or routing to the right person?
tedchoc@chromium.org changed reviewers: + palmer@chromium.org
Adding palmer@ from security (please reroute if someone is a better assignee for this!). No idea if these file formats are trustworthy from any arbitrary source and it seems like something someone from security should have the final say over (I can stamp final approval if security signs off). https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:190: <category android:name="android.intent.category.DEFAULT" /> seems odd that this is the one VIEW handler that lacks BROWSABLE...any idea why that is? https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> why did this drop the file restriction? I see in the bug comment that this handles certain cases, but what are the schemes that are used in those?
https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:190: <category android:name="android.intent.category.DEFAULT" /> On 2016/07/26 17:01:47, Ted C wrote: > seems odd that this is the one VIEW handler that lacks BROWSABLE...any idea why > that is? Not really sure about this...seems like this piece of code was there from the very beginning. https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> On 2016/07/26 17:01:47, Ted C wrote: > why did this drop the file restriction? I see in the bug comment that this > handles certain cases, but what are the schemes that are used in those? Google Drive, Gmail and Android's Download use content scheme.
https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> On 2016/07/26 17:12:59, Vivian wrote: > On 2016/07/26 17:01:47, Ted C wrote: > > why did this drop the file restriction? I see in the bug comment that this > > handles certain cases, but what are the schemes that are used in those? > > Google Drive, Gmail and Android's Download use content scheme. Should we specify file || content then like the below filters?
https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> On 2016/07/26 17:17:09, Ted C wrote: > On 2016/07/26 17:12:59, Vivian wrote: > > On 2016/07/26 17:01:47, Ted C wrote: > > > why did this drop the file restriction? I see in the bug comment that this > > > handles certain cases, but what are the schemes that are used in those? > > > > Google Drive, Gmail and Android's Download use content scheme. > > Should we specify file || content then like the below filters? In Android's developer guide (https://developer.android.com/guide/topics/manifest/data-element.html) it says: "If the filter has a data type set (the mimeType attribute) but no scheme, the content: and file: schemes are assumed." But I guess specifying the schemes would make it less confusing.
https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> On 2016/07/26 17:21:37, Vivian wrote: > On 2016/07/26 17:17:09, Ted C wrote: > > On 2016/07/26 17:12:59, Vivian wrote: > > > On 2016/07/26 17:01:47, Ted C wrote: > > > > why did this drop the file restriction? I see in the bug comment that > this > > > > handles certain cases, but what are the schemes that are used in those? > > > > > > Google Drive, Gmail and Android's Download use content scheme. > > > > Should we specify file || content then like the below filters? > > In Android's developer guide > (https://developer.android.com/guide/topics/manifest/data-element.html) it says: > "If the filter has a data type set (the mimeType attribute) but no scheme, the > content: and file: schemes are assumed." > But I guess specifying the schemes would make it less confusing. Ah, didn't know that. Not sure whether it is worth specifying then. It is nice for clarity, but that is only because I'm not intimately familiar with intent-filters.
On 2016/07/26 17:24:02, Ted C wrote: > Ah, didn't know that. Not sure whether it is worth specifying then. It is nice > for clarity, but that is only because I'm not intimately familiar with > intent-filters. I'm not sure how many people in Chrome are intimately familiar with them.. so I'd prefer either explicit schemes or a comment.
https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2098263002/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:191: <data android:mimeType="multipart/related"/> On 2016/07/26 17:24:02, Ted C wrote: > On 2016/07/26 17:21:37, Vivian wrote: > > On 2016/07/26 17:17:09, Ted C wrote: > > > On 2016/07/26 17:12:59, Vivian wrote: > > > > On 2016/07/26 17:01:47, Ted C wrote: > > > > > why did this drop the file restriction? I see in the bug comment that > > this > > > > > handles certain cases, but what are the schemes that are used in those? > > > > > > > > Google Drive, Gmail and Android's Download use content scheme. > > > > > > Should we specify file || content then like the below filters? > > > > In Android's developer guide > > (https://developer.android.com/guide/topics/manifest/data-element.html) it > says: > > "If the filter has a data type set (the mimeType attribute) but no scheme, the > > content: and file: schemes are assumed." > > But I guess specifying the schemes would make it less confusing. > > Ah, didn't know that. Not sure whether it is worth specifying then. It is nice > for clarity, but that is only because I'm not intimately familiar with > intent-filters. Uploaded a new patch, making the schemes explicit.
lgtm
I've pinged the security team. Will get back to you ASAP.
OK, LGTM. Thanks!
The CQ bit was checked by weiran@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (rubber stamp)
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2098263002/#ps80001 (title: "Make schemes explicit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753. BUG=622139 ========== to ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753. BUG=622139 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753. BUG=622139 ========== to ========== [Offline Page]Making mhtml file recognized as openable by Chrome. There are several scenarios that user want to open an mhtml file of offline page: from third party apps when user receive the file as attachment, from Android’s download home, and from file manager apps. The third party apps that offline page sharing feature intended to make use of are Gmail, ShareIt, Xender, Beam and Drive. Each of these schemes differs in the intent sent out when trying to open the mhtml file. This CL made changes on the intent filters so that Chrome would be known to the system as a handler of mhtml VIEW intent in different schemes. First filter only declares mimeType, enables intent that keeps the mimeType "multipart/related" (Beam,Drive,Download, File Manager). Second filter declares the pathPatter, enables intent that lost the mimeType but keeps the ".mhtml" postfix (Xender). The last filter added [mimeType=”*/*”], enables ShareIt open the file. Deleting either of the intent filter will cause at least one scheme not working properly. Drive and Download use content uri, which is not considered as local url. In MHTMLArchive::create, mhtml pages from non-local URLs are blocked for security reasons. As a result, content won't show properly. Related discussion can be seen in bug 630753. BUG=622139 Committed: https://crrev.com/6d63d246b893f5033749391030e8a36172356c3a Cr-Commit-Position: refs/heads/master@{#408242} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6d63d246b893f5033749391030e8a36172356c3a Cr-Commit-Position: refs/heads/master@{#408242}
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
