Encouraging a clean git history makes for a more readable log of file changes.
Commit message are much more readable when they talk about features or bug fixes (as described in the existing checklist item about good commit descriptions).
Aim is to prevent a lot of commits which talk about the journey of a PR, such as fixing CI build issues and addressing review feedback, rather than talking about improvements to the project.
* Rework npm run network-audit into a browser test suite
This change adds a new browser test to brave_browser_tests that runs
similar tests to what Brave used to do when running the network-audit
npm script, via the --network_log command line parameter.
As with that previous script, this test makes sure Brave is launched
with --net-log-capture-mode=Everything and --log-net-log, and that
the network_log.json and network-audit-results.json files are written
to disk after the network audit process is completed. However, in this
case we're also prepending the name of the browser test to the names
of such files, so that we can have more than one browser test without
overriding each others' results.
Last, this tests adds two browser tests to check both the case of
loading the simple.html file via the embedded test server and the
more real world scenario of loading brave://welcome, in a final
attempt to replicate what npm run network-audit did for me on a
recent build (and also as a way to validate that having multiple
browser tests will work fine).
Resolves https://github.com/brave/brave-browser/issues/7207
Resolves https://github.com/brave/brave-browser/issues/7281
* Remove all trace of npm run network-audit and replace it where needed
Drop code related to the npm run network-audit command, including the
command itself, and then make sure that the newly added browser tests
are run as part of the npm run test-security command.
Also, this change moves the definitions of the three whitelists used
by the network audit process (i.e. protocols, prefixes and patterns)
to a separate header file so that we can adapt .github/CODEOWNERS to
only cover those lists and not the browser tests themselves.
Last, also adapt Jenkinsfile to remove all references to the former
npm run network-audit command.
* Remove Google-owned prefixes and patterns from network audit whitelist
As noted by @diracdeltas on Slack, these entries should be removed as
they correspond with internal 307 responses that should not be going
out to the network, so we're dropping them as part of this work.
* Extract brave_network_audit_browsertest.cc to a separate binary
A proper re-implementation of these tests will require to "leave the
browser open" for at least 2 minutes, for which we'd need to pass a
big timeout parameters when running the browser tests, delaying the
execution of the brave_browser_tests for no good reason.
Instead, we create a new test suite 'brave_network_audit_tests' that
will exclusively perform the network audit process, so that we can
specify a different timeout only for it (in a follow-up patch).
* Force BraveNetworkAuditTest tests to keep the browser open after load
Similarly to what was done via the npm run network-audit script, we
make sure that we wait ~2 minutes after loading an URL to make sure
we gather enough information (i.e. network requests) before verifying
that no allowed URL requests are made during that time.
In one hand, this means explicitly waiting test until such amount of
time has passed in the tests themselves. In the other hand, this also
means passing specific timeout values to the brave_network_audit_tests
when running it via the npm run test-security script.
* Simplify BraveNetworkAuditTest test suite by having one browser test
Having separate tests was nice in order to be able to run them in
parallel but, at the same time, was probably not necessary because
we want to test the behaviour of firing up one browser and monitoring
network requests after loading some URLs, so probably better to go
back to having one test only.
Additionally, this allows simplifying a bit the creation of the json
files with the log of the requests and the result of the audit, which
don't need having a test-based prefix anymore.
* Whitelist https://laptop-updates-staging.brave.com for network audit
This staging-related URL might be present on devs' machines if they
have it set on their .npmrc file, for instance, so let's add it to
the whitelisted entries similarly to how other staging-related URLs
are already present there.
* Don't whitelist http://componentupdater.brave.com/service/update2
As suggested by @diracdeltas, we can remove the HTTP version of this
URL from the whitelist for the network audit (see [1]).
[1] github.com/brave/brave-core/pull/10389#discussion_r723587821
* Increase timeout for network-audit browser tests to 5min after loads
It seems some network requests might take longer than 2 minutes to
happen, so let's increase the timeout to 5 minutes after loading the
URLs, and adapt the caller script to account for that.
* Also check network requests on brave://rewards with Brave Rewards enabled
Add one more case to the network audit process to double check whether
only allowed network requests happen when Brave Rewards is enabled.
* Whitelist Brave Rewards-related URL prefixes for the network audit
This means whitelisting the following prefixes, found to be hit in
the test after enabling Rewards and waiting for ~5 minutes:
On production environments (e.g. Relese builds on CI):
- https://api.rewards.brave.com/v1/parameters
- https://rewards.brave.com/publishers/prefix-list
- https://grant.rewards.brave.com/v1/promotions
On development environments:
- https://api.rewards.bravesoftware.com/v1/parameters",
- https://rewards-stg.bravesoftware.com/publishers/prefix-list",
- https://grant.rewards.bravesoftware.com/v1/promotions",
* Restore network-audit and define test-security in terms of it
* Disable the ThinLTO cache for the brave_network_audit_tests GN target
We need to disable the ThinLTO cache or the linker will die with
a "Resource temporarily unavailable" error due to the linking
reaching the vm.max_map_count limit of 65530 memory mappings.
* Use JSONFileValueSerializer instead of base::JSONWriter
* Replaced use of deprecated APIs with the correct ones
github autogenerated a file that was invalid because of our branch-matching regex. this manually fixes it.
also skips the run for c++/java because codeql doesn't know how to build
those
fix https://github.com/brave/brave-browser/issues/13431
Resolves https://github.com/brave/brave-browser/issues/12691
The idea is to clean up the checklist and leave only relevant
and important entries. Also populate the reviewers section with
frequently overlooked issues.
Francois Marier