use yarn.lock when building the apps

[willdurand]

Fix mozilla/addons#12327

[codecov-io]

Codecov Report

Merging #5897 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    mozilla/addons-frontend#5897   +/-   ##
=======================================
  Coverage   97.72%   97.72%           
=======================================
  Files         229      229           
  Lines        5858     5858           
  Branches     1123     1123           
=======================================
  Hits         5725     5725           
  Misses        118      118           
  Partials       15       15

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18923a...9f14a30. Read the comment docs.

[willdurand]

I cannot make the --offline approach work, so I guess this is a good start.

[kumar303]

Let me know if I'm missing something but I don't think this solves https://github.com/mozilla/addons-frontend/issues/3034 (I just updated its description btw). Aside from not solving the issue, I don't see any benefit to adding --pure-lockfile.

As far as I can tell, --pure-lockfile only stops yarn install from generating a lock file, that's it. I cloned the repo in a temp directory, edited the version of a dependency in package.json, and ran yarn install --pure-lockfile. It still tried to install the new dependency version from package.json.

Maybe we need --frozen-lockfile? I think that will fail if package.json is introducing new modules/versions. Getting a failure like this in production/dev/stage is too late, though.

Anyway, I'm not convinced that either option will solve this part of https://github.com/mozilla/addons-frontend/issues/3034 :

A production install should never upgrade a package (or its dependencies) beyond what's in our lock file.

A lock file will always contain loose sub-dependency versions if the package author declared them as such. This means those packages could get silently upgraded in production and that's what I want to avoid.

I think we need to consider a new approach. We either need an offline mirror or something like shrinkwrap.

[willdurand]

Let me know if I'm missing something but I don't think this solves mozilla/addons#10714 (I just updated its description btw). Aside from not solving the issue, I don't see any benefit to adding --pure-lockfile.

As far as I can tell, --pure-lockfile only stops yarn install from generating a lock file, that's it.

It is not clear to me what this option does actually. I re-read the GH issues and it now seems pointless: I don't understand why people keep mentioning it when it comes to looking for "how to install dependencies according to the lock file?". Anyway...

I cloned the repo in a temp directory, edited the version of a dependency in package.json, and ran yarn install --pure-lockfile. It still tried to install the new dependency version from package.json.

That sounds like the correct behavior to me given that you change the package.json file without changing the yarn.lock file. More info here: yarnpkg/yarn#5270 (comment). Those STR are unfortunately not matching what we want to achieve in mozilla/addons#10714 IIRC.

Both files should be synchronized, and that's why we keep pushing Sync yarn.lock with master commits time to time. yarn install does whatever needs to be done to make sure package.json is satisfied, which should be the case because we synchronize both files all the time.

Maybe we need --frozen-lockfile? I think that will fail if package.json is introducing new modules/versions. Getting a failure like this in production/dev/stage is too late, though.

Adding such an option on Travis means we can stop using Greenkeeper because all PRs will be red :/

We could potentially add a Travis job for builds on master, but that means we cannot merge Greenkeeper PRs directly from the web UI (which is fine to me, though).

Anyway, I'm not convinced that either option will solve this part of mozilla/addons#10714 :

A production install should never upgrade a package (or its dependencies) beyond what's in our lock file.

I tend to disagree: that will be the case with this PR given that it adds the yarn.lock file when installing the dependencies in the Docker image build process.

I think we need to consider a new approach. We either need an offline mirror or something like shrinkwrap.

Offline mode is probably too much honestly, we would have to commit 50-60MB of tar files and maintain them. This is probably the best paranoid option, though.

We currently don't use the yarn.lock file at all, we are very lucky that everything works. Having that should partially fix the issue mentioned in the OP.

The second part could be fixed by running yarn check during the Docker image build, which is used by the UI tests IIRC so we would see the errors in travis-ci.

WDYT?

[willdurand]

@kumar303 ^

[bobsilverberg]

@willdurand Do we need to keep this PR open, or are we looking at other alternatives to solve the issue?

[willdurand]

@willdurand Do we need to keep this PR open, or are we looking at other alternatives to solve the issue?

I think it should stay open until I get the chance to talk to @kumar303 about it again, hopefully I'll have an update by the end of this week.

[kumar303]

Sorry for missing the last comment.

I'll come back to this requirement again:

A production install should never upgrade a package or its dependencies beyond what's in our lock file (or something else that declares all package versions).

You mentioned in the last comment that your patch will fix this. How?

Let's take isomorphic-fetch as an example. We require a specific version of it (not a loose version):

addons-frontend/package.json

Line 184 in 9f14a30

"isomorphic-fetch": "2.2.1",

But in the lock file, its dependency on node-fetch is loose (if I understand how lock files work--maybe I don't!):

addons-frontend/yarn.lock

Lines 4992 to 4997 in 9f14a30

	isomorphic-fetch@2.2.1, isomorphic-fetch@^2.1.1:
	version "2.2.1"
	resolved "https://registry.yarnpkg.com/isomorphic-fetch/-/isomorphic-fetch-2.2.1.tgz#611ae1acf14f5e81f729507472819fe9733558a9"
	dependencies:
	node-fetch "^1.0.1"
	whatwg-fetch ">=0.10.0"

What if an attacker hacks an account and publishes 1.0.2 of node-fetch with a coin miner in it? Won't the next production install of addons-frontend pull in 1.0.2 of node-fetch? If not, how does your patch prevent that?

[kumar303]

A production install should never upgrade a package or its dependencies beyond what's in our lock file (or something else that declares all package versions).

I also just realized that I worded this incorrectly. What I mean is to never upgrade any sub-dependency beyond its version that existed at the time we pinned the top-level dependency.

We accomplish this in Python by pinning all package versions (even sub-dependencies) and installing the requirements with --no-deps.

[willdurand]

You mentioned in the last comment that your patch will fix this. How?

@kumar303 mainly by using the yarn.lock file, this file is not used yet so it is a miracle that things work and get installed properly. It can only be better. Yarn folks say that the yarn.lock file ensures reproducible builds (and yarn in general, but does that mean it is SemVer-compliant? Or better?)

According to this issue, frozen-lockfile will only fail if package.json and yarn.lock files are not in sync, which is not what we want either.

I also just realized that I worded this incorrectly. What I mean is to never upgrade any sub-dependency beyond its version that existed at the time we pinned the top-level dependency.

We accomplish this in Python by pinning all package versions (even sub-dependencies) and installing the requirements with --no-deps.

Offline mirror is probably the feature that allows that: https://yarnpkg.com/blog/2016/11/24/offline-mirror/.

To sum up, I really think we need that yarn.lock file as part of the build process, that will be better than without. We can still open a separate issue for the paranoid mode (a.k.a. pinning all package versions of everything). WDYT?

[kumar303]

I really think we need that yarn.lock file as part of the build process, that will be better than without.

I agree. If you want to merge your patch as is then that's fine with me. I don't think it should close mozilla/addons#10714 but you could make another issue to track it like "Make deploy builds more predictable with yarn.lock" or something.

We can still open a separate issue for the paranoid mode (a.k.a. pinning all package versions of everything).

I intended mozilla/addons#10714 to be that issue and I think the bullet point about making sure no packages can be upgraded captures that intent. I will update the issue to clarify what I meant in that sentence.

[willdurand]

@kumar303 alrighty, updated PR title/desc. and created a different issue. r?

[kumar303]