Validation has been added to the build script when the "prod" target is
selected. We now ensure that all expected environment variables are
set, and that no extra environment variables are present (which might
indicate that the wrong configuration file is being used).
The `prod` target uses a new `.metamaskprodrc` configuration file. Each
required variable can be specified either via environment variable or
via this config file. CI will continue set these via environment
variable, but for local manual builds we can use the config file to
simplify the build process and ensure consistency.
A new "dist" target has been added to preserve the ability to build a
"production-like" build without this validation.
The config validation is invoked early in the script, in the CLI
argument parsing step, so that it would fail more quickly. Otherwise
we'd have to wait a few minutes longer for the validation to run.
This required some refactoring, moving functions to the utility module
and moving the config to a dedicated module.
Additionally, support has been added for all environment variables to
be set via the config file. Previously the values `PUBNUB_PUB_KEY`,
`PUBNUB_SUB_KEY`, `SENTRY_DSN`, and `SWAPS_USE_DEV_APIS` could only be
set via environment variable. Now, all of these variables can be set
either way.
Closes#15003
There is a SES bug that results in errors being printed to the console
as `{}`[1]. The known workaround is to print the error stack rather
than printing the error directly. This affects our build script when it
is run with LavaMoat.
We used this workaround in one place in the build script already, but
not in the handler for task errors. We now use it in both places.
The workaround has been moved to a function that we can use throughout
the build script.
[1]: https://github.com/endojs/endo/issues/944
We use the `rc` package to read the `.metamaskrc` configuration file,
which is in "ini" format. This package has been replaced by the `ini`
package.
The `rc` package was not actively maintained, and it has had recent
security vulnerabilities. But most importantly, the config object
returned by `rc` includes a bunch of extra information that made build
script validation [1] difficult to implement. Specifically, it made it
challenging to ensure no extra environment variables were present.
The `ini` package on the other hand is simple, well maintained, and
is simpler to use. This package doesn't add any extra properties to the
object it returns, making validation easy.
[1]: https://github.com/MetaMask/metamask-extension/issues/15003
The "scripts" portion of the build script has been refactored to pass
the "build target" throughout the file. The "build target" is the
target environment for the build, reflected by the command used to
start the build (e.g. "dev", "prod", "test", or "testDev").
Beforehand we derived the variables `devMode` and `testing` from this
build target, and passed these throughout the script. However, there is
a future change [1] that requires adding a new build target that acts
like "prod" in some ways but not others. It was easier to refactor to
pass through `buildTarget` directly than it was to add a _third_
boolean flag to indirectly represent the target.
The existence of the "testDev" target made it convenient to still have
the `testing` and `devMode` flag, so helper functions were added to
derive those values from the build target. I anticipate that these will
only be needed temporarily though. We will probably be able to get rid
of the `testDev` target and the related complexities when we start
adding more flags (like `--watch`[2] and `--minify`[3]) to the build
script directly.
[1]: https://github.com/MetaMask/metamask-extension/issues/15003
[2]: https://github.com/MetaMask/metamask-extension/issues/12767
[3]: https://github.com/MetaMask/metamask-extension/issues/12768
* Fix "app-init" injection
The way we were injecting variables into the `app-init.js` bundle was
accidentally overwriting the bundle output with the raw `app-init.js`
source file. This is a problem because the bundling process handles a
lot of things we care about like source maps, polyfills and other
necessary Babel transformations, environment variable injection, and
minification.
Instead of using string replacement to inject variables, we are now
using environment variables. The old string replacement strategy has
been removed, and the `app-init.js` module is now generated using the
same process as our other bundles.
A new option, "extraEnvironmentVariables", was added to allow us to
inject environment variables specifically for this bundle.
* Add check to ensure APPLY_LAVAMOAT is set
This is a follow-up to #15318, which fixed a problem with environment
variables. Every function in this module that passes options related to
environment variables has been updated with a doc comment. This should
make it clearer which options are mandatory and which are optional,
hopefully preventing a similar mistake from happening in the future.
The environment variables `IN_TEST` and `METAMASK_DEBUG` were not
being set to `false` correctly. Instead those variables were being
skipped, and were resolved to `undefined` at runtime. This is confusing
because the other environment variables do not work that way - they can
be set to false.
The build script has been updated to ensure those two environment
variables are always set to `true` or `false` - never `undefined`.
Additionally, the `METAMASK_VERSION` environment variable was being
omitted from the `app-init.js` bundle. For the sake of consistency,
that has also been restored.
Some of the functions in `development/build/scripts.js` have been
renamed to better describe their function, and to be more consistent
with other similar functions.
Two unused options have been removed from the `createNormalBundle`
function in the build script: 'extraEntries` and `modulesToExpose`.'
Both of these options were used in the old "main" bundles, before we
began using the "factored" bundles. They have been unused since #11080.
This reverts commit f09ab88891, reversing
changes made to effc761e0e.
This is being temporarily reverted to make it easier to release an
urgent fix for v10.15.1.
#14583 broke the development build scripts (e.g. `yarn start`) by adding a positional argument to a package script (`build:dev`) that is used and passed positional arguments in the build script itself. This PR removes the positional argument from the `build:dev` script and `yarn start` now works again. In addition, the `--apply-lavamoat` flag is properly forwarded to child processes, which was not the case in the original implementation.
To test, `yarn start` should work and LavaMoat should _not_ be applied, in distinction to `yarn build:dev dev --apply-lavamoat=true`. Whether LavaMoat is applied can be determined by checking whether `Object.isFrozen(Object.prototype)` is `true` (with LavaMoat) or `false` (without LavaMoat).
Adds a new flag, `--apply-lavamoat`, to the main build script. The flag controls whether LavaMoat is actually applied to the output of the build process. The flag defaults to `true`, but we explicitly set it to `false` in the `start` package script. Meanwhile, the `start:lavamoat` script is modified such that it applies LavaMoat to the build output in development mode, but it no longer runs the build process itself under LavaMoat as there aren't very compelling reasons to do so.
This change is motivated by the fact that development builds do not have their own dedicated LavaMoat policies, which causes development builds to fail since #14537. The downside of this change is that LavaMoat-related failures will not be detected when running `yarn start`. @kumavis has plans for fixing this problem in a future major version of the `@lavamoat` suite.
The phishing warning page URL environment variable has been renamed
from `PHISHING_PAGE_URL` to `PHISHING_WARNING_PAGE_URL`. We call this
page the "phishing warning page" everywhere else, and this name seemed
better suited (it's not a phishing page itself).
The variable has been listed and documented in `.metamaskrc.dist` as
well.
The e2e tests have been updated for `@metamask/phishing-warning@1.1.0`.
The iframe case was updated with a new design, which required test
changes. The third test that was meant to ensure the phishing page
can't redirect to an extension page has been updated to navigate
directly to the phishing warning page and setting the URL manually via
query parameters, as that was the only way to test that redirect.
An externally hosted phishing warning page is now used rather than the
built-in phishing warning page.The phishing page warning URL is set via
configuration file or environment variable. The default URL is either
the expected production URL or `http://localhost:9999/` for e2e testing
environments.
The new external phishing page includes a design change when it is
loaded within an iframe. In that case it now shows a condensed message,
and prompts the user to open the full warning page in a new tab to see
more details or bypass the warning. This is to prevent a clickjacking
attack from safelisting a site without user consent.
The new external phishing page also includes a simple caching service
worker to ensure it continues to work offline (or if our hosting goes
offline), as long as the user has successfully loaded the page at least
once. We also load the page temporarily during the extension startup
process to trigger the service worker installation.
The old phishing page and all related lines have been removed. The
property `web_accessible_resources` has also been removed from the
manifest. The only entry apart from the phishing page was `inpage.js`,
and we don't need that to be web accessible anymore because we inject
the script inline into each page rather than loading the file directly.
New e2e tests have been added to cover more phishing warning page
functionality, including the "safelist" action and the "iframe" case.
#14583 broke the development build scripts (e.g. `yarn start`) by adding a positional argument to a package script (`build:dev`) that is used and passed positional arguments in the build script itself. This PR removes the positional argument from the `build:dev` script and `yarn start` now works again. In addition, the `--apply-lavamoat` flag is properly forwarded to child processes, which was not the case in the original implementation.
To test, `yarn start` should work and LavaMoat should _not_ be applied, in distinction to `yarn build:dev dev --apply-lavamoat=true`. Whether LavaMoat is applied can be determined by checking whether `Object.isFrozen(Object.prototype)` is `true` (with LavaMoat) or `false` (without LavaMoat).
Adds a new flag, `--apply-lavamoat`, to the main build script. The flag controls whether LavaMoat is actually applied to the output of the build process. The flag defaults to `true`, but we explicitly set it to `false` in the `start` package script. Meanwhile, the `start:lavamoat` script is modified such that it applies LavaMoat to the build output in development mode, but it no longer runs the build process itself under LavaMoat as there aren't very compelling reasons to do so.
This change is motivated by the fact that development builds do not have their own dedicated LavaMoat policies, which causes development builds to fail since #14537. The downside of this change is that LavaMoat-related failures will not be detected when running `yarn start`. @kumavis has plans for fixing this problem in a future major version of the `@lavamoat` suite.
* lavamoat - apply lavamoat protections to popup and notification
* build - enable lavamoat for home
* lavamoat - add missing ui overrides for react family
* deps/patches - patch zxcvbn for ses compat
Certain build steps accidentally omitted the `version` variable. It has
now been restored to all steps, ensuring that all environment variables
are correctly injected into all bundles.
A check has been added to the Sentry setup module to ensure the release
is not omitted in the future.
Certain build steps accidentally omitted the `version` variable. It has
now been restored to all steps, ensuring that all environment variables
are correctly injected into all bundles.
A check has been added to the Sentry setup module to ensure the release
is not omitted in the future.
This commit modifies the build system so that TypeScript files can be
transpiled into ES5 just like JavaScript files.
Note that this commit does NOT change the build system to run TypeScript
files through the TypeScript compiler. In other words, no files will be
type-checked at the build stage, as we expect type-checking to be
handled elsewhere (live, via your editor integration with `tsserver`,
and before a PR is merged, via `yarn lint`). Rather, we merely instruct
Babel to strip TypeScript-specific syntax from any files that have it,
as if those files had been written using JavaScript syntax alone.
Why take this approach? Because it prevents the build process from being
negatively impacted with respect to performance (as TypeScript takes a
significant amount of time to run).
It's worth noting the downside of this approach: because we aren't
running files through TypeScript, but relying on Babel's [TypeScript
transform][1] to identify TypeScript syntax, this transform has to keep
up with any syntax changes that TypeScript adds in the future. In fact
there are a few syntactical forms that Babel already does not recognize.
These forms are rare or are deprecated by TypeScript, so I don't
consider them to be a blocker, but it's worth noting just in case it
comes up later. Also, any settings we place in `tsconfig.json` will be
completely ignored by Babel. Again, this isn't a blocker because there
are some analogs for the most important settings reflected in the
options we can pass to the transform. These and other caveats are
detailed in the [documentation for the transform][2].
[1]: https://babeljs.io/docs/en/babel-plugin-transform-typescript
[2]: https://babeljs.io/docs/en/babel-plugin-transform-typescript#caveats
The version of a build is now derived from both the `version` field in
`package.json` and the requested build type and version. The build type
and version are added onto the manifest version as a suffix, according
to the SemVer prerelease format.
We already have support in the extension for versions of this format,
but to apply a Flask or Beta version required manual updates to
`package.json`. Now it can be done just with build arguments.
A `get-version` module was created to make it easier to generate the
version in the various places we do that during the build. It was
created in the `development/lib` directory because it will be used by
other non-build development scripts in a future PR.
The `BuildType` constant was extracted to its own module as well, and
moved to the `development/lib` directory. This was to make it clear
that it's used by various different development scripts, not just the
build.
The version of a build is now derived from both the `version` field in
`package.json` and the requested build type and version. The build type
and version are added onto the manifest version as a suffix, according
to the SemVer prerelease format.
We already have support in the extension for versions of this format,
but to apply a Flask or Beta version required manual updates to
`package.json`. Now it can be done just with build arguments.
A `get-version` module was created to make it easier to generate the
version in the various places we do that during the build. It was
created in the `development/lib` directory because it will be used by
other non-build development scripts in a future PR.
The `BuildType` constant was extracted to its own module as well, and
moved to the `development/lib` directory. This was to make it clear
that it's used by various different development scripts, not just the
build.
If an error occurs while running Browserify, the stream that Browserify
creates will emit an `error` event. However, this event is not being
handled, so Node will catch it instead. But the error message it
produces is very nebulous, as it merely spits out the stream object and
completely ignores the actual error that occurred. So this commit
listens for the `error` event and outputs the error.
One note here is that when we are outputting the error, we must get
around a bug that exists in Endo where if you pass an Error object to
`console.{log,error,info,debug}` then you will just see `{}` on-screen.
We get around this by printing `err.stack`.
ESLint rules have been added to enforce our JSDoc conventions. These
rules were introduced by updating `@metamask/eslint-config` to v9.
Some of the rules have been disabled because the effort to fix all lint
errors was too high. It might be easiest to enable these rules one
directory at a time, or one rule at a time.
Most of the changes in this PR were a result of running
`yarn lint:fix`. There were a handful of manual changes that seemed
obvious and simple to make. Anything beyond that and the rule was left
disabled.
The ESLint config for the extension explicitly includes support for
Prettier. However, this is already being provided by our global ESLint
config (`@metamask/eslint-config`). Therefore there is no need to
include it here. In fact, this is causing weird issues where the `curly`
option is getting overridden somehow. After this change, these syntaxes
are invalid:
``` javascript
if (foo) return;
```
``` javascript
if (foo) return 'bar';
```
`remote-redux-devtools` is now explicitly excluded and disabled in non-
dev builds, and in the `testDev` build. This was causing console errors
in the `testDev` build during e2e tests, which would cause certain
tests to fail.
This was already only supposed to be enabled for development builds,
but this library used the `NODE_ENV` environment variable to make that
determination. This gives us more control over when it's disabled.
The React dev tools can result in console errors if dev tools is not
open during the test. Some of our e2e tests fail if there are any
console errors, so these errors break those tests.
`react-devtools` has been completely disabled for `testDev` builds to
make debugging e2e tests easier. The React dev tools can still be used
from development builds.
A propType error was showing up during e2e tests with a `testDev`
build. It was caused by `process.env.IN_TEST` being treated as a
boolean, when in fact it is either the string `'true'` or a boolean.
`IN_TEST` has been updated to always be a boolean. `loose-envify` has
no trouble injecting boolean values, so there's no reason to treat this
as a string.
The environment variables used for test builds was wrong for certain
bundles because the `testing` flag wasn't passed through to the
function that determines which environment variables to inject.
Effectively this means that test builds on `master` were going to the
production `metamask` Sentry project rather than the `test-metamask`
project. This has been the case since #11080.
The `testing` flag is now included for all bundles, and test builds now
use the `test-metamask` Sentry project in all cases.
Mark Stacey
Niranjana Binoy
Sam Gbafa
Brad Decker
Jyoti Puri
ryanml
Dan J Miller
kumavis
Erik Marks
Elliot Winkler
David Walsh