Rebase Orfox patches onto Tor Browser 8.0 for TBA
This is a replacement for #26338 (moved). We decided Tor Browser for Android should be based on ESR 60 because Mozilla Firefox for Android (Fennec) is currently in maintenance-mode - the diff between releases will be very small, and we will backport any security-related patches. Basing TBA on ESR60 means we can use the same git branch for desktop and mobile.
[0] https://lists.torproject.org/pipermail/tbb-dev/2018-June/000852.html
Activity
Required for first alpha.
Trac:
Parent: N/A to #26531 (moved)Okay, I'm giving in. I hit a crash-bug a week ago on this branch, and I've been trying to reproduce it, but I've no luck. We should start the review process.
Branch: tor-browser-60.1.0esr-8.0-1+26401 Repo: https://git.torproject.org/user/sysrqb/tor-browser.git
This branch does not include #26028 (moved), #24926 (moved). This branch does include #22059 (moved), #25363 (moved), #25364 (moved), #25366 (moved), #25367 (moved).
Trac:
Status: new to needs_reviewI will review in batches:
97ca08c7c8bc Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs
-
Do we want to disable the app.update.enabled (I am asking because of the #26242 (moved))?
-
About the user agent, I think it would be rv: 60.0, Gecko/60.0 and Firefox/60.0. Do we need to specify the android version?
-
I think we should disable all the sensors. (device.sensors.*)
69bdd94ecb8e Bug 25741 - TBA: Add default configure options in dedicated file LGTM
ae36b1d20547 Bug 25741 - TBA: Do not register Stumbler listener at start up LGTM
6d7a72d3e95b Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION I think we need to add something in the .mozconfigure specifying the current browser version, right?
9d8303c14531 Bug 25741 - TBA: Disable features at compile-time LGTM
66ecd900c106 Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files. Are we going to use Orfox assets? if yes, LGTM
2f7b87b25f2b Bug 25741 - TBA: Move GCM Push prefs within preprocessor guard LGTM
970c95dff0b3 Bug 25741 - TBA: Exclude unwanted Stumbler tests LGTM
-
cc199e9fda91 Bug 25741 - TBA: Clear state when the app exits, by default LGTM
5494b0193552 Bug 25741 - TBA: Do not import bookmarks and history from native browser by default LGTM
d9370c64b055 Bug 25741 - TBA: Do not save browsing history by default LGTM
47c04b064189 Bug 25741 - TBA: Move CAMERA permission within MOZ_WEBRTC We are already disabling the MediaDevices API in the prefs. Do we also need to make this change?
_908c2f542a82 Bug 25741 - TBA: Conditionally require *LOCATION permissions LGTM
'' 85b5d47bfa7f Bug 25741 - TBA: Conditionally require WIFI and NETWORK permissions'' I am not sure if we want to disable the access to the network state by default. We already have a fingerprint protection in place for the Network Information API. And again the update service has few checks about where the user is connected.
4b3c94077749 Bug 25741 - TBA: Disable QR Code reader by default LGTM
91a6e589e9c8 Bug 25741 - TBA: Disable the microphone by default LGTM
4d1310b1c439 Bug 25741 - TBA: Disable telemetry and experiments LGTM
25ec90395fc8 Bug 25741 - TBA: Remove sync option from preferences LGTM
21149e7a423d Bug 25741 - TBA: Neuter Firefox Accounts LGTM
e0bb700e3300 Bug 25741 - TBA: Only include GCM permissions if we want them LGTM
21f0480aff98 Bug 25741 - TBA: Only include Firefox Account permissions if we want them LGTM
cb7427ebb480 Bug 25741 - TBA: Always Quit, do not restore the last session LGTM
035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default Can we change toolkit.telemetry.enabled preference to false? the other part of the code LGTM.
ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?
952f0ae78cc1 Orfox: Centralized proxy applied to CrashReporter, SuggestClient, Distribution, AbstractCommunicator and BaseResources. Can we use the
network.proxy.*instead of hardcoding the proxy configuration in the java file?44c65b7904d8 Orfox: add BroadcastReceiver to receive Tor status from Orbot Should we initialize the
sTorStatus?db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed Is
import android.os.Looperneeded?e1ed56fc4ed4 Orfox: disable screenshots and prevent page from being in "recent apps" LGTM
94f9d2324391 Orfox: show only the "restricted" onboarding first time screens LGTM
14cefdc4bd90 Bug 25741 - TBA: Disable GeckoNetworkManager LGTM
91eeee498b67 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version hmm, are we not using the useragent pref?
a2c4e6217da4 Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily. Are we going to keep the guardian project in the list of suggested websites?
Replying to igt0:
I will review in batches:
Thanks!
97ca08c7c8bc Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs
- Do we want to disable the app.update.enabled (I am asking because of the #26242 (moved))?
My plan was disabling this pref for now. We can add a URL when have a plan for #26242 (moved).
- About the user agent, I think it would be rv: 60.0, Gecko/60.0 and Firefox/60.0. Do we need to specify the android version?
I was following Mozilla's new UAS format. This is the format Tor Browser is using on desktop now, too - but this may change with #26146 (moved). I chose that Android version because it is the oldest version of Android we support and the oldest version of Android that Google report 1%+ users.
https://developer.android.com/about/dashboards/ https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in#140 (you probably saw this already)
- I think we should disable all the sensors. (device.sensors.*)
Yes. I was thinking we can enumerate the other fingerprinting prefs we want disabled/changed in #25703 (moved). I only concentrated on the defenses Orfox provided in this patch.
6d7a72d3e95b Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION I think we need to add something in the .mozconfigure specifying the current browser version, right?
We can provide it on the command line when running ./mach configure (this is how tor-browser-build provides it). I was using
./mach configure --with-tor-browser-version=8.0 --with-distribution-id=org.torproject --enable-update-channel=alpha --enable-bundled-fontsWhen the environment variable
TB_BUILD_WITH_UPDATERis not set, then the updater is excluded at compile-time.66ecd900c106 Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files. Are we going to use Orfox assets? if yes, LGTM
I think yes, until we have replacements.
Replying to igt0:
035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default Can we change toolkit.telemetry.enabled preference to false? the other part of the code LGTM.
Yes, we should inherit this from 000-tor-browser.js: https://gitweb.torproject.org/user/sysrqb/tor-browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor-browser-60.1.0esr-8.0-1%2b26401#n96
ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?
This may not be necessary. This change was part of a larger patch in the Orfox patchset. We probably don't need it, but defense-in-depth and such.
Replying to igt0:
952f0ae78cc1 Orfox: Centralized proxy applied to CrashReporter, SuggestClient, Distribution, AbstractCommunicator and BaseResources. Can we use the
network.proxy.*instead of hardcoding the proxy configuration in the java file?Unfortunately, no, not easily. This is within GeckoView, and gradle builds it this library early in the build process. Accessing preferences is handled by
mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java, and this is built during the Fennec build process. Basically, GeckoPreferences doesn't exist when GeckoView is compiled.I considered some solutions/workarounds for this, but I decided hardcoding these values is an easy solution and it is probably acceptable for the first alpha. When we integrate Tor into the app we will need a new/different solution anyway.
Do you think there is another easy way we can use the prefs?
44c65b7904d8 Orfox: add BroadcastReceiver to receive Tor status from Orbot Should we initialize the
sTorStatus?Yes!
db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed Is
import android.os.Looperneeded?No, I'll delete it.
Replying to igt0:
91eeee498b67 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version hmm, are we not using the useragent pref?
No :( These are used by the Java network connections. We should use the pref in the future, but I didn't want a large patch here.
a2c4e6217da4 Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily. Are we going to keep the guardian project in the list of suggested websites?
Yes, for this ticket. We have #25917 (moved) for changing which sites we present in TBA. (I didn't want to make too many changes).
Sorry, I forgot to update this ticket. I have another branch ready for review. Some of the above comments are different from the changes I made.
Branch tor-browser-60.1.0esr-8.0-1+26401_1
ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?
I reverted this change. The pref is enough.
035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default Can we change toolkit.telemetry.enabled preference to false?
I reverted the default checkbox change.
db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed Is import android.os.Looper needed?
I moved this
importinto the correct commit. It was included in the wrong one.Replying to igt0:
21f0480aff98 Bug 25741 - TBA: Only include Firefox Account permissions if we want them LGTM
After the next round of reviews, I'll fixup this commit so it includes:
diff --git a/mobile/android/base/FennecManifest_permissions.xml.in b/mobile/android/base/FennecManifest_permissions.xml.in index 590429845085..1b7b2d5475ec 100644 --- a/mobile/android/base/FennecManifest_permissions.xml.in +++ b/mobile/android/base/FennecManifest_permissions.xml.in @@ -30,7 +30,9 @@ <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/> <uses-permission android:name="com.android.launcher.permission.INSTALL_SHORTCUT"/> <uses-permission android:name="com.android.launcher.permission.UNINSTALL_SHORTCUT"/> +#ifdef MOZ_ANDROID_LOCATION <uses-permission android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/> +#endif <uses-permission android:name="android.permission.WAKE_LOCK"/> <uses-permission android:name="android.permission.VIBRATE"/>(based on the comment I made at the bottom of ticket:24796#comment:10)
I started my review based on
tor-browser-60.1.0esr-8.0-1+26401which is what I stuck to. Here comes the first batch of comments:commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay
"We exclude the all testStumbler*.java files" (one of "the" and "all" should be enough :) )
Maybe use
''instead of""as all exclude rules are using the formercommit 2f7b87b25f2b2648f2f72addae1bcf7283bbf7da -- okay commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay
.mozconfig-android: are we really still using ndk r10e for esr60; I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use
--enable-striplike on other platforms as wellmozconfig.common.override: why do we have all of those changes here? Shouldn't the .mozconfig-android file already take care of them? If there are some options missing in the former let's add them there. Looking closer at that file it seems this is intended for Mozilla's try infra? While I think that's worthwhile I think we should have a separate bug for that and thinking about a strategy including all the other platforms we support as well.
"When using --disable-crashreporter the symbole file" -> "When using --disable-crashreporter the symbols file"
commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file a bug in Mozilla's bug tracker for the unused defines?)
commit 6d7a72d3e95bf483a9256cfe3bf9b3888a363da3 -- okay commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay
Indentation:
+ if (AppConstants.isTorBrowser()) { getApplicationContext().sendBroadcast( new Intent(INTENT_REGISTER_STUMBLER_LISTENER) ); + } // !isTorBrowser()commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)
commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay
I think we can skip the whole UA override thing as it does not play any role anymore once we set the resistfingerprinting pref (which is done for mobile as well)
Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.
s/URLS/URLs/
no need for dom.battery.enabled anymore (see: #22554 (moved))
network.manage-offline-statusis already set in 000-tor-browser.jsgeo.enabledis already set in 000-tor-browser.jsDo we need
network.tickle-wifi.enabledgiven #18799 (moved)?I think we should exclude the VR related pref and fix that in the desktop pref file instead (#21607 (moved)).
It's not clear to me why we have some of the prefs set in mobile but not desktop (e.g. the clearOnShutdown ones). I guess we could look over it in a new ticket to make sure we really only include android specific prefs in the new prefs file?
commit 25ec90395fc82795e8b130c3d763d4b8893f1114 -- okay commit 4d1310b1c43975cfefaa8134f54f1e487db6e431 -- okay commit 91a6e589e9c89219eb15c721122a0564d7399b4b -- okay commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?) commit 85b5d47bfa7f2ecc33767e0d5cd4bfd0a0a33460 -- okay commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the additional, empty line intentional?
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/> #endif + <uses-permission android:name="android.permission.INTERNET"/>) commit 47c04b0641898dfe2a2f1ca53aee101f41809d15 -- okay commit d9370c64b055fdcb9293aa1fc718eab8c2876257 -- okay commit 5494b0193552b2833d05bd507264d9796b651cca -- okay commit cc199e9fda917acee9435eba791a8d1a69536443 -- okay commit ad48b4d561522353c621a5d5166fc31b2517b624 -- okay commit 035fbfb88a3c05c9b9776139b4e318ee956a1f6f -- okay commit cb7427ebb4805c42e67cb0e93b3f8b8a5b19a365 -- okay commit 21f0480aff989a3853188a2ae424152a8e34b72b -- okay commit e0bb700e33001643de6d3e58adf8a2c881a61dbc -- okay
-
commit 21149e7a423d1f88866e5781e521021a08fca126 -- not okay
Indentation of code in the else-clauses is missing. Are we good with all the other account manager related things that Orfox patched but this patch omits?
commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???
That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?
Do we really want to have mobile/android/orfox? What happened to
+ <string name="no_space_to_start_error">&no_space_to_start_error;</string> + <string name="error_loading_file">&error_loading_file;</string>?
-
I need to look with fresh eyes at commit 44c65b7904d8bf599345991942e0407af2030199 and commit 952f0ae78cc17f66624f3c634d831ff0aedc9692 tomorrow but the comments to the remaining commits are:
commit a2c4e6217da42219327206508015ee24846907c4 -- probably okay (Any reason the Facebook bg color (#ffecf0f3) did not get adjusted as done in the original patch?)
commit 91eeee498b6733075d1de69d9f3568bfbffb5f05 -- not okay
I assume the resistfingerprinting related UA is not being used by Java code automatically? If so, then we should hardcode that one and not a custom one.
commit 14cefdc4bd90083e22ac263954a857d254b346a9 -- not okay
Indentation of the if-clauses
commit 94f9d2324391c5c18ac22a025b322a71adff3fcc -- okay commit e1ed56fc4ed4a6cdcef8c692cb2d5f909174ddc4 -- okay
Replying to gk:
I started my review based on
tor-browser-60.1.0esr-8.0-1+26401which is what I stuck to. Here comes the first batch of comments:Thanks, no problem.
commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay
"We exclude the all testStumbler*.java files" (one of "the" and "all" should be enough :) )
Maybe use
''instead of""as all exclude rules are using the formerChanged.
commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay
.mozconfig-android: are we really still using ndk r10e for esr60;
No, I'll update the comment (Fennec is build using NDK r15c now)
I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use
--enable-striplike on other platforms as wellThe defaults are effectively
--disable-stripand--enable-install-stripbut we should test these options and understand what improves and what fails when we enable/disable them. I'm still not sure.dnl ======================================================== dnl = Enable stripping of libs & executables dnl ======================================================== MOZ_ARG_ENABLE_BOOL(strip, [ --enable-strip Enable stripping of libs & executables ], ENABLE_STRIP=1, ENABLE_STRIP= ) dnl ======================================================== dnl = Enable stripping of libs & executables when packaging dnl ======================================================== MOZ_ARG_ENABLE_BOOL(install-strip, [ --enable-install-strip Enable stripping of libs & executables when packaging ], PKG_SKIP_STRIP= , PKG_SKIP_STRIP=1)$ grep -e ENABLE_STRIP -e PKG_SKIP_STRIP obj-arm-linux-androideabi/config.status 'ENABLE_STRIP': '', 'PKG_SKIP_STRIP': '',mozconfig.common.override: why do we have all of those changes here? Shouldn't the .mozconfig-android file already take care of them? If there are some options missing in the former let's add them there. Looking closer at that file it seems this is intended for Mozilla's try infra? While I think that's worthwhile I think we should have a separate bug for that and thinking about a strategy including all the other platforms we support as well.
Okay, that's fair, I'll factor out these changes.
"When using --disable-crashreporter the symbole file" -> "When using --disable-crashreporter the symbols file"
ack.
commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file a bug in Mozilla's bug tracker for the unused defines?)
Yes, I'll open a ticket for that.
commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay
Indentation: {{{
-
if (AppConstants.isTorBrowser()) { getApplicationContext().sendBroadcast( new Intent(INTENT_REGISTER_STUMBLER_LISTENER) ); -
} // !isTorBrowser()
}}}
I did this so the diff is only the new lines instead of the entire statement. This seemed easier for reviewing and future rebase. I can indent the conditional if that is more readable.
commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)
I added those in torbrowser.configure because they look like configure flags but they are environment variables. They are still in torbrowser.configure only for documentation purposes. I can delete them if it's confusing or you think they aren't helpful.
commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay
I think we can skip the whole UA override thing as it does not play any role anymore once we set the resistfingerprinting pref (which is done for mobile as well)
Okay, I'll delete it.
Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.
Hm! Those
//were only supposed to be visual and not affect the inclusion. I thought the preprocessor preserves and enforces the ifdef when it scans the file - but I see this did not happen.app.update.url.androidis still set. I'll look into this more.s/URLS/URLs/
ack.
no need for dom.battery.enabled anymore (see: #22554 (moved))
Okay, I agree. I'll delete that.
network.manage-offline-statusis already set in 000-tor-browser.jsgeo.enabledis already set in 000-tor-browser.jsI see! Deleted.
Do we need
network.tickle-wifi.enabledgiven #18799 (moved)?Okay, no, that looks good.
I think we should exclude the VR related pref and fix that in the desktop pref file instead (#21607 (moved)).
I'm okay with this.
It's not clear to me why we have some of the prefs set in mobile but not desktop (e.g. the clearOnShutdown ones). I guess we could look over it in a new ticket to make sure we really only include android specific prefs in the new prefs file?
Yes. I think we should discuss this, in particular I'm not sure if autoplay should be enabled or disabled. This may be one example where it provides a better user experience if it is disabled on mobile but the remains enabled on desktop.
commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?)
I think this provides a safe default. When this is enabled, the user is provided an icon in the awesome bar for scanning a QRCode that (presumably) contains a URL the user wants to visit. When the icon is pressed, another app is opened (if it is installed) where a picture is taken and it is scanned for a QRCode and then decoded. I believe the scanning/decoding is performed on a remote server. Disabling this by default protects against accidentally revealing their location to third parties. The user could tap the icon by mistake and launch the third party app, and we don't have any control over what that app does or what information that app sends.
commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the additional, empty line intentional? {{{ #endif + }}} )
No, I'll revert that new line.
-
Replying to gk:
commit 21149e7a423d1f88866e5781e521021a08fca126 -- not okay
Indentation of code in the else-clauses is missing.
I'll correct these.
Are we good with all the other account manager related things that Orfox patched but this patch omits?
Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is: https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773
commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???
That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?
Yes. The imported files are from commit 26304115de4939f20f023715ab7b079ce7105c1d and (it looks like) the old version was tag 1.2 (commit 2f3e6f0bbea4755617286813b2dd80907d0a573f in the https://github.com/guardianproject/NetCipher.git repo)
But I thought about this some more. I'll ask Nathan if there were any critical fixes between the 1.2 tag and the current master. If there weren't any critical patches, then I'll revert this commit and cherry-pick Orfox's netcipher import commit from tag 1.2. It may not be worth the time reviewing the changes between versions if we consider we are only using netcipher and Orbot in the first couple alpha releases, and we'll use a tor launcher before the first stable is released.
Do we really want to have mobile/android/orfox? What happened to {{{
- &no_space_to_start_error;
- &error_loading_file; }}} ?
That came from this commit, https://github.com/guardianproject/tor-browser/commit/b2c793dc73c23c60d5d3e22c6a138fad9f7cd1b2
Those strings aren't included anymore. I missed the
orfox -> torbrowserchange. I'll correct that.Replying to gk:
commit a2c4e6217da42219327206508015ee24846907c4 -- probably okay (Any reason the Facebook bg color (#ffecf0f3) did not get adjusted as done in the original patch?)
No, I applied the patch manually because there were many conflict. I think I chose keeping the Fennec background color instead of changing it to #ffecf0f3 because #3B5998 is the normal "Facebook Blue".
commit 91eeee498b6733075d1de69d9f3568bfbffb5f05 -- not okay
I assume the resistfingerprinting related UA is not being used by Java code automatically? If so, then we should hardcode that one and not a custom one.
Okay, I'll change the UA strings so they are hard coded instead of concatenating static values.
commit 14cefdc4bd90083e22ac263954a857d254b346a9 -- not okay
Indentation of the if-clauses
Ack.
-
Replying to sysrqb:
Replying to gk:
[snip]
I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use
--enable-striplike on other platforms as wellThe defaults are effectively
--disable-stripand--enable-install-stripbut we should test these options and understand what improves and what fails when we enable/disable them. I'm still not sure.Well, that makes it so that we build with symbols but then strip them during the packaging step (that can be confusing alone in case you want to get debug symbols after packaging up everything). It seems to be cleaner to me to indicate upfront in the .mozconfig file that we don't have symbols available by specifying
--enable-strip.[snip]
commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)
I added those in torbrowser.configure because they look like configure flags but they are environment variables. They are still in torbrowser.configure only for documentation purposes. I can delete them if it's confusing or you think they aren't helpful.
I don't feel that strongly about it. Let's keep the commit as-is if you want.
[snip]
Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.
Hm! Those
//were only supposed to be visual and not affect the inclusion. I thought the preprocessor preserves and enforces the ifdef when it scans the file - but I see this did not happen.app.update.url.androidis still set. I'll look into this more.I don't think that's the case for the .js file. At any rate, I would follow the style Mozilla has in this file which is plain
#ifdef's.[snip]
commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?)
I think this provides a safe default. When this is enabled, the user is provided an icon in the awesome bar for scanning a QRCode that (presumably) contains a URL the user wants to visit. When the icon is pressed, another app is opened (if it is installed) where a picture is taken and it is scanned for a QRCode and then decoded. I believe the scanning/decoding is performed on a remote server. Disabling this by default protects against accidentally revealing their location to third parties. The user could tap the icon by mistake and launch the third party app, and we don't have any control over what that app does or what information that app sends.
Good point. I was not aware that the QRCode treatment is actually done remotely, so, yes, let's keep the commit.
[snip]
-
Replying to sysrqb:
Replying to gk:
[snip]
Are we good with all the other account manager related things that Orfox patched but this patch omits?
Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is: https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773
I looked at the Orfox links in https://bugzilla.mozilla.org/show_bug.cgi?id=1314778 and grepped a bit in the esr60 source code. I am not sure why Orfox for esr52 does not have all the fixes that could still be applied. Maybe that's not necessary anymore for a reason I don't see at first glance, or maybe that's been an oversight? At any rate there are still unpatched cases for esr60 that were patched back then (e.g. AccountManager related part in SendTab.java)
commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???
That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?
Yes. The imported files are from commit 26304115de4939f20f023715ab7b079ce7105c1d and (it looks like) the old version was tag 1.2 (commit 2f3e6f0bbea4755617286813b2dd80907d0a573f in the https://github.com/guardianproject/NetCipher.git repo)
But I thought about this some more. I'll ask Nathan if there were any critical fixes between the 1.2 tag and the current master. If there weren't any critical patches, then I'll revert this commit and cherry-pick Orfox's netcipher import commit from tag 1.2. It may not be worth the time reviewing the changes between versions if we consider we are only using netcipher and Orbot in the first couple alpha releases, and we'll use a tor launcher before the first stable is released.
Sounds reasonable to me. I'll wait with further review for that commit then.
[snip]
-
Further comments:
commit 44c65b7904d8bf599345991942e0407af2030199 -- okay
Don't we need
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=6c507ea953072deb125d3ad4afa3282ae1ce884b https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=eea65c845366312fe2d6b8d955264b2158044db8
?
It seems you omitted
which means you lean to WONTFIX #24926 (moved)? Shouldn't we keep the current Orfox behavior until we make a final decision? (I don't have a strong opinion here)
EDIT: I just saw in comment:3 not including that one is actually intentional.
-
Finally:
commit 952f0ae78cc17f66624f3c634d831ff0aedc9692 -- not okay
It seems we don't need
+import java.net.Proxy; +import java.net.InetSocketAddress;and regarding
+import ch.boye.httpclientandroidlib.HttpHost; +import ch.boye.httpclientandroidlib.conn.params.ConnRoutePNames;The first gets imported twice with that code snippet and could you move the second one to where it fits into the ch.boye.httpclientandroidlib.conn.* area?
-
Oh, and could you adjust the commit message of 952f0ae78cc17f66624f3c634d831ff0aedc9692. It's slightly confusing now that https://bugzilla.mozilla.org/show_bug.cgi?id=1357997 got fixed.
Replying to gk:
Replying to sysrqb:
Replying to gk:
[snip]
Are we good with all the other account manager related things that Orfox patched but this patch omits?
Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is: https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773
I looked at the Orfox links in https://bugzilla.mozilla.org/show_bug.cgi?id=1314778 and grepped a bit in the esr60 source code. I am not sure why Orfox for esr52 does not have all the fixes that could still be applied. Maybe that's not necessary anymore for a reason I don't see at first glance, or maybe that's been an oversight? At any rate there are still unpatched cases for esr60 that were patched back then (e.g. AccountManager related part in SendTab.java)
Replying to gk:
Don't we need
I noticed yesterday this commit was lost during a rebase. I'll include this in the next branch.
Same. I'll cherry-pick this commit in the next branch, too.
?
It seems you omitted
which means you lean to WONTFIX #24926 (moved)? Shouldn't we keep the current Orfox behavior until we make a final decision? (I don't have a strong opinion here)
EDIT: I just saw in comment:3 not including that one is actually intentional.
I'm hesitant including this. On the one hand, it's possible some Orfox uses are using this feature and will want it included in TBA. On the other hand, this is a feature uniquely available on Android and it exposes Tor Browser to a new attack vector (DoS by a third-party app). I'm not against the idea of implementing this functionality, especially considering the situations where some people use their mobile devices, but I'd like this provided behind a pref.
It's easy for me to say "I don't think including this is a good idea" and then try justifying not including it. Instead of that, let's include it and then decide on the best way forward with #24926 (moved).
-
One additional thing while I was thinking about our update setup: the app update URL and enabling the MAR signature and verification machine belong together but are dealt with by different means (once by an environment variable check (
TB_BUILD_WITH_UPDATER) and the other time by an preprocessor directive (#ifdef TOR_BROWSER_VERSION). I think using onceTB_BUILD_WITH_UPDATERand the other timeTOR_BROWSER_VERSIONis confusing. And if we really want to enable updates sometime in the future we'd need to deal with both asTOR_BROWSER_VERSIONis very likely set in both cases: for builds with and without updating enabled.So,
TB_BUILD_WITH_UPDATER(or something similar) should cover both the app update URL and the pieces in the .mozconfig file.(Oh, and right now it seems we would need two builds if we'd ship an updating and a non-updating version. I wonder if we could design it in a way that we only needed some preference flip during the final packaging step to take care of that. Right I think we can if we always build with the update mechanism enabled but just make sure we don't publish update files/disable update checks per pref).
Replying to gk:
One additional thing while I was thinking about our update setup: the app update URL and enabling the MAR signature and verification machine belong together but are dealt with by different means (once by an environment variable check (
TB_BUILD_WITH_UPDATER) and the other time by an preprocessor directive (#ifdef TOR_BROWSER_VERSION). I think using onceTB_BUILD_WITH_UPDATERand the other timeTOR_BROWSER_VERSIONis confusing. And if we really want to enable updates sometime in the future we'd need to deal with both asTOR_BROWSER_VERSIONis very likely set in both cases: for builds with and without updating enabled.So,
TB_BUILD_WITH_UPDATER(or something similar) should cover both the app update URL and the pieces in the .mozconfig file.(Oh, and right now it seems we would need two builds if we'd ship an updating and a non-updating version. I wonder if we could design it in a way that we only needed some preference flip during the final packaging step to take care of that. Right I think we can if we always build with the update mechanism enabled but just make sure we don't publish update files/disable update checks per pref).
Yes, these are good points. I think I'll delete
TB_BUILD_WITH_UPDATERand we can discuss a better way for handling this later. It may be easier if we introduce another configure flag for this. I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky.Replying to sysrqb:
I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky. I take this back, I think we can accomplish this without much risk (Google Play's only says we can't update the app, but it doesn't say the app must not be capable of updating itself). #26528 (moved) may give us what we need for this.
-
Replying to sysrqb:
Replying to sysrqb:
I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky. I take this back, I think we can accomplish this without much risk (Google Play's only says we can't update the app, but it doesn't say the app must not be capable of updating itself). #26528 (moved) may give us what we need for this.
That's what I had in mind, yes.
-
Okay, we are getting close, just the following comments/remarks are left:
commit 0b932f895e3b2866161048a6007e61e257dc395b -- not okay
There is no need for a
--enable-install-stripas we already strip everything with--enable-strip.commit 408e63083f15d59c98fee619a277dfe123429d8c -- not okay
we should follow the Android version as used in the resist fingerprinting mode, see:
I.e. "Android 4.1.0" > "Android 6.0" which is way more popuplar than 4.1.0 as well.
commit 7bc689d797439d6394853e421f71eda5d7d1631a -- not okay
+import android.content.IntentFilter; <- seems to be something not used (probably part of the NetCipher upgrade on the previous branch?)
checkStartOrbot()came beforeprocessTabQueue()before (and the if-clause around!mHasResumedgot deleted). Doesn't the order matter here? (I don't know just for double-checking)Trac:
Status: needs_review to needs_revision Replying to gk:
Okay, we are getting close, just the following comments/remarks are left:
commit 0b932f895e3b2866161048a6007e61e257dc395b -- not okay
There is no need for a
--enable-install-stripas we already strip everything with--enable-strip.Okay.
commit 408e63083f15d59c98fee619a277dfe123429d8c -- not okay
we should follow the Android version as used in the resist fingerprinting mode, see:
I.e. "Android 4.1.0" > "Android 6.0" which is way more popuplar than 4.1.0 as well.
Hrm. Yes, I see. I did consider using a higher Android version, but I thought it would be better if we used the lowest supported version number instead of the most popular. In any case, I don't feel strongly about this. I'll change the hard-coded UAS so it matches the RFP string.
commit 7bc689d797439d6394853e421f71eda5d7d1631a -- not okay
+import android.content.IntentFilter; <- seems to be something not used (probably part of the NetCipher upgrade on the previous branch?)
Ugh. That was added in the wrong commit. The
importshould be in 694067738b04b6a670d072c64a7afa4be63581b1 ("Orfox: add BroadcastReceiver..."). I'll move it.checkStartOrbot()came beforeprocessTabQueue()before (and the if-clause around!mHasResumedgot deleted). Doesn't the order matter here? (I don't know just for double-checking)Yeah, this was something I looked at while I was rebasing the commit. Calling
checkStartOrbot()was moved as a fix for a crash (https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=dc6f0c7a561321460ca0f5987f30017ee1066e38).I think the
!mHasResumedclause was deleted in Orfox as a way of preventing the app from bringing itself to the foreground the next time it was paused (put into the background). After it was deleted,mHasResumedwould never be set to true inonResume(), so whenonPause()is called themHasResumedconditional isn't executed andPrompt:ShowTopis never registered. I don't know why this was missed during the rebase - I might've been confused while resolving a merge conflict. I'll include this deletion. https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=7040f97c97b238b2adef4cae96471907d38caf1dI think we dont need this commit: https://gitweb.torproject.org/user/sysrqb/tor-browser.git/commit/?h=tor-browser-60.1.0esr-8.0-1%2b26401_3&id=109f0a19ed90738210577cdca051564c0f57a1c6
We are going to implement the onboarding in the https://trac.torproject.org/projects/tor/ticket/25696
Besides that, nice work!
-
Replying to igt0:
I think we dont need this commit: https://gitweb.torproject.org/user/sysrqb/tor-browser.git/commit/?h=tor-browser-60.1.0esr-8.0-1%2b26401_3&id=109f0a19ed90738210577cdca051564c0f57a1c6
We are going to implement the onboarding in the https://trac.torproject.org/projects/tor/ticket/25696
Good point. I took all the patches from
tor-browser-60.1.0esr-8.0-1+26401_3except the above one and applied them totor-browser-60.1.0esr-8.0-1, woo!Besides that, nice work!
Indeed, thanks all!
Trac:
Status: needs_review to closed
Keywords: TorBrowserTeam201807 deleted, TorBrowserTeam201807R added
Resolution: N/A to fixed mentioned in issue #26858 (moved)