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

Show warning when using Math.random #950

Merged
merged 2 commits into from
Nov 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/snaps-browserify-plugin/src/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import {
DEFAULT_SNAP_BUNDLE,
getSnapManifest,
} from '@metamask/snaps-utils/test-utils';
import { checkManifest, evalBundle } from '@metamask/snaps-utils';
import {
checkManifest,
evalBundle,
PostProcessWarning,
} from '@metamask/snaps-utils';
import plugin, { Options, SnapsBrowserifyTransform } from './plugin';

jest.mock('fs');
Expand Down Expand Up @@ -129,6 +133,18 @@ describe('plugin', () => {
expect(result).toMatchSnapshot();
});

it('logs post processing warnings', async () => {
jest.spyOn(console, 'log').mockImplementation(() => undefined);

await bundle({
code: 'console.log(Math.random());',
});

expect(console.log).toHaveBeenCalledWith(
`Bundle Warning: Processing of the Snap bundle completed with warnings.\n${PostProcessWarning.UnsafeMathRandom}`,
);
});

it('generates a source map', async () => {
const result = await bundle({ browserifyOptions: { debug: true } });

Expand Down
8 changes: 8 additions & 0 deletions packages/snaps-browserify-plugin/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ export class SnapsBrowserifyTransform extends Transform {
inputSourceMap,
});

if (result.warnings.length > 0) {
console.log(
`Bundle Warning: Processing of the Snap bundle completed with warnings.\n${result.warnings.join(
'\n',
)}`,
);
}

postBundle(this.#options, result.code)
.catch((error) => {
callback(error);
Expand Down
18 changes: 17 additions & 1 deletion packages/snaps-rollup-plugin/src/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ import {
DEFAULT_SNAP_BUNDLE,
getSnapManifest,
} from '@metamask/snaps-utils/test-utils';
import { checkManifest, evalBundle } from '@metamask/snaps-utils';
import {
checkManifest,
evalBundle,
PostProcessWarning,
} from '@metamask/snaps-utils';
import snaps, { Options } from './plugin';

jest.mock('fs');
Expand Down Expand Up @@ -143,6 +147,18 @@ describe('snaps', () => {
`);
});

it('logs post processing warnings', async () => {
jest.spyOn(console, 'log').mockImplementation(() => undefined);

await bundle({
code: 'console.log(Math.random());',
});

expect(console.log).toHaveBeenCalledWith(
`Bundle Warning: Processing of the Snap bundle completed with warnings.\n${PostProcessWarning.UnsafeMathRandom}`,
);
});

it('generates a source map', async () => {
await fs.writeFile('/source-map.ts', DEFAULT_SNAP_BUNDLE);

Expand Down
8 changes: 8 additions & 0 deletions packages/snaps-rollup-plugin/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ export default function snaps(options?: Partial<Options>): Plugin {
sourceMap: true,
});

if (result.warnings.length > 0) {
this.warn(
`Bundle Warning: Processing of the Snap bundle completed with warnings.\n${result.warnings.join(
'\n',
)}`,
);
}

return { code: result.code, map: result.sourceMap };
},

Expand Down
30 changes: 29 additions & 1 deletion packages/snaps-utils/src/post-process.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { postProcessBundle } from './post-process';
import { postProcessBundle, PostProcessWarning } from './post-process';

describe('postProcessBundle', () => {
it('trims the string', () => {
Expand Down Expand Up @@ -58,6 +58,7 @@ describe('postProcessBundle', () => {
(1, eval)("<!" + "--" + " foo " + "--" + ">");
(1, foo.eval)("<!" + "--" + " bar " + "--" + ">");",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -84,6 +85,7 @@ describe('postProcessBundle', () => {
const bar = 'baz';
}",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand Down Expand Up @@ -112,6 +114,7 @@ describe('postProcessBundle', () => {
regeneratorRuntime.bar();
}",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -124,6 +127,7 @@ describe('postProcessBundle', () => {

var _marked = [a].map(regeneratorRuntime.mark);",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -138,6 +142,7 @@ describe('postProcessBundle', () => {
{
"code": "const foo = 'regeneratorRuntime';",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -152,6 +157,7 @@ describe('postProcessBundle', () => {
{
"code": "const foo = "<!" + "--" + " bar " + "--" + ">";",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -168,6 +174,7 @@ describe('postProcessBundle', () => {
"code": "const foo = "foo bar " + "import" + "()" + " baz";
const bar = "foo bar " + "import" + "(this works too)" + " baz";",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -182,6 +189,7 @@ describe('postProcessBundle', () => {
{
"code": "const foo = \`\${"<!"}\${"--"} bar \${"--"}\${">"} \${"<!" + "--" + " baz " + "--" + ">"} \${qux}\`;",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -203,6 +211,7 @@ describe('postProcessBundle', () => {
foo\`
foo \${"import"}\${"()"} \${"import" + "(bar)"} \${qux} \`;",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -217,6 +226,7 @@ describe('postProcessBundle', () => {
{
"code": "// < !-- foo -- >",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -233,6 +243,7 @@ describe('postProcessBundle', () => {
"code": "// Foo bar import\\() baz
// Foo bar import\\(baz) qux",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -247,6 +258,7 @@ describe('postProcessBundle', () => {
{
"code": "const foo = '';",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -265,6 +277,7 @@ describe('postProcessBundle', () => {
const foo = 'foo';
const bar = \`bar\${foo}\`;",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand All @@ -278,6 +291,7 @@ describe('postProcessBundle', () => {
{
"code": "const foo = \`\${"<!"}\${"--"} \\\` \${foo} \\\` \${"--"}\${">"}\`;",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand Down Expand Up @@ -320,6 +334,7 @@ describe('postProcessBundle', () => {
(1, foo.eval)('bar');
});",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand Down Expand Up @@ -362,6 +377,7 @@ describe('postProcessBundle', () => {
],
"version": 3,
},
"warnings": [],
}
`);
});
Expand All @@ -380,6 +396,7 @@ describe('postProcessBundle', () => {
"code": "const foo = 'bar';
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJmb28iXSwic291cmNlcyI6WyJ1bmtub3duIl0sInNvdXJjZXNDb250ZW50IjpbIlxuICAgICAgY29uc3QgZm9vID0gJ2Jhcic7XG4gICAgIl0sIm1hcHBpbmdzIjoiQUFDTSxNQUFNQSxHQUFHLEdBQUcsS0FBWiJ9",
"sourceMap": null,
"warnings": [],
}
`);
});
Expand Down Expand Up @@ -432,7 +449,18 @@ describe('postProcessBundle', () => {
],
"version": 3,
},
"warnings": [],
}
`);
});

it('returns a warning when using Math.random', () => {
const code = `
const foo = Math.random();
`;

const { warnings } = postProcessBundle(code);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toBe(PostProcessWarning.UnsafeMathRandom);
});
});
25 changes: 24 additions & 1 deletion packages/snaps-utils/src/post-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,18 @@ export type PostProcessOptions = {
* @property code - The modified code.
* @property sourceMap - The source map for the modified code, if the source map
* option was enabled.
* @property warnings - Any warnings that occurred during the post-processing.
*/
export type PostProcessedBundle = {
code: string;
sourceMap?: SourceMap | null;
warnings: PostProcessWarning[];
};

export enum PostProcessWarning {
UnsafeMathRandom = '`Math.random` was detected in the bundle. This is not a secure source of randomness.',
}

// The RegEx below consists of multiple groups joined by a boolean OR.
// Each part consists of two groups which capture a part of each string
// which needs to be split up, e.g., `<!--` is split into `<!` and `--`.
Expand Down Expand Up @@ -209,6 +215,8 @@ export function postProcessBundle(
inputSourceMap,
}: Partial<PostProcessOptions> = {},
): PostProcessedBundle {
const warnings = new Set<PostProcessWarning>();

const pre: PluginObj['pre'] = ({ ast }) => {
ast.comments?.forEach((comment) => {
// Break up tokens that could be parsed as HTML comment terminators. The
Expand Down Expand Up @@ -255,6 +263,17 @@ export function postProcessBundle(
}),
);
}

// Detect the use of `Math.random()` and add a warning.
if (
node.callee.type === 'MemberExpression' &&
node.callee.object.type === 'Identifier' &&
node.callee.object.name === 'Math' &&
node.callee.property.type === 'Identifier' &&
node.callee.property.name === 'random'
) {
warnings.add(PostProcessWarning.UnsafeMathRandom);
}
},

