Issue 5690: Add key for brave services #3198

jumde · 2019-08-16T18:19:13Z

Fix brave/brave-browser#5690

Submitter Checklist:

Test Plan:

Open Fiddler and enable SSL proxying: Settings > HTTPS > Decrypt HTTPS Traffic
Download the root cert and enable trust for the cert using Keychain Access
Verify that all requests to *.brave.com have the header `BraveServiceKey: dummytoken

Services:

Remaining proxies are currently blocked on: brave/brave-browser#6331

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

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

bridiver · 2019-08-20T16:34:47Z

browser/net/brave_static_redirect_network_delegate_helper.cc

@@ -8,13 +8,35 @@
 #include <memory>
 #include <vector>

+#include "base/strings/stringprintf.h"


bridiver · 2019-08-20T16:35:42Z

browser/net/brave_static_redirect_network_delegate_helper.cc

+  static URLPattern proxy_pattern(URLPattern::SCHEME_HTTP |
+      URLPattern::SCHEME_HTTPS, kBraveProxyPattern);
+
+  if (proxy_pattern.MatchesURL(ctx->request_url)) {


I'm not sure this is a viable method to use anymore for headers with network service - cc @iefremov

I'm also not sure this was ever really the right place to add headers

why not? I tested OnBeforeStartTransaction_SiteHacksWork and it worked for UA overriding. It should be safe for non-restricted headers (Cookies, Referer, etc)

It's definitely not the right place for UA overriding because there are methods specifically for that. The other method I was thinking of was NavigationHandle::SetRequestHeaders but that only works for main/subframe navigations

fmarier · 2019-09-16T17:56:18Z

browser/net/brave_system_request_handler.cc

 namespace brave {

+void AddBraveServicesKeyHeader(network::ResourceRequest* url_request) {
+  static URLPattern proxy_pattern(URLPattern::SCHEME_HTTP |


Do we need HTTP here? I'm pretty sure all of our proxies are HTTPS only.

fmarier · 2019-09-16T17:57:19Z

common/network_constants.cc

@@ -5,6 +5,7 @@

 #include "brave/common/network_constants.h"

+const char kBraveProxyPattern[] = "*://*.brave.com/*";


This could be https://*.brave.com/* I think.

iefremov · 2019-10-09T11:58:11Z

browser/net/BUILD.gn

+
+defines = []
+if (brave_services_key != "") {
+  defines += [ "BRAVE_SERVICES_KEY=\"$brave_services_key\"" ]


mind adding a comment describing this?

iefremov · 2019-10-09T11:58:27Z

browser/net/brave_system_request_handler.cc

 #include "services/network/public/cpp/resource_request.h"
 #include "url/gurl.h"

+#if !defined(BRAVE_SERVICES_KEY)
+#define BRAVE_SERVICES_KEY "dummytoken"


I'd like to see some comments here

iefremov · 2019-10-09T11:58:55Z

browser/net/brave_system_request_handler.cc

+                                  kBraveProxyPattern);
+  if (proxy_pattern.MatchesURL(url_request->url)) {
+      url_request->headers.SetHeaderIfMissing(kBraveServicesKeyHeader,
+                                          BRAVE_SERVICES_KEY);


clang-format

iefremov · 2019-10-09T11:59:31Z

browser/net/brave_system_request_handler_unittest.cc

@@ -0,0 +1,62 @@
+/* Copyright (c) 2019 The Brave Authors. All rights reserved.


Can we add a browser test?

iefremov · 2019-10-09T12:01:12Z

browser/net/brave_system_request_handler_unittest.cc

+  net::TestURLRequestContext* context() { return context_.get(); }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;


It seems to me that we don't need all this stuff for simple tests

iefremov

There are some minors

iefremov · 2019-10-14T17:41:55Z

browser/net/brave_system_request_handler_browsertest.cc

+
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    EXPECT_TRUE(https_server_.Start());


I'd use ASSERT_TRUE

iefremov · 2019-10-14T17:42:08Z

browser/net/brave_system_request_handler_browsertest.cc

+    return loader_factory_;
+  }
+
+  void SetUp() override { InProcessBrowserTest::SetUp(); }


iefremov · 2019-10-14T17:42:39Z

browser/net/brave_system_request_handler_browsertest.cc

+                          ->GetSharedURLLoaderFactory();
+  }
+
+  bool LoadURL(std::string host) {


const std::string& or base::StringPiece

iefremov · 2019-10-14T17:43:24Z

browser/net/brave_system_request_handler_browsertest.cc

+  }
+
+  bool LoadURL(std::string host) {
+    std::unique_ptr<network::ResourceRequest> request =


auto here and below

iefremov · 2019-10-14T17:44:53Z

browser/net/brave_system_request_handler_browsertest.cc

+  scoped_refptr<network::SharedURLLoaderFactory> loader_factory_ = nullptr;
+};
+
+IN_PROC_BROWSER_TEST_F(SystemNetworkContextManagerBrowsertest,


Many thanks for making the test!

iefremov · 2019-10-16T16:19:30Z

browser/net/brave_system_request_handler_unittest.cc

+
+namespace brave {
+
+class BraveSystemRequestHandlerTest : public testing::Test {


you don't need to have an empty class, just use TEST(BraveSystemRequestHandlerTest, AddBraveServiceKeyHeader) for tests

iefremov · 2019-10-16T16:19:50Z

browser/net/brave_system_request_handler_unittest.cc

+};
+
+TEST_F(BraveSystemRequestHandlerTest, AddBraveServiceKeyHeader) {
+    GURL url("https://demo.brave.com");


clang-format

iefremov · 2019-10-16T16:20:23Z

browser/net/brave_system_request_handler_unittest.cc

+};
+
+TEST_F(BraveSystemRequestHandlerTest, AddBraveServiceKeyHeader) {
+    GURL url("https://demo.brave.com");


const GURL

iefremov · 2019-10-16T16:21:09Z

browser/net/brave_system_request_handler_unittest.cc

+
+#include "brave/browser/net/url_context.h"
+#include "brave/common/network_constants.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"


most of includes are not needed - these three below and url_context.h

jumde added the Uplift/Blocked label Aug 16, 2019

jumde self-assigned this Aug 16, 2019

jumde mentioned this pull request Aug 16, 2019

Issue 5691: Add brave service key to the npm config brave/brave-browser#5695

Merged

28 tasks

jumde removed the Uplift/Blocked label Aug 16, 2019

jumde changed the title ~~Issue 5690: Add key for brave services~~ [WIP] Issue 5690: Add key for brave services Aug 16, 2019

jumde force-pushed the keyed_apis branch from 6791566 to 39624e3 Compare August 16, 2019 18:54

jumde changed the title ~~[WIP] Issue 5690: Add key for brave services~~ Issue 5690: Add key for brave services Aug 16, 2019

jumde force-pushed the keyed_apis branch from 39624e3 to 97ea15d Compare August 19, 2019 17:48

jumde changed the title ~~Issue 5690: Add key for brave services~~ [WIP] Issue 5690: Add key for brave services Aug 19, 2019

jumde force-pushed the keyed_apis branch from 97ea15d to cfb37a5 Compare August 19, 2019 18:02

jumde requested review from bsclifton, bbondy, bridiver and diracdeltas August 19, 2019 19:41

jumde changed the title ~~[WIP] Issue 5690: Add key for brave services~~ Issue 5690: Add key for brave services Aug 20, 2019

bridiver reviewed Aug 20, 2019

View reviewed changes

jumde changed the title ~~Issue 5690: Add key for brave services~~ [WIP] Issue 5690: Add key for brave services Aug 20, 2019

jumde force-pushed the keyed_apis branch 3 times, most recently from 4548cb6 to 70a808f Compare August 20, 2019 22:42

fmarier reviewed Sep 16, 2019

View reviewed changes

jumde force-pushed the keyed_apis branch 2 times, most recently from 49cc058 to 14bbc69 Compare September 30, 2019 16:57

jumde force-pushed the keyed_apis branch 3 times, most recently from 0d48e43 to 9771069 Compare October 8, 2019 00:35

jumde changed the title ~~[WIP] Issue 5690: Add key for brave services~~ Issue 5690: Add key for brave services Oct 8, 2019

jumde force-pushed the keyed_apis branch from 9771069 to 123ac6e Compare October 8, 2019 04:29

jumde force-pushed the keyed_apis branch 3 times, most recently from f07fe4d to e9a934c Compare October 8, 2019 15:08

iefremov reviewed Oct 9, 2019

View reviewed changes

iefremov requested changes Oct 9, 2019

View reviewed changes

jumde force-pushed the keyed_apis branch 7 times, most recently from 2327e50 to 117d8a4 Compare October 14, 2019 16:41

iefremov previously approved these changes Oct 14, 2019

View reviewed changes

jumde dismissed iefremov’s stale review via 08f7e83 October 15, 2019 03:07

jumde force-pushed the keyed_apis branch from 117d8a4 to 08f7e83 Compare October 15, 2019 03:07

iefremov reviewed Oct 16, 2019

View reviewed changes

iefremov approved these changes Oct 16, 2019

View reviewed changes

Issue 5690: Add key for brave services

da9fe51

jumde force-pushed the keyed_apis branch from 08f7e83 to da9fe51 Compare October 16, 2019 21:16

jumde merged commit 869deea into master Oct 16, 2019

jumde added this to the 0.73.x - Nightly milestone Oct 17, 2019

jumde deleted the keyed_apis branch October 23, 2019 16:27

kjozwiak mentioned this pull request Oct 29, 2019

Add key for brave services brave/brave-browser#5690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5690: Add key for brave services #3198

Issue 5690: Add key for brave services #3198

jumde commented Aug 16, 2019 •

edited

Loading

bridiver Aug 20, 2019

bridiver Aug 20, 2019 •

edited

Loading

bridiver Aug 20, 2019

iefremov Aug 20, 2019

bridiver Aug 20, 2019

fmarier Sep 16, 2019

fmarier Sep 16, 2019

iefremov Oct 9, 2019

jumde Oct 10, 2019

iefremov Oct 9, 2019

jumde Oct 10, 2019

iefremov Oct 9, 2019

iefremov Oct 9, 2019

jumde Oct 11, 2019

iefremov Oct 9, 2019

iefremov left a comment

iefremov Oct 14, 2019

iefremov Oct 14, 2019

iefremov Oct 14, 2019

iefremov Oct 14, 2019

iefremov Oct 14, 2019

iefremov Oct 16, 2019

iefremov Oct 16, 2019

iefremov Oct 16, 2019

iefremov Oct 16, 2019

		@@ -5,6 +5,7 @@

		#include "brave/common/network_constants.h"

		const char kBraveProxyPattern[] = "://.brave.com/*";

		@@ -0,0 +1,62 @@
		/* Copyright (c) 2019 The Brave Authors. All rights reserved.


		namespace brave {

		class BraveSystemRequestHandlerTest : public testing::Test {

Issue 5690: Add key for brave services #3198

Issue 5690: Add key for brave services #3198

Conversation

jumde commented Aug 16, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

bridiver Aug 20, 2019 • edited Loading

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

iefremov left a 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

jumde commented Aug 16, 2019 •

edited

Loading

bridiver Aug 20, 2019 •

edited

Loading