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

refactor: ポート変更時にEngineInfoの情報を書き換えないようにする。 #2282

Merged
merged 4 commits into from
Oct 8, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/backend/browser/contract.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
import { EngineInfo, envEngineInfoSchema } from "@/type/preload";
import { type EngineInfo, envEngineInfoSchema } from "@/type/preload";

const baseEngineInfo = envEngineInfoSchema
.array()
.parse(JSON.parse(import.meta.env.VITE_DEFAULT_ENGINE_INFOS))[0];

export const defaultEngine: EngineInfo = {
...baseEngineInfo,
type: "path", // FIXME: ダミーで"path"にしているので、エンジンAPIのURLを設定できるようにし、type: "URL"にする
isDefault: true,
};
export const defaultEngine: EngineInfo = (() => {
const { protocol, hostname, port, pathname } = new URL(baseEngineInfo.host);
return {
...baseEngineInfo,
protocol,
hostname,
defaultPort: port,
pathname: pathname === "/" ? "" : pathname,
type: "path", // FIXME: ダミーで"path"にしているので、エンジンAPIのURLを設定できるようにし、type: "URL"にする
isDefault: true,
};
})();
export const directoryHandleStoreKey = "directoryHandle";
6 changes: 5 additions & 1 deletion src/backend/electron/engineAndVvppController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ export class EngineAndVvppController {
this.configManager.set("engineSettings", engineSettings);

await this.engineProcessManager.runEngineAll();
this.runtimeInfoManager.setEngineInfos(engineInfos);
this.runtimeInfoManager.setEngineInfos(
engineInfos,
this.engineInfoManager.altPortInfos,
);
await this.runtimeInfoManager.exportFile();
}

Expand All @@ -162,6 +165,7 @@ export class EngineAndVvppController {
// TODO: setからexportの処理は排他処理にしたほうがより良い
this.runtimeInfoManager.setEngineInfos(
this.engineInfoManager.fetchEngineInfos(),
this.engineInfoManager.altPortInfos,
);
await this.runtimeInfoManager.exportFile();
}
Expand Down
19 changes: 16 additions & 3 deletions src/backend/electron/manager/RuntimeInfoManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
import fs from "fs";
import AsyncLock from "async-lock";
import log from "electron-log/main";
import type { AltPortInfos } from "@/store/type";
import { EngineId, EngineInfo } from "@/type/preload";

/**
* ランタイム情報書き出しに必要なEngineInfo
*/
export type EngineInfoForRuntimeInfo = Pick<
EngineInfo,
"uuid" | "host" | "name"
"uuid" | "protocol" | "hostname" | "defaultPort" | "pathname" | "name"
>;

/**
Expand Down Expand Up @@ -58,12 +59,17 @@ export class RuntimeInfoManager {
* エンジン情報(書き出し用に記憶)
*/
private engineInfos: EngineInfoForRuntimeInfo[] = [];
private altportInfos: AltPortInfos = {};