MemberExpression(path) {
Expand Down Expand Up @@ -437,7 +456,11 @@ export function postProcessBundle(
throw new Error('Bundled code is empty.');
}

return { code: file.code, sourceMap: file.map };
return {
code: file.code,
sourceMap: file.map,
warnings: Array.from(warnings),
};
} catch (error) {
throw new Error(`Failed to post process code:\n${error.message}`);
}
Expand Down
16 changes: 15 additions & 1 deletion packages/snaps-webpack-plugin/src/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
DEFAULT_SNAP_BUNDLE,
getSnapManifest,
} from '@metamask/snaps-utils/test-utils';
import { checkManifest, evalBundle } from '@metamask/snaps-utils';
import {
checkManifest,
evalBundle,
PostProcessWarning,
} from '@metamask/snaps-utils';
import SnapsWebpackPlugin, { Options } from './plugin';

jest.mock('@metamask/snaps-utils', () => ({
Expand Down Expand Up @@ -139,6 +143,16 @@ describe('SnapsWebpackPlugin', () => {
expect(code).not.toContain(`// Returns baz`);
});

it('logs post processing warnings', async () => {
const { stats } = await bundle({
code: 'console.log(Math.random());',
});

expect(stats.toJson().warnings?.[0].message).toMatch(
`SnapsWebpackPlugin: Bundle Warning: Processing of the Snap bundle completed with warnings.\n${PostProcessWarning.UnsafeMathRandom}`,
);
});

it('generates a source map', async () => {
const { fs } = await bundle({
webpackOptions: {
Expand Down
10 changes: 10 additions & 0 deletions packages/snaps-webpack-plugin/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ export default class SnapsWebpackPlugin {
inputSourceMap: devtool ? (asset.map() as SourceMap) : undefined,
});

if (processed.warnings.length > 0) {
compilation.warnings.push(
new WebpackError(
`${PLUGIN_NAME}: Bundle Warning: Processing of the Snap bundle completed with warnings.\n${processed.warnings.join(
'\n',
)}`,
),
);
}

const replacement = processed.sourceMap
? new SourceMapSource(
processed.code,
Expand Down