Skip to content

Commit

Permalink
refactor: make inpage.js inject itself into the MAIN world (manifes…
Browse files Browse the repository at this point in the history
…t v2 only) (#22524)

## **Description**

I'm working on a new build process that is primary focused on
compilation speed and simplicity. The current manifest v2 way of
injecting inpage.js makes that task harder.

Affects manifest v2 only.

This PR changes the way inpage.js is injected into the MAIN world by
decoupling `inpage.js` from `contentscript.js`.

This new way is very similar to the old way, except `inpage.js` now
injects _itself_ into the document (MAIN world).

One potential issue with this new way is that `contentscript.js` used to
conditionally inject `inpage.js` into the MAIN world, but with this
change the `inpage.js` file is just another "content_script". This might
not be a problem though, as `inpage.js` itself checks all of the same
conditions `contentscript.js` does, i.e., they both check
`shouldInjectProvider()`, but the difference is that _only_ `inpage.js`
decides itself if it will inject a provider into the MAIN world, whereas
before it was up to both contentscript.js AND inpage.js to agree to
inject the provider.

To reiterate why I want to make this change: the current system requires
custom logic specifically for `contentscript.js` and `inpage.js`, and
inpage.js MUST complete compilation before `contentscript.js` can be
start to be compiled. Additionally, `contentscript.js` needs special
treatment of `fs.readFileSync` in order to inline `inpage.js` as text
into itself. This complicates and slows down the build system.

---

One thing this PR does NOT do is speed up the build, as I didn't change
the order of compilation. `inpage.js` is still compiled before
`contentscript.js`, even though `contentscript.js` no longer depends on
`inpage.js`. I a) didn't know how to make this change, and b) was afraid
it'd complicate an already perhaps dubious PR.

---

Some things to look out for in testing: will this new way behave if the
page is not an HTML doctype, is an XML/PDF document, doesn't have a
`documentElement`, is on the blocked domains list, etc.

---

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to any web page not on out block list
2. Open devtools in your browser
3. Type `window.ethereum` and press <enter> in the Console
4. A `Proxy` object should be logged (_not_ `undefined`)

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: weizman <weizmangal@gmail.com>
  • Loading branch information
davidmurdoch and weizman authored Feb 8, 2024
1 parent 423268c commit ba3a20f
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 36 deletions.
3 changes: 2 additions & 1 deletion app/manifest/v2/_base.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
"lockdown-install.js",
"lockdown-run.js",
"lockdown-more.js",
"contentscript.js"
"contentscript.js",
"inpage.js"
],
"run_at": "document_start",
"all_frames": true
Expand Down
31 changes: 0 additions & 31 deletions app/scripts/contentscript.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,6 @@ import { checkForLastError } from '../../shared/modules/browser-runtime.utils';
import { isManifestV3 } from '../../shared/modules/mv3.utils';
import shouldInjectProvider from '../../shared/modules/provider-injection';

// These require calls need to use require to be statically recognized by browserify
const fs = require('fs');
const path = require('path');

const inpageContent = fs.readFileSync(
path.join(__dirname, '..', '..', 'dist', 'chrome', 'inpage.js'),
'utf8',
);
const inpageBundle = inpageContent;

// contexts
const CONTENT_SCRIPT = 'metamask-contentscript';
const INPAGE = 'metamask-inpage';
Expand Down Expand Up @@ -61,24 +51,6 @@ let extensionMux,
pageMux,
pageChannel;

/**
* Injects a script tag into the current document
*
* @param {string} content - Code to be executed in the current document
*/
function injectScript(content) {
try {
const container = document.head || document.documentElement;
const scriptTag = document.createElement('script');
scriptTag.setAttribute('async', 'false');
scriptTag.textContent = content;
container.insertBefore(scriptTag, container.children[0]);
container.removeChild(scriptTag);
} catch (error) {
console.error('MetaMask: Provider injection failed.', error);
}
}

/**
* PHISHING STREAM LOGIC
*/
Expand Down Expand Up @@ -545,9 +517,6 @@ const start = () => {
}

if (shouldInjectProvider()) {
if (!isManifestV3) {
injectScript(inpageBundle);
}
initStreams();

// https://bugs.chromium.org/p/chromium/issues/detail?id=1457040
Expand Down
23 changes: 22 additions & 1 deletion development/build/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const { callbackify } = require('util');
const path = require('path');
const { writeFileSync, readFileSync } = require('fs');
const { writeFileSync, readFileSync, unlinkSync } = require('fs');
const EventEmitter = require('events');
const assert = require('assert');
const gulp = require('gulp');
Expand Down Expand Up @@ -43,6 +43,7 @@ const {
getBuildName,
getBuildAppId,
getBuildIcon,
makeSelfInjecting,
} = require('./utils');

const {
Expand Down Expand Up @@ -471,6 +472,26 @@ function createScriptTasks({
version,
applyLavaMoat,
}),
() => {
// MV3 injects inpage into the tab's main world, but in MV2 we need
// to do it manually:
if (process.env.ENABLE_MV3) {
return;
}
// stringify inpage.js into itself, and then make it inject itself into the page
browserPlatforms.forEach((browser) => {
makeSelfInjecting(
path.join(__dirname, `../../dist/${browser}/${inpage}.js`),
);
});
// delete the inpage.js source map, as it no longer represents inpage.js
// and so `yarn source-map-explorer` can't handle it. It's also not
// useful anyway, as inpage.js is injected as a `script.textContent`,
// and not tracked in Sentry or browsers devtools anyway.
unlinkSync(
path.join(__dirname, `../../dist/sourcemaps/${inpage}.js.map`),
);
},
createNormalBundle({
buildTarget,
buildType,
Expand Down
18 changes: 17 additions & 1 deletion development/build/utils.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const path = require('path');
const { readFileSync } = require('fs');
const { readFileSync, writeFileSync } = require('fs');
const semver = require('semver');
const { capitalize } = require('lodash');
const { loadBuildTypesConfig } = require('../lib/build-type');
Expand Down Expand Up @@ -287,6 +287,21 @@ function getBuildIcon({ buildType }) {
const svg = readFileSync(svgLogoPath, 'utf8');
return `data:image/svg+xml,${encodeURIComponent(svg)}`;
}
/**
* Takes the given JavaScript file at `filePath` and replaces its contents with
* a script that injects the original file contents into the document in which
* the file is loaded. Useful for MV2 extensions to run scripts synchronously in the
* "MAIN" world.
*
* @param {string} filePath - The path to the file to convert to a self-injecting
* script.
*/
function makeSelfInjecting(filePath) {
const fileContents = readFileSync(filePath, 'utf8');
const textContent = JSON.stringify(fileContents);
const js = `{let d=document,s=d.createElement('script');s.textContent=${textContent};d.documentElement.appendChild(s).remove();}`;
writeFileSync(filePath, js, 'utf8');
}

module.exports = {
getBrowserVersionMap,
Expand All @@ -299,4 +314,5 @@ module.exports = {
logError,
getPathInsideNodeModules,
wrapAgainstScuttling,
makeSelfInjecting,
};
4 changes: 2 additions & 2 deletions development/sourcemap-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ async function start() {
`common-0.js`,
`background-0.js`,
`ui-0.js`,
// `contentscript.js`, skipped because the validator is erroneously sampling the inlined `inpage.js` script
`inpage.js`,
`contentscript.js`,
// `inpage.js`, skipped because the validator can't sample the inlined `inpage.js` script
];
let valid = true;

Expand Down

0 comments on commit ba3a20f

Please sign in to comment.