Closed Bug 1358946 Opened 7 years ago Closed 7 years ago

Reader mode page is saved as normal bookmark

Categories

(Firefox for Android Graveyard :: Reading List, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Reading List
Type:
defect

Tracking

(fennec+, firefox53 wontfix, firefox54+ verified, firefox55+ verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
fennec + ---
firefox53 --- wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

1358946.patch
4.26 KB, patch
maliu
: review+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Since the update Firefox no longer adds a page in readerview mode to reading list, it simply adds it as a normal bookmark. 


Source:
https://www.reddit.com/r/firefox/comments/66iqgq/firefox_53_no_longer_saves_reader_view_pages_on/


Previously we use the url about:reader?url= to specify if it's a reader mode page and cache the page content. But there's securty concern to show the about:reader?url= in url so they are striped.

We need another way to find if the Tab is reader mode or not other than the url.
[Tracking Requested - why for this release]: regression of popular workflow
Blocks: CVE-2017-5463
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Keywords: regression
Looked into desktop with @evanxd.

Desktop also shows the url without "about:reader?url="
But if you view page info, the url has that.

So I think in DB it's still saved as "about:reader?url=http:/xxxxxxx" but shows the url without "about:reader?url="

I'm gonna test how that works in Fennec.

Maybe just did my last patch did.
Assignee: nobody → cnevinchen
tracking-fennec: ? → 54+
See Also: → 1355448
Group: firefox-core-security, mozilla-employee-confidential
I set the flag cause it's related to previous security patch.
Comment on attachment 8861636 [details]
Bug 1358946 - Fix bug when reader mode page is saved as normal bookmark.

https://reviewboard.mozilla.org/r/133628/#review136774

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:86
(Diff revision 2)
>          filterFlags(intent);
>  
>          startActivity(intent);
>      }
>  
>      private void dispatchCustomTabsIntent() {

Why is this in dispatch"CustomTabs"Intent not dispatch"Normal"Intent?

If "about:reader" prefix is not allowed from external, would it be better if we strip it in onCreate before any dispatch take place?

The activity "BrowserApp" is declare exported as true, which means other app can still sent URL with "about:reader" prefix directly to BrowserApp and bypass this stirpping. Would it be better if we strip it from "onNewIntent" and "onCreate" of BrowserApp or GeckoApp?
Attachment #8861636 - Flags: review?(max) → review-
reader mode regression, tracking for 54/55.
status-firefox56: affected → ---
tracking-firefox54: ? → +
tracking-firefox55: ? → +
tracking-firefox56: ? → ---
We have a fix for bug 1338867 in release does this really need to be a security bug?
Attached file 1358946.patch (obsolete) — 
The bookmark/cache didn't work because Android still need the url to contain "about:reader?url=" to know this page is in reader mode page. Then handle bookmarking & caching correctly. 

For the intent issue, I just stripe 'about:reader?url=' to avoid misuse of the intent. see bug 1338867
Attachment #8862331 - Flags: review?(s.kaspari)
Attachment #8862331 - Flags: review?(max)
Attachment #8861636 - Attachment is obsolete: true
Attachment #8861636 - Flags: review?(s.kaspari)
Attachment #8862331 - Flags: review?(max) → review+
Group: firefox-core-security, mozilla-employee-confidential
Comment on attachment 8862331 [details]
1358946.patch

Review of attachment 8862331 [details]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1167,5 @@
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          GeckoAppShell.ensureCrashHandling();
>  
> +        IntentHelper.stripeDataUri(getIntent());

stripe -> strip

@@ +2184,1 @@
>          final SafeIntent intent = new SafeIntent(externalIntent);

This should probably be done on a SafeIntent object
Attachment #8862331 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #10)

> stripe -> strip
Done 
> @@ +2184,1 @@
> >          final SafeIntent intent = new SafeIntent(externalIntent);
> 
> This should probably be done on a SafeIntent object
Using SafeIntent is good. But I think it's a little bit implicit cause I'll also override the data uri when user call "getIntent()" in the acivity.

Would you please  teach me how to delete the old patch and use the new patch in mozreview? Since Daniel has remove the flags so I guess I should use mozreview now?
Flags: needinfo?(s.kaspari)
Comment on attachment 8862331 [details]
1358946.patch

Oh.. I just found how to make the old patch absolete :)
Attachment #8862331 - Attachment is obsolete: true
Attachment #8862331 - Attachment is patch: false
need review first
Flags: needinfo?(cnevinchen)
Keywords: checkin-needed
Oh... this previously passed review in attachment 8862331 [details]
I just added some fix from comment 10 and submitted again using mozreview.
Flags: needinfo?(cnevinchen)
Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139774

r- because this reintroduces bug 1338867. Intents are just a convenient way to send stuff to the browser, but it's probably possible to convince users to copy/paste things into the URL bar themselves, which works just fine.

We shouldn't stop stripping the URL inside the browser, either - this doesn't just remove the code added in bug 1338867, but also entirely removes the code that changes the displayed URL. This means that about:reader URLs show up in the URL bar as-is (which I confirmed by testing this patch on my local android device), and they shouldn't.
Attachment #8864742 - Flags: review-
Re-reading the comments here: If you rely on the un-stripped URL to make the "is this page in reader mode" determination, you should keep the unchanged URL in addition to the stripped URL, or you should strip the URL from java code before displaying it in the location bar, but this patch does neither of these things.
Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139930

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2183
(Diff revision 1)
>          return bitmap;
>      }
>  
>      @Override
>      protected void onNewIntent(Intent externalIntent) {
> +        IntentHelper.stripDataUri(getIntent());

Do we get the correct intent from "getIntent()"? or the parameter of onNewIntent "externalIntent"?

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
>  import android.app.Activity;
> -import android.content.Context;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:33
(Diff revision 1)
>      protected void onCreate(Bundle savedInstanceState) {
>          super.onCreate(savedInstanceState);
>  
>          GeckoAppShell.ensureCrashHandling();
>  
> +        IntentHelper.stripDataUri(getIntent());

Instead of injecting stripDataUri to onCreate and onNewIntent, what if we override "getIntent()"?
Will that cause a little performance problem since we strip and parse the url everytime?

So how about we override "setIntent()"? Whenever system invoke for onCreate and also onNewIntent, they will always get the striped one.

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
(Diff revision 1)
>  
>  import android.app.ActivityManager;
>  import android.content.Intent;
>  import android.graphics.Bitmap;
>  import android.net.Uri;
> -import android.os.Build;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:32
(Diff revision 1)
>  
>  import org.mozilla.gecko.AppConstants;
>  import org.mozilla.gecko.EventDispatcher;
>  import org.mozilla.gecko.GeckoApp;
>  import org.mozilla.gecko.GeckoAppShell;
> -import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.IntentHelper;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
(Diff revision 1)
>  import org.mozilla.gecko.Tabs;
>  import org.mozilla.gecko.util.ColorUtil;
>  import org.mozilla.gecko.util.EventCallback;
>  import org.mozilla.gecko.util.FileUtils;
>  import org.mozilla.gecko.util.GeckoBundle;
> -import org.mozilla.gecko.widget.AnchoredPopup;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:160
(Diff revision 1)
>       * shortcut has set, if not we reload the homescreens url
>       */
>      @Override
>      protected void onNewIntent(Intent externalIntent) {
>  
> +        IntentHelper.stripDataUri(getIntent());

Do we get the correct intent from "getIntent()"? or the parameter of onNewIntent "externalIntent"?
Attachment #8864742 - Flags: review?(max) → review-
(In reply to Nevin Chen [:nechen] from comment #12)
> (In reply to Sebastian Kaspari (:sebastian) from comment #10)
> 
> > stripe -> strip
> Done 
> > @@ +2184,1 @@
> > >          final SafeIntent intent = new SafeIntent(externalIntent);
> > 
> > This should probably be done on a SafeIntent object
> Using SafeIntent is good. But I think it's a little bit implicit cause I'll
> also override the data uri when user call "getIntent()" in the acivity.

We had problems with badly crafted Intents in the past - that's the reason why we have SafeIntent. If you now start to read from the Intent object directly then this might reintroduce those issues.
Flags: needinfo?(s.kaspari)
Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review140038
Attachment #8864742 - Flags: review?(s.kaspari)
Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139930

> Instead of injecting stripDataUri to onCreate and onNewIntent, what if we override "getIntent()"?
> Will that cause a little performance problem since we strip and parse the url everytime?
> 
> So how about we override "setIntent()"? Whenever system invoke for onCreate and also onNewIntent, they will always get the striped one.

It may be more effecient. But I'd prefer not to overrdie Android default behavior (although..... :P)
SafeIntent is the previous solution we took. So I want to go for that.
Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review142710
Attachment #8864742 - Flags: review?(max) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 47659d899be0 -d 437489a51017: rebasing 396271:47659d899be0 "Bug 1358946 - Strip about:reader in intent data uri. r=maliu" (tip)
merging mobile/android/chrome/content/browser.js
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 47659d899be0 -d a0c7189d60e3: rebasing 396286:47659d899be0 "Bug 1358946 - Strip about:reader in intent data uri. r=maliu" (tip)
merging mobile/android/chrome/content/browser.js
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like this fix is not landed, so not uplifted yet?
tracking-fennec: 54+ → +
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86af05828caf
Strip about:reader in intent data uri. r=maliu
Hi Wesley 
Sorry for the late. Please request SV for nightly test before uplift this. Thanks
Flags: needinfo?(whuang)
Backed out for Android bustage in SafeIntent.java:

https://hg.mozilla.org/integration/autoland/rev/75760878c7da78938b7ecc3060875f4b2f4ac732

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=86af05828caffe4a0a7cd2a33e1e4e0d54e39b08&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100085569&repo=autoland

[task 2017-05-18T11:49:26.515046Z] 11:49:26     INFO -  /home/worker/workspace/build/src/mobile/android/base/resources/drawable-xxhdpi/orange_check.png: libpng warning: iCCP: Not recognizing known sRGB profile that has been edited
[task 2017-05-18T11:49:27.143444Z] 11:49:27     INFO -  Note: Some input files use or override a deprecated API.
[task 2017-05-18T11:49:27.143735Z] 11:49:27     INFO -  Note: Recompile with -Xlint:deprecation for details.
[task 2017-05-18T11:49:27.143840Z] 11:49:27     INFO -  /home/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:15: error: package org.mozilla.gecko.util does not exist
[task 2017-05-18T11:49:27.144588Z] 11:49:27     INFO -  import org.mozilla.gecko.util.StringUtils;
[task 2017-05-18T11:49:27.145671Z] 11:49:27     INFO -                               ^
[task 2017-05-18T11:49:27.145741Z] 11:49:27     INFO -  /home/worker/workspace/build/src/java_home/bin/jar cMf gecko-thirdparty.jar -C gecko-thirdparty-classes .
[task 2017-05-18T11:49:27.388044Z] 11:49:27     INFO -  /home/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:144: error: cannot find symbol
[task 2017-05-18T11:49:27.388132Z] 11:49:27     INFO -                  final String strippedUrl = StringUtils.getQueryParameter(url, "url");
[task 2017-05-18T11:49:27.388167Z] 11:49:27     INFO -                                             ^
[task 2017-05-18T11:49:27.388189Z] 11:49:27     INFO -    symbol:   variable StringUtils
[task 2017-05-18T11:49:27.388205Z] 11:49:27     INFO -    location: class SafeIntent
[task 2017-05-18T11:49:27.425041Z] 11:49:27     INFO -  2 errors
[task 2017-05-18T11:49:27.431684Z] 11:49:27     INFO -  /home/worker/workspace/build/src/config/makefiles/java-build.mk:149: recipe for target 'gecko-mozglue.jar' failed
[task 2017-05-18T11:49:27.431928Z] 11:49:27     INFO -  gmake[5]: *** [gecko-mozglue.jar] Error 1
Flags: needinfo?(cnevinchen)
Flags: needinfo?(cnevinchen)
Attachment #8864742 - Flags: review+ → review?(max)
Attachment #8864742 - Attachment is obsolete: true
Attachment #8864742 - Flags: review?(max)
Attached patch 1358946.patch — — Splinter Review 

        Attachment #8869297 -
        Flags: review?(max)

    
    

    
  
Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

org.mozilla.gecko.util.StringUtils can't be found at compile time. I'm not sure if mach build causes this. Strange thing is it works on my machine. And I've pulled the latest code from m-c... anyway, the new approach I took will avoid this issue.

        Attachment #8869297 -
        Flags: review?(max) → review?(walkingice0204)

        Attachment #8869297 -
        Flags: review?(walkingice0204) → review?(max)

    
    

    
  
FW the needinfo to SV team and qe-verify+
This fix is targeted to go to 54 (beta).
Flags: qe-verify+
Flags: needinfo?(whuang)
Flags: needinfo?(ioana.chiorean)

    
  

        Attachment #8869297 -
        Flags: review?(max) → review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04554768a8e0
Strip about:reader in intent data uri. r=maliu
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/04554768a8e0
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

    
    

    
  
Since this bug targest 54. I marked it as 54+.
I'll make a uplift request after qe-verify+.
This is a small change, just remove the cause and add a little check on the incoming intent.
tracking-fennec: + → 54+

    
    

    
  
Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

Approval Request Comment
[Feature/Bug causing the regression]: Strip the URL in JS side to avoid unexpected URL from external intents
[User impact if declined]: Reader mode pages were saved as normal bookmarks
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: All behavior is the same as before(52). We only add an extra check for external intent here.
[String changes made/needed]: no

        Attachment #8869297 -
        Flags: approval-mozilla-beta?

    
    

    
  
Verified as fixed in build 55.0a1 from 2017/05/31.
Device: Nexus 9 (Android 7.1.1) and Huawei Honor (Android 5.1.1).
Testing was based on adding articles on reading list, prompt displayed, remove articles, reading list folder, number of items.

          status-firefox55:
          fixed → verified

    
    

    
  
Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

The fix seems low risk and has been verified/tested by SoftVision on Nightly, Beta54+

        Attachment #8869297 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
Taking my NI as Sorina worked on this.
Flags: needinfo?(ioana.chiorean)

    
    

    
  
Ni on Sorina to be tested on next Bet a( Tuesday)
Flags: needinfo?(sorina.florean)

    
    

    
  
firefox:54+ is clear, so I guess we'll simplify tracking:fennec flags.
tracking-fennec: 54+ → +

    
    

    
  
Verified as fixed on the latest Beta build, 54.0b14.
This issue was tested on a Nexus 9 - Android 6.0 and on a Samsung Galaxy S6 EDGE - Android 6.0
Clearing Sorina's NI
Flags: needinfo?(sorina.florean)

    
    

    
  
Removing qe-verify flag and marking 54:verified based on comment 45.
Status: RESOLVED → VERIFIED

          status-firefox54:
          fixed → verified
Flags: qe-verify+

    
  
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: