Document workspace file resolution logic

[jsejcksn]

Issue Type

• Other (Documentation)

I see there is resolution logic for this, but it is not yet documented.

Ref. source:

denox/src/parser/deno_workspace.ts

Lines 12 to 27 in 7774ec1

	async function loadDenoWorkspace(): Promise<DenoWorkspace> {
	try {
	const denoWorkspaceFilePath = await getFirstExistingPath(
	DENO_WORKSPACE_FILES,
	);
	if (extname(denoWorkspaceFilePath) === ".ts") {
	const appendFileScheme = Deno.build.os == "windows" ? 'file:///' : '';
	return await _loadTSWorkspace(appendFileScheme + denoWorkspaceFilePath);
	}
	return await _loadYAMLWorkspace(denoWorkspaceFilePath) as DenoWorkspace;
	} catch (e) {
	throw _handleLoadDenoWorkspaceErrors(e);
	}
	}

denox/src/const.ts

Lines 3 to 14 in 7774ec1

	const DENO_WORKSPACE_FILES = [
	"deno-workspace.yml",
	"deno-workspace",
	"deno-workspace.yaml",
	".deno-workspace",
	".deno-workspace.yml",
	".deno-workspace.yaml",
	"deno-workspace.json",
	".deno-workspace.json",
	"deno-workspace.ts",
	".deno-workspace.ts",
	];

I think it would be good to reconsider the resolution order based on some rules. This is how ESLint does it. Here is a suggested idea with included reasoning:

(Don't support extensionless deno-workspace because of ambiguity — but if you feel strongly about this, explain clearly which format(s) are valid without an extension, e.g. yaml/json)

1. First, files that begin with deno-workspace (not dotfiles because in the event of a conflict, the dotfile is potentially hidden and could be a surprise).

2. Next, in the following extension order, preferring more human-readable formats and static files, which offer better security at the cost of flexibility (more on this below):

   • .yaml
   • .yml
   • .json
   • (no extension would go here if it is not a script)
   • .ts
   • .js

3. Files that begin with .deno-workspace following the same extension order as above

So, the suggested order would be:

• deno-workspace.yaml
• deno-workspace.yml
• deno-workspace.json
• deno-workspace (if it is not a script)
• deno-workspace.ts
• deno-workspace.js
• .deno-workspace.yaml
• .deno-workspace.yml
• .deno-workspace.json
• .deno-workspace (if it is not a script)
• .deno-workspace.ts
• .deno-workspace.js

Security

It appears that when evaluating workspace script files, they are evaluated with the permissions inherited from denox, which is suggested to be --allow-all at installation. Is that correct? If so, that could be very dangerous.

Static configuration files are easier to reason about than dynamic scripts, so they are easier to secure.

[BentoumiTech]

@jsejcksn I like your suggested order.

I'll go further by actually removing the hidden file support as it's better if the workspace file is explicitly visible.

What solution do you think is possible to avoid possible issues when evaluating workspace script files?

[jsejcksn]

What solution do you think is possible to avoid possible issues when evaluating workspace script files?

I think it's not simple, and that all solutions involve trade-offs.

At the most security-oriented end of the spectrum: don't support script config files.
At the opposite end of the spectrum: make no changes.

All of the in-between relies on taking a position about what a config script should and shouldn't be allowed to do.

You could evaluate and serialize the export from the config in a sub-process (Deno.run), providing only the appropriate permissions. Determining what those permissions should be is the real question.

For example: providing --allow-run is the same as providing all permissions, so I think that should not be allowed.

Should a config script be able to write to the filesystem? I don't think config files should create filesystem side-effects, so --allow-write is also not appropriate, IMO.

Is reading files ok (--allow-read)? Probably. Should it be limited to the current directory and its children? Perhaps—I don't know.

Is accessing the network ok? Providing this permission would allow a malicious config to exfiltrate data. I can imagine how there might be a legitimate use case for this permission, but I have never seen an example of one.

What do you think? Take a look at the other permissions, too.

[BentoumiTech]

I actually like your idea of evaluating and exporting in a sub-process.

Regarding the permissions, I think allow-env and allow-read are a good starting point that would cover 90% of the use cases. I would rather add more permissions in the future if it makes sense than remove some.