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

trusted フラグの追加 #249

Merged
merged 7 commits into from
May 14, 2021
Merged

trusted フラグの追加 #249

merged 7 commits into from
May 14, 2021

Conversation

yu-ogi
Copy link
Contributor

@yu-ogi yu-ogi commented May 11, 2021

このPullRequestが解決する内容

  • RunnerManager#createRunner() のパラメータに trusted フラグを追加します。
    • このフラグが真の場合、対象のゲームコンテンツを「信頼されたもの」として実行します。信頼されたコンテンツであれば、エンジンモジュールから外部モジュールまたは組み込みモジュールへのアクセスが許可されます。それにより将来的には headless-driver の描画内容を出力する機能の提供などが期待できます。
  • vm2 周りの実装を一部リファクタリングします。
    • loadFile() の処理を headless-driver 側に移動します。
    • Asset 側までファイルロード関数 (loadFileHandler()) を渡すようにします。

破壊的変更

  • 部分的に あり
    • ただし headless-driver を利用する分には影響がありません。

@@ -1,4 +1,3 @@
export * from "./Runner";
export * from "./Platform";
export * from "./Looper";
export * from "./utilsInSandbox";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

headless-driver 側で定義しているグローバルオブジェクトの型定義を headless-driver-runner 側で定義しているのがどうにも奇妙で部分的な変更が困難だったため、 headless-driver 側からファイルロード関数を渡すように修正してしまいました。

@@ -34,7 +34,7 @@ beforeAll(() => {
jest.spyOn(ExecuteVmScriptV3, "getFilePath").mockReturnValue(path.resolve(__dirname, "../../lib/", "ExecuteVmScriptV3.js"));
});

describe("ホスティングされたコンテンツの動作テスト", () => {
describe("untrusted コンテンツの動作テスト (URL)", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

このあたりは文言修正のみです。

describe("trusted コンテンツの動作テスト: 異常系", () => {
// テストが上手く行かない (`NodeVM` 上で `process.exit()` が実行できてしまう) ため一旦無効に。
// TODO: テストが動作するように修正
xit("Akashic V1 のコンテンツで process が参照できないことを確認", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

本来 NodeVM 上で process.exit() を実行してもエラーとなる (process is not defined となる) はずなのですが、どうにも process.exit() が実行できてしまうようでした。
ミニマムのサンプルを作ってみても再現できず原因が全くわからないため、やむを得ず TODO コメントでテストコードのみ残しています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

調査してみたところ、どうやら context: host とすると NodeVM の require 先では process.exit() が有効となることがわかりました。これは仕様上の制限かもしれない (外部モジュールまで sandbox の制限をすると正常な動作が期待できないなどの理由) ため、そうであればこのテスト自体は不要となるかもしれません。

// module1.js
module.exports = () => {
	require("./module2")();
}
// module2.js
module.exports = () => {
	console.log('typeof process.exit', typeof process.exit);
}
// index.js
const { NodeVM } = require('vm2');

const vm1 = new NodeVM({
	require: {
		context: 'sandbox',
		external: true,
	}
});
const vm2 = new NodeVM({
	require: {
		context: 'host',
		external: true,
	}
});

// sandbox-0
vm1.run(`console.log('typeof process.exit', typeof process.exit)`);
// sandbox-1
vm1.run(`require('./module1')();`, 'index.js');
// sandbox-2
vm1.run(`require('./module2')();`, 'index.js');

// host-0
vm2.run(`console.log('typeof process.exit', typeof process.exit)`);
// host-1
vm2.run(`require('./module1')();`, 'index.js');
// host-2
vm2.run(`require('./module2')();`, 'index.js');
$ node index.js
typeof process.exit undefined
typeof process.exit undefined
typeof process.exit undefined
typeof process.exit undefined
typeof process.exit function
typeof process.exit function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/patriksimek/vm2#nodevm

require.context - host (default) to require modules in host and proxy them to sandbox. sandbox to load, compile and require modules in sandbox. Builtin modules except events always required in host and proxied to sandbox.

runner.stop();
});

it("Akashic V1 コンテンツで allowedUrls に指定していない外部アセットは読み込めないことを確認", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

 trusted でも allowUrls が有効であることを確認します。

3: ExecVmScriptV3.getFilePath()
const nvm = this.createVm(params.trusted);

const runnerParameter: RunnerParameters = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

バージョンごとに runnerParameters が異なることを想定して別々でパラメータを定義していましたが、型定義自体は RunnerParameters であることには変わりないため共通の変数でまとめてしまいました。

@yu-ogi yu-ogi added the enhancement New feature or request label May 11, 2021
@yu-ogi yu-ogi requested review from ShinobuTakahashi and xnv May 11, 2021 04:57
})
.catch((e) => loader._onAssetError(this, e));
this.loadFileHandler(this.path, (err, text) => {
// NOTE: v1 のため型については精査しない
Copy link
Contributor

Choose a reason for hiding this comment

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

コメントが v1 となっています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

おお、ありがとうございます。ややこしいので 過去バージョンのため〜〜 に統一してしまいました。

@@ -327,37 +298,46 @@ export class RunnerManager {
}

protected async resolveContent(contentUrl: string): Promise<EngineConfiguration> {
Copy link
Contributor

Choose a reason for hiding this comment

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

ここと resolveGameConfiguration(), loadJSON()async も消せそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここは単に不要な return await を嫌っただけなので他の箇所については修正する必要は無いかなと。

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、理解しました。ありがとうございます。

Copy link
Member

@xnv xnv left a comment

Choose a reason for hiding this comment

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

コメント加えましたが approved.

* asset へのアクセスを許可する URL。
* この URL と一致もしくは先頭一致しない asset へのアクセスはエラーとなる。
* null が指定された場合は全てのアクセスを許可する。
* 信頼されたコンテンツであるかどうか。
Copy link
Member

Choose a reason for hiding this comment

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

true の場合と false の場合の違いについてコメントしておきたいです。(特に true の場合に何が可能になるのか、どういう時に使用すべきでないか)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらコメント追加しました。

@yu-ogi
Copy link
Contributor Author

yu-ogi commented May 14, 2021

#250 をマージ後、このブランチを master へマージしようと思います。

prepare-1.7.0 ブランチを master から派生してそちらにマージしようと思います。

@yu-ogi yu-ogi changed the base branch from master to prepare-1.7.0 May 14, 2021 03:40
@yu-ogi yu-ogi merged commit b4f9bf5 into prepare-1.7.0 May 14, 2021
@yu-ogi yu-ogi deleted the support-trusted-mode branch May 14, 2021 03:42
@yu-ogi yu-ogi mentioned this pull request May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants