Brave News: Do not use APIRequestHelper for binary data #12857

petemill · 2022-03-31T23:48:15Z

APIRequestHelper has recently changed to validate that the response is valid JSON.

Resolves brave/brave-browser#22024

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:

fmarier

@petemill This doesn't look like it removes any of the (somewhat) identifying request headers (e.g. Accept-Language, User-Agent) when sending a request to the private CDN. Is that header removal done somewhere else? Or did we end up not implementing it? I can't remember.

petemill · 2022-04-02T03:26:57Z

@petemill Did doesn't look like it removes any of the (somewhat) identifying request headers (e.g. Accept-Language, User-Agent) when sending a request to the private CDN. Is that header removal done somewhere else? Or did we end up not implementing it? I can't remember.

@fmarier The header removal is done here https://github.com/brave/brave-core/pull/12857/files#diff-f5dc813d6939b2fdab7d89947a9657e8d6583d004b2d74c4839017647ca637f4R50

petemill · 2022-04-02T03:27:40Z

I pushed a refactor earlier today and didn't have a chance to build which is why there's CI errors, which I'll address now

goodov · 2022-04-04T06:42:22Z

components/brave_private_cdn/private_cdn_request_helper.cc

+    net::NetworkTrafficAnnotationTag annotation_tag,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    : annotation_tag_(annotation_tag),
+      url_loader_factory_(url_loader_factory) {}


std::move(url_loader_factory)

fixed, and noting that this is it's being done in the code I copied it from 😄 https://github.com/brave/brave-core/blob/master/components/api_request_helper/api_request_helper.cc#L45

goodov · 2022-04-04T06:45:24Z

components/brave_private_cdn/private_cdn_request_helper.cc

+  request->credentials_mode = network::mojom::CredentialsMode::kOmit;
+  request->method = net::HttpRequestHeaders::kGetMethod;
+  // Don't send any identifying information, such as language or user agent
+  for (auto entry : brave::private_cdn_headers)


const auto& entry

goodov · 2022-04-04T07:08:15Z

components/brave_private_cdn/private_cdn_request_helper.cc

+    }
+  }
+  url_loaders_.erase(iter);
+  std::move(callback).Run(response_code, *response_body);


response_body can be nullptr if a network error has occurred [1]. It's better just check response_body before dereferencing in case some other corner case will appear in SimpleURLLoader.

See SetAllowPartialResults and this code:
https://github.com/chromium/chromium/blob/4c7435484856fc171e16fa84d9c244fbadd6fa4a/services/network/public/cpp/simple_url_loader.cc#L1541-L1543

goodov · 2022-04-04T07:19:18Z

components/brave_today/browser/brave_news_controller.cc

+            base::StringPiece body_payload(body.data(), body.size());
+            if (response_code < 200 || response_code >= 300 ||
+                (is_padded &&
+                 !brave::PrivateCdnHelper::GetInstance()->RemovePadding(


RemovePadding can be static.

Would prefer to change in a follow-up. This is not introduced in this PR, and would prefer to limit this PR to fix the regression and not refactor.

goodov · 2022-04-04T07:26:22Z

components/brave_today/browser/brave_news_controller.cc

+                     &body_payload))) {
+              // Byte padding removal failed
+              absl::optional<std::vector<uint8_t>> args;
+              std::move(callback).Run(std::move(args));


absl::nullopt?

goodov · 2022-04-04T07:28:52Z

components/brave_private_cdn/private_cdn_request_helper.cc

+    bool auto_retry_on_network_change /* = true */,
+    size_t max_body_size /* = 5 * 1024 * 1024 */) {
+  // Do not allow unbounded max body
+  DCHECK(max_body_size != = -1u);


!= =? 😏

thanks for spotting - fixed

goodov · 2022-04-04T07:31:25Z

components/brave_private_cdn/private_cdn_request_helper.cc

+  request->credentials_mode = network::mojom::CredentialsMode::kOmit;
+  request->method = net::HttpRequestHeaders::kGetMethod;
+  // Don't send any identifying information, such as language or user agent
+  for (auto entry : brave::private_cdn_headers)


brave::private_cdn_headers should be a base::NoDestructor instance accessible (and constructible) via some getter like brave::GetPrivateCDNHeaders().

Would prefer to change in a follow-up. This is not introduced in this PR, and would prefer to limit this PR to fix the regression and not refactor

Create PrivateCDNRequestHelper to help ensure requests are anonymous. APIRequestHelper was no longer appropriate as it requires response to be JSON format.

petemill requested review from fmarier, mariospr, spylogsster and alexsafe March 31, 2022 23:48

petemill self-assigned this Mar 31, 2022

petemill force-pushed the fix-news-image-request branch from 364ba7b to b088c3b Compare April 1, 2022 19:40

petemill requested a review from a team as a code owner April 1, 2022 19:40

petemill force-pushed the fix-news-image-request branch from b088c3b to 033cdc3 Compare April 1, 2022 19:41

fmarier reviewed Apr 2, 2022

View reviewed changes

fmarier approved these changes Apr 3, 2022

View reviewed changes

goodov reviewed Apr 4, 2022

View reviewed changes

petemill force-pushed the fix-news-image-request branch from 033cdc3 to f67bf99 Compare April 4, 2022 22:46

Brave News: Do not use APIRequestHelper for binary data

ed65156

Create PrivateCDNRequestHelper to help ensure requests are anonymous. APIRequestHelper was no longer appropriate as it requires response to be JSON format.

petemill force-pushed the fix-news-image-request branch from f67bf99 to ed65156 Compare April 4, 2022 22:47

petemill requested a review from goodov April 4, 2022 22:48

goodov approved these changes Apr 5, 2022

View reviewed changes

petemill merged commit ddc239e into master Apr 5, 2022

petemill deleted the fix-news-image-request branch April 5, 2022 16:54

petemill added this to the 1.39.x - Nightly milestone Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brave News: Do not use APIRequestHelper for binary data #12857

Brave News: Do not use APIRequestHelper for binary data #12857

petemill commented Mar 31, 2022

fmarier left a comment •

edited

Loading

petemill commented Apr 2, 2022

petemill commented Apr 2, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

goodov Apr 4, 2022

petemill Apr 4, 2022

Brave News: Do not use APIRequestHelper for binary data #12857

Brave News: Do not use APIRequestHelper for binary data #12857

Conversation

petemill commented Mar 31, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fmarier left a comment • edited Loading

Choose a reason for hiding this comment

petemill commented Apr 2, 2022

petemill commented Apr 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier left a comment •

edited

Loading