/**
* エンジン情報を登録する
*/
public setEngineInfos(engineInfos: EngineInfoForRuntimeInfo[]) {
public setEngineInfos(
engineInfos: EngineInfoForRuntimeInfo[],
altportInfos: AltPortInfos,
) {
this.engineInfos = engineInfos;
this.altportInfos = altportInfos;
}

/**
Expand All @@ -76,9 +82,16 @@ export class RuntimeInfoManager {
formatVersion: this.fileFormatVersion,
appVersion: this.appVersion,
engineInfos: this.engineInfos.map((engineInfo) => {
const altPort: string | undefined =
this.altportInfos[engineInfo.uuid];
const port = altPort ?? engineInfo.defaultPort;
// NOTE: URLを正規化する
const url = new URL(`${engineInfo.protocol}//${engineInfo.hostname}`);
url.port = port;
return {
uuid: engineInfo.uuid,
url: engineInfo.host, // NOTE: 元のEngineInfo.hostにURLが入っている
// NOTE: URLインターフェースは"pathname"が空文字でも"/"を付けるので手動で結合する。
url: `${url.origin}${engineInfo.pathname}`,
name: engineInfo.name,
};
}),
Expand Down
26 changes: 10 additions & 16 deletions src/backend/electron/manager/engineInfoManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ function fetchDefaultEngineInfos(defaultEngineDir: string): EngineInfo[] {
const engines = envSchema.parse(JSON.parse(defaultEngineInfosEnv));

return engines.map((engineInfo) => {
const { protocol, hostname, port, pathname } = new URL(engineInfo.host);
return {
...engineInfo,
protocol,
hostname,
defaultPort: port,
pathname: pathname === "/" ? "" : pathname,
isDefault: true,
type: "path",
executionFilePath: path.resolve(engineInfo.executionFilePath),
Expand Down Expand Up @@ -85,7 +90,10 @@ export class EngineInfoManager {

engines.push({
uuid: manifest.uuid,
host: `http://127.0.0.1:${manifest.port}`,
protocol: "http:",
hostname: "127.0.0.1",
defaultPort: manifest.port.toString(),
pathname: "",
name: manifest.name,
path: engineDir,
executionEnabled: true,
Expand Down Expand Up @@ -140,15 +148,6 @@ export class EngineInfoManager {
...fetchDefaultEngineInfos(this.defaultEngineDir),
...this.fetchAdditionalEngineInfos(),
];
// 代替ポートに置き換える
engineInfos.forEach((engineInfo) => {
const altPortInfo = this.altPortInfos[engineInfo.uuid];
if (altPortInfo) {
const url = new URL(engineInfo.host);
url.port = altPortInfo.to.toString();
engineInfo.host = url.origin;
}
});
return engineInfos;
}

Expand Down Expand Up @@ -191,12 +190,7 @@ export class EngineInfoManager {
* エンジン起動時にポートが競合して代替ポートを使う場合に使用する。
*/
updateAltPort(engineId: EngineId, port: number) {
const engineInfo = this.fetchEngineInfo(engineId);
const url = new URL(engineInfo.host);
this.altPortInfos[engineId] = {
from: Number(url.port),
to: port,
};
this.altPortInfos[engineId] = port.toString();
}

/**
Expand Down
10 changes: 7 additions & 3 deletions src/backend/electron/manager/engineProcessManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import {
findAltPort,
getPidFromPort,
getProcessNameFromPid,
type HostInfo,
isAssignablePort,
url2HostInfo,
} from "../portHelper";

import { EngineInfo, EngineId, EngineSettings } from "@/type/preload";
Expand Down Expand Up @@ -86,7 +86,11 @@ export class EngineProcessManager {
}

// { hostname (localhost), port (50021) } <- url (http://localhost:50021)
const engineHostInfo = url2HostInfo(new URL(engineInfo.host));
const engineHostInfo: HostInfo = {
protocol: engineInfo.protocol,
hostname: engineInfo.hostname,
port: Number(engineInfo.defaultPort),
Copy link
Member

Choose a reason for hiding this comment

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

HostInfo.port、Numberじゃなくてstringでも良さそうな気がしますね!

まあとはいえEngineManifestのportがnumberなので全部stringにはできないですね。
まあどこからstringにするかというだけですが、このエディタ上では大体stringで統一しておくと良さそうかも?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

portHelper周りがNumberで処理しているのでここがNumberとStringを切り替える部分だと思いました。

メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。

Copy link
Member

@Hiroshiba Hiroshiba Oct 7, 2024

Choose a reason for hiding this comment

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

なるほどです!!

portHelper周りもstringで良い気もしました!
けどまあ、既存コードになじませる意図だとNumberのが良さそうなので、このプルリクエストはこの形が良さそうだなと感じました!!
ということでresolve!

メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。

現状、engineManifestの読み込みは型のバリデーションも含むzodで行っているので、必ず数値しか入らないと思われます。


ちょっとこのプルリクとは外れますが、危ないのは事実ですね・・・。
一番危ないのはたぶんコマンド実行している部分execFileSyncがshell: trueで動いてることだと思います。hostnameとかでいたずらされるかもしれない。
shell: falseにすれば問題ないはず・・・。

};

// ポートが塞がっていれば代替ポートを探す
let port = engineHostInfo.port;
Expand Down Expand Up @@ -152,7 +156,7 @@ export class EngineProcessManager {
const enginePath = engineInfo.executionFilePath;
const args = engineInfo.executionArgs.concat(useGpu ? ["--use_gpu"] : [], [
"--host",
new URL(engineInfo.host).hostname,
engineHostInfo.hostname,
"--port",
port.toString(),
]);
Expand Down
8 changes: 0 additions & 8 deletions src/backend/electron/portHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,6 @@ const portWarn = (port: number, message: string, isNested = false) => {
log.warn(`${isNested ? "| " : ""}PORT ${port}: ${message}`);
};

export function url2HostInfo(url: URL): HostInfo {
return {
protocol: url.protocol,
hostname: url.hostname,
port: Number(url.port),
};
}

/**
* "netstat -ano" の stdout から, 指定したポートを LISTENING しているプロセスの id を取得します.
*
Expand Down
4 changes: 2 additions & 2 deletions src/components/Menu/MenuBar/MenuBar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const currentVersion = ref("");
const audioKeys = computed(() => store.state.audioKeys);

// デフォルトエンジンの代替先ポート
const defaultEngineAltPortTo = computed<number | undefined>(() => {
const defaultEngineAltPortTo = computed<string | undefined>(() => {
const altPortInfos = store.state.altPortInfos;

// ref: https://github.com/VOICEVOX/voicevox/blob/32940eab36f4f729dd0390dca98f18656240d60d/src/views/EditorHome.vue#L522-L528
Expand All @@ -65,7 +65,7 @@ const defaultEngineAltPortTo = computed<number | undefined>(() => {

// <defaultEngineId>: { from: number, to: number } -> to (代替先ポート)
if (defaultEngineInfo.uuid in altPortInfos) {
return altPortInfos[defaultEngineInfo.uuid].to;
return altPortInfos[defaultEngineInfo.uuid];
} else {
return undefined;
}
Expand Down
3 changes: 2 additions & 1 deletion src/components/Talk/TalkEditor.vue
Original file line number Diff line number Diff line change
Expand Up @@ -554,11 +554,12 @@ watch(
// 代替ポートをトースト通知する
for (const engineId of store.state.engineIds) {
const engineName = store.state.engineInfos[engineId].name;
const defaultPort = store.state.engineInfos[engineId].defaultPort;
const altPort = store.state.altPortInfos[engineId];
if (!altPort) return;

void store.actions.SHOW_NOTIFY_AND_NOT_SHOW_AGAIN_BUTTON({
message: `${altPort.from}番ポートが使用中であるため ${engineName} は、${altPort.to}番ポートで起動しました`,
message: `${defaultPort}番ポートが使用中であるため ${engineName} は、${altPort}番ポートで起動しました`,
icon: "compare_arrows",
tipName: "engineStartedOnAltPort",
});
Expand Down
10 changes: 9 additions & 1 deletion src/store/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,15 @@ const proxyStoreCreator = (_engineFactory: IEngineConnectorFactory) => {
new Error(`No such engineInfo registered: engineId == ${engineId}`),
);

const instance = _engineFactory.instance(engineInfo.host);
const altPort: string | undefined = state.altPortInfos[engineId];
const port = altPort ?? engineInfo.defaultPort;
// NOTE: URLを正規化する
const url = new URL(`${engineInfo.protocol}//${engineInfo.hostname}`);
url.port = port;
// NOTE: URLインターフェースは"pathname"が空文字でも"/"を付けるので手動で結合する。
const instance = _engineFactory.instance(
`${url.origin}${engineInfo.pathname}`,
);
return Promise.resolve({
invoke: (v) => (arg) =>
// FIXME: anyを使わないようにする
Expand Down
2 changes: 1 addition & 1 deletion src/store/type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export type Command = {
export type EngineState = "STARTING" | "FAILED_STARTING" | "ERROR" | "READY";

// ポートが塞がれていたときの代替ポート情報
export type AltPortInfos = Record<EngineId, { from: number; to: number }>;
export type AltPortInfos = Record<EngineId, string>;
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

仮にdefaultPortがundefinableになっても、ここは必ずstringになりそう!


export type SaveResult =
| "SUCCESS"
Expand Down
5 changes: 4 additions & 1 deletion src/type/preload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,10 @@ export type MinimumEngineManifestType = z.infer<

export type EngineInfo = {
uuid: EngineId;
host: string; // NOTE: 実際はorigin(プロトコルとhostnameとport)が入る
protocol: string; // `http:`など
hostname: string; // `example.com`など
defaultPort: string; // `50021`など。空文字列もありえる。
pathname: string; // `/engine`など。空文字列もありえる。
name: string;
path?: string; // エンジンディレクトリのパス
executionEnabled: boolean;
Expand Down
46 changes: 34 additions & 12 deletions tests/unit/backend/electron/RuntimeInfo.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,35 @@ test("想定通りのラインタイム情報が保存されている", async ()
const runtimeInfoManager = new RuntimeInfoManager(tempFilePath, appVersion);

// エンジン情報
runtimeInfoManager.setEngineInfos([
{
uuid: EngineId("00000000-0000-0000-0000-000000000001"),
host: "https://example.com/engine1",
name: "engine1",
},
{
uuid: EngineId("00000000-0000-0000-0000-000000000002"),
host: "https://example.com/engine2",
name: "engine2",
},
]);
runtimeInfoManager.setEngineInfos(
[
{
uuid: EngineId("00000000-0000-0000-0000-000000000001"),
protocol: "https:",
hostname: "example.com",
defaultPort: "",
pathname: "/engine1",
name: "engine1",
},
{
uuid: EngineId("00000000-0000-0000-0000-000000000002"),
protocol: "https:",
hostname: "example.com",
defaultPort: "",
pathname: "/engine2",
name: "engine2",
},
{
uuid: EngineId("00000000-0000-0000-0000-000000000003"),
protocol: "http:",
hostname: "127.0.0.1",
defaultPort: "8080",
pathname: "",
name: "engine3",
},
],
{ [EngineId("00000000-0000-0000-0000-000000000003")]: "8081" },
);

// ファイル書き出し
await runtimeInfoManager.exportFile();
Expand All @@ -49,6 +66,11 @@ test("想定通りのラインタイム情報が保存されている", async ()
"url": "https://example.com/engine2",
"uuid": "00000000-0000-0000-0000-000000000002",
},
{
"name": "engine3",
"url": "http://127.0.0.1:8081",
"uuid": "00000000-0000-0000-0000-000000000003",
},
],
"formatVersion": 1,
}
Expand Down
Loading