Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes https everywhere not working with existing database. #13143

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

mkarolin
Copy link
Collaborator

Fixes brave/brave-browser#22503

We use third_party/zlib to unzip HTTPS Everywhere database from a zip
file. Chromium recently changed unzip behavior to error out if the
target file already exists (previously it happily overrode existing
files). This caused our code that unzips the file to error out and not
load the db.

Changed our code to delete existing unzipped directory before unzipping.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/7ba6004ba3727e9f5339841cfe0f54e7e51dfbf2

commit 7ba6004ba3727e9f5339841cfe0f54e7e51dfbf2
Author: François Degros fdegros@chromium.org
Date: Thu Mar 17 05:46:45 2022 +0000

[zip] Unzip() does not overwrite files anymore

Changed the way FilePathWriterDelegate creates a new file, by using
FLAG_CREATE instead of FLAG_CREATE_ALWAYS.

This prevents FilePathWriterDelegate and zip::Unzip() from overwriting
any existing file. This allows the detection of duplicated or
conflicting file names when extracting a ZIP archive. See
ZipTest.UnzipRepeatedFileName.

This fixes a confusing corner case on case-insensitive file systems,
which could result in file contents not matching file names as stored in
the ZIP archive. See ZipTest.UnzipDifferentCases.

BUG=chromium:953256

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

We use third_party/zlib to unzip HTTPS Everywhere database from a zip
file. Chromium recently changed unzip behavior to error out if the
target file already exists (previously it happily overrode existing
files). This caused our code that unzips the file to error out and not
load the db.

Changed our code to delete existing unzipped directory before unzipping.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/7ba6004ba3727e9f5339841cfe0f54e7e51dfbf2

commit 7ba6004ba3727e9f5339841cfe0f54e7e51dfbf2
Author: François Degros <fdegros@chromium.org>
Date:   Thu Mar 17 05:46:45 2022 +0000

    [zip] Unzip() does not overwrite files anymore

    Changed the way FilePathWriterDelegate creates a new file, by using
    FLAG_CREATE instead of FLAG_CREATE_ALWAYS.

    This prevents FilePathWriterDelegate and zip::Unzip() from overwriting
    any existing file. This allows the detection of duplicated or
    conflicting file names when extracting a ZIP archive. See
    ZipTest.UnzipRepeatedFileName.

    This fixes a confusing corner case on case-insensitive file systems,
    which could result in file contents not matching file names as stored in
    the ZIP archive. See ZipTest.UnzipDifferentCases.

    BUG=chromium:953256
@mkarolin mkarolin requested review from bridiver and iefremov April 22, 2022 19:03
@mkarolin mkarolin self-assigned this Apr 22, 2022
@stephendonner
Copy link
Contributor

stephendonner commented Apr 25, 2022

Verified PASSED using

Brave 1.40.11 Chromium: 101.0.4951.41 (Official Build) nightly (x86_64)
Revision 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS macOS Version 11.6.5 (Build 20G527)

New Profile

  1. new profile
  2. loaded http://https-everywhere.badssl.com
  3. ensured I was redirected to https://https-everywhere.badssl.com automatically
  4. reloaded the above tab; confirmed I stayed on https://https-everywhere.badssl.com
  5. pasted the HTTP URL into both existing and new tabs, and ensured I got redirected to https://https-everywhere.badssl.com

httpse

Existing Profile

  1. ensure my profile had <PROFILE_DIR>/oofiananboodjbbmdelgdommihjbkfag/1.0.91/6.0/httpse.leveldb
  2. loaded http://https-everywhere.badssl.com
  3. ensured I was redirected to https://https-everywhere.badssl.com automatically
  4. reloaded the above tab; confirmed I stayed on https://https-everywhere.badssl.com
  5. pasted the HTTP URL into both existing and new tabs, and ensured I got redirected to https://https-everywhere.badssl.com

Screen Shot 2022-04-25 at 3 17 29 PM

@kjozwiak
Copy link
Member

kjozwiak commented Apr 25, 2022

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.40.11 Chromium: 101.0.4951.41 (Official Build) nightly (64-bit)
-- | --
Revision | 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS | Windows 11 Version 21H2 (Build 22000.613)

Clean Profile/New Install

Using the STR/Cases outlined via brave/brave-browser#22503 (comment), ensured that:

  • loading http://https-everywhere.badssl.com automatically redirected the user to HTTPS
  • opened several tabs and ensured that the redirect from http://https-everywhere.badssl.com -> HTTPS is working
  • pasted http://https-everywhere.badssl.com into several existing tabs and ensured the page was being redirected to HTTPS
  • ensured disabling Upgrade connections to HTTPS via the V2 shields didn't re-direct the user to HTTPS when loading http://https-everywhere.badssl.com
  • ensured that once shields are disabled, loading http://https-everywhere.badssl.com doesn't redirect the user to HTTPS

Existing Profile/Upgrade Case

Reproduced the original issue while running 1.40.09 Chromium: 101.0.4951.41. Once I had a profile which was affected and included <PROFILE_DIR>/oofiananboodjbbmdelgdommihjbkfag/1.0.91/6.0/httpse.leveldb as per the STR/Cases, I updated to 1.40.11 Chromium: 101.0.4951.41 and ensured the following:

  • the active tab that had https://https-everywhere.badssl.com opened automatically updated to HTTPS (was failing before)
  • using the same tab that originally had https://https-everywhere.badssl.com opened and opened the HTTP version several times and ensured it was being updated to https://https-everywhere.badssl.com every single time
  • ensured that disabling Upgrade connections to HTTPS via the V2 shields worked as expected and didn't redirect the user to the HTTPS page when loading http://https-everywhere.badssl.com

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPSE upgrades not working with Chromium 101
4 participants