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

ci - install deps with "--har" flag to capture network activity #7143

Merged
merged 4 commits into from
Sep 11, 2019

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Sep 10, 2019

Add yarn install HTTP Archive (HAR) file to build-artifacts.
The intention of this is to have some documentation on provenance of dependency sources for use in incident response.

If something weird ends up in our bundle, we should be able to clearly identify how it got there. This is intended to be one piece of that puzzle.

https://yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-har
https://en.wikipedia.org/wiki/HAR_(file_format)
https://w3c.github.io/web-performance/specs/HAR/Overview.html

@metamaskbot
Copy link
Collaborator

Builds ready [eddf498]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [9ada8c2]: chrome, firefox, edge, opera

@kumavis
Copy link
Member Author

kumavis commented Sep 10, 2019

You can see the generated har file here (at 9.6 MB its kinda massive), it claimed to take 0 sec to upload

@kumavis
Copy link
Member Author

kumavis commented Sep 10, 2019

the generated har file does not have the downloaded content which may significantly reduce the value of this. it does include lots of metadata that might be useful when working with external security teams like yarn/npm/cloudflare.

we may not need this for every prerelease

@kumavis
Copy link
Member Author

kumavis commented Sep 10, 2019

as an alternative we could explore capturing pcaps for releases, but they may be very large

Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool to me, but would love to have our yarn expert @Gudahtt also approve it.

@Gudahtt
Copy link
Member

Gudahtt commented Sep 10, 2019

It definitely couldn't hurt to collect HAR files. I'm not entirely sure when they'd be useful, but who knows.

we may not need this for every prerelease

I agree - it would be nice to collect this only for actual releases, perhaps.

CircleCI v2.1 does allow conditional steps within jobs. Maybe we could just run that step on release branches (and on master, unless/until we decide to get rid of it)

@metamaskbot
Copy link
Collaborator

Builds ready [141c76e]: chrome, firefox, edge, opera

@kumavis
Copy link
Member Author

kumavis commented Sep 11, 2019

It definitely couldn't hurt to collect HAR files. I'm not entirely sure when they'd be useful, but who knows.

yes, better to have something in the event of an incident, but I really would prefer to have something that includes request content so we can try to trace. maybe a tarball of node_modules after a yarn --ignore-scripts? I wish there was a way to ignore scripts and then run them later. 1) download 2) snapshot 3) run scripts. the copay hack involved changing the content of other packages in node_modules when run. so having some records from install time would help identify where it actually came from.

@kumavis
Copy link
Member Author

kumavis commented Sep 11, 2019

there doesn't seem to be any performance impact

@kumavis
Copy link
Member Author

kumavis commented Sep 11, 2019

I looked into adding logic for doing it conditionally but it was not super fun
We may try to do too many things in circleCI yml that would be easier in bash

anyways I think this can be merged as is. if getting HAR always causes a problem, we can revisit

Gudahtt
Gudahtt previously approved these changes Sep 11, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really would prefer to have something that includes request content so we can try to trace. maybe a tarball of node_modules after a yarn --ignore-scripts? I wish there was a way to ignore scripts and then run them later.

I think there is - we could use the Yarn cache. It's in $(yarn cache dir), and it contains an un-archived cache of all packages downloaded during the installation. Yarn also allows installing from just the cache ("offline mode"), so we should be able to reproduce the install phase completely (including install scripts). This would mean saving a lot of files though, so we'd definitely want it to be conditional.

I think this can be merged as is. if getting HAR always causes a problem, we can revisit

Sure, sounds good 👍

@metamaskbot
Copy link
Collaborator

Builds ready [141c76e]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [c094475]

@kumavis kumavis merged commit dbbf698 into develop Sep 11, 2019
@kumavis kumavis deleted the yarn-har branch September 11, 2019 16:03
Gudahtt added a commit that referenced this pull request Sep 17, 2019
…evelop

* origin/develop: (31 commits)
  Performance: Delivery optimized images (#7176)
  Add `appName` message to each locale
  Remove the disk store (#7170)
  Update @hapi/subtext as per security advisory (#7172)
  Add fixes for German translations (#7168)
  Fix recipient field of approve screen (#7171)
  3box integration 2.0 (#6972)
  ci - metamaskbot - include links to dep-viz and all artifacts (#7155)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Remove redundant error logging (#7158)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  ci - install deps with "--har" flag to capture network activity (#7143)
  ci - create source-map-explorer build-artifacts (#7141)
  ci - build-artifacts - generate sesify-viz for inspecting deps (#7151)
  Publish GitHub release from master branch (#7136)
  fix rinkeby spelling (#7148)
  deps - move gulp-terser-js to devDeps
  test:integration - fix renamed test data file
  lint fix
  ...
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.

4 participants