Skip to content

/ brave-core

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

Issue 3258: Using BraveUpdateClientConfig to send requests for extension update in XML and disable CUP signing #1602

Merged
merged 1 commit into from Feb 9, 2019
Merged

Issue 3258: Using BraveUpdateClientConfig to send requests for extension update in XML and disable CUP signing #1602

merged 1 commit into from Feb 9, 2019

Conversation

@jumde
Copy link
Contributor

jumde commented Feb 6, 2019 •
edited

fix: brave/brave-browser#3258

Description

Go-Updater expects update requests in XML format. ChromeUpdateClientConfig uses feature flag kFeatureUpdateClientUseJSON to check if the request should be in XML or JSON format. This flag is disabled by default, but overridden by field trials.

This change ensures that BraveUpdateClientConfig uses ProtocolHandlerFactoryXML is used to send XML update requests to go-updater. This change also disables CUP signing for extension updates.

auditors: @bbondy, @emerick

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Download an extension (Please DM me if you need a profile with outdated extensions)
  2. Navigate to brave://extensions
  3. Enable Developer Mode and click Update
  4. Extensions should be updated.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source
@jumde jumde self-assigned this Feb 6, 2019
@jumde jumde force-pushed the disable_client_use_json branch 2 times, most recently from 083eee2 to 8de1464 Feb 7, 2019
@jumde jumde changed the title Disable kFeatureUpdateClientUseJSON to send requests for extension update in XML Issue 3258: Using BraveUpdateClientConfig to send requests for extension update in XML and disable CUP signing Feb 7, 2019
@jumde jumde requested review from emerick and bbondy Feb 7, 2019
@emerick
emerick reviewed Feb 7, 2019
View changes
Copy link
Contributor

emerick left a comment

I guess there's no way to opt out of this particular field trial? This change seems fine, but just wondering if that would be a simpler way to achieve the same effect.
@jumde
Copy link
Contributor Author

jumde commented Feb 7, 2019

I guess there's no way to opt out of this particular field trial? This change seems fine, but just wondering if that would be a simpler way to achieve the same effect.

@emerick - We can override the field trial flag by adding the feature to the disabled features in https://github.com/brave/brave-core/blob/master/app/brave_main_delegate.cc#L134.

@bbondy and I discussed the fix before, but using BraveUpdateClientConfig to enforce XML responses and disable CUP signing feels like the right way to go.
@emerick
Copy link
Contributor

emerick commented Feb 7, 2019

@jumde OK, makes sense.
@emerick
emerick reviewed Feb 7, 2019
View changes
Copy link
Contributor

emerick left a comment

LGTM
@jumde jumde force-pushed the disable_client_use_json branch 6 times, most recently from 7e6dfe5 to aac79c2 Feb 7, 2019
@jumde
Issue 3258: Using BraveUpdateClientConfig to send requests for extens…
Loading status checks…
496bf17 
…ion update

in XML and disable CUP signing.

Go-Updater expects update requests in XML format. ChromeUpdateClientConfig uses
feature flag kFeatureUpdateClientUseJSON to check if the request should be in
XML or JSON format. This flag is disabled by default, but overridden by field
trials.

This change ensures that ProtocolHandlerFactoryXML is used to send XML update
requests to go-updater. This change also disables CUP signing for extension
updates.

auditors: @bbondy, @emerick
@jumde jumde force-pushed the disable_client_use_json branch from aac79c2 to 496bf17 Feb 8, 2019
@jumde jumde mentioned this pull request Feb 8, 2019
Closed
@jumde
Copy link
Contributor Author

jumde commented Feb 8, 2019

PR passed the PR builder: https://staging.ci.brave.com/job/brave-browser-build-pr/job/brave-core-PR-1602/15/
@emerick
emerick approved these changes Feb 8, 2019
View changes
Copy link
Contributor

emerick left a comment

LGTM
@jumde jumde merged commit d5e0001 into master Feb 9, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/jenkins/pr-head The build of this commit was aborted
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jumde jumde added this to the 0.62.x - Nightly milestone Feb 9, 2019
@mihaiplesa mihaiplesa deleted the disable_client_use_json branch Feb 9, 2019
jumde added a commit that referenced this pull request Feb 12, 2019
@jumde
Merge pull request #1602 from brave/disable_client_use_json
Loading status checks…
ef58582 
Issue 3258: Using BraveUpdateClientConfig to send requests for extension update in XML and disable CUP signing
@jumde jumde mentioned this pull request Feb 12, 2019
Merged
5 of 18 tasks complete
jumde added a commit that referenced this pull request Feb 13, 2019
@jumde
Merge pull request #1602 from brave/disable_client_use_json
Loading status checks…
d78148b 
Issue 3258: Using BraveUpdateClientConfig to send requests for extension update in XML and disable CUP signing
@jumde jumde mentioned this pull request Feb 13, 2019
Merged
3 of 18 tasks complete
fmarier pushed a commit that referenced this pull request Oct 29, 2019
@bbondy
Merge pull request #1602 from srirambv/issue_template
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
94392dc 
Update issue template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emerick emerick

@bbondy bbondy

Assignees

@jumde jumde

Labels
None yet
Projects
None yet
Milestone
  0.60.x - Release
Linked issues

Successfully merging this pull request may close these issues.

Extension update requests are sent in JSON are not handled correctly
3 participants
@jumde @emerick @tomlowenthal
You can’t perform that action at this time.