Skip to content

mount import directory for pyodide sandbox #2073

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.

Sign up for GitHub

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

Merged
paulfitz merged 4 commits into from
Jan 30, 2026
Merged

mount import directory for pyodide sandbox #2073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
919d96b
mount import directory for pyodide sandbox
paulfitz Jan 27, 2026
49ccaef
fix lint
paulfitz Jan 27, 2026
d82c935
run two pyodide imports
paulfitz Jan 27, 2026
f55449c
expand on a comment
paulfitz Jan 30, 2026
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
1 change: 1 addition & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ jobs:
make setup
cd ../..
yarn run test:server -g 'ActiveDoc.useQuerySet|Sandbox'
yarn run test:nbrowser -g 'Importer.*should.show.correct.preview'
env:
MOCHA_WEBDRIVER_HEADLESS: 1
GRIST_SANDBOX_FLAVOR: pyodide
Expand Down
2 changes: 1 addition & 1 deletion app/server/lib/SandboxPyodide.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export function getPyodideSettings(options: ISandboxOptions): PyodideSettings {
);

// If the sandbox is being used to do an import, we'll need read access
// to that too. We drop read access before running user code.
// to that too. We will not drop read access to this.
const importDir = options.importDir ? fs.realpathSync(path.resolve(options.importDir)) : undefined;
const importAllow = importDir ? [`--allow-read=${importDir}`] : [];

Expand Down
29 changes: 24 additions & 5 deletions sandbox/pyodide/pipe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,16 @@ class GristPipe {
async mountImportDirIfNeeded() {
if (process.env.IMPORTDIR) {
this.log("Setting up import from", process.env.IMPORTDIR);
// Could mount directly, but instead copy so we can drop read perms early.
await this.copyFiles(process.env.IMPORTDIR, "/import_src", "/import");
// All imports for a given doc live in the same root directory.
Copy link
Copy Markdown
Collaborator

@fflorent fflorent Jan 28, 2026

Choose a reason for hiding this comment

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

Alright, IMPORTDIR is a subdirectory to a temporary directory attributed exclusively to the ActiveDoc?

Also after adding some logs, I found that a sandbox is spawned during the import, am I right? It is fine here to share the same import directory because it is scoped to the ActiveDoc? (in which case it would make sense to me too)

Copy link
Copy Markdown
Member Author

@paulfitz paulfitz Jan 28, 2026

Choose a reason for hiding this comment

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

Yes that is what @Spoffy found when working on a Grist Desktop issue.

Copy link
Copy Markdown
Contributor

@Spoffy Spoffy Jan 30, 2026

Choose a reason for hiding this comment

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

That was my understanding / findings on how this all worked, yeah. The logic connecting parseFile in DocPluginManager to the sandbox is convoluted, but I'll do my best to summarise:

  • File parsers (e.g. CSV, JSON, Excel) are defined as plugins in plugins/core/manifest.yml.
  • These are instantiated as PluginInstance objects in DocPluginManager (which is created on a per-doc basis) when DocPluginManager is created. DocPluginManager is created one-per-doc.
  • When DocPluginManager initialises, it creates a new temporary directory (on a per-doc basis) that's passed to each plugin instance.
  • When a plugin instance is first used (more specifically - when it first tries to make an RPC call to the sandbox, e.g. to parse a file), it's activated, which spawns a new sandbox
  • Plugin instances are shut down when the document is, or after 5 minutes of inactivity. This shuts down that plugin's sandbox.

In short: yes, the temp directory is per active document. 🙂

Copy link
Copy Markdown
Contributor

@Spoffy Spoffy Jan 30, 2026

Choose a reason for hiding this comment

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

Is it worth adding to this comment, something like:

Copying import files in and dropping read permissions isn't possible, due to the same sandbox being re-used for subsequent imports. The files would only copied once on startup, meaning the files for these later imports won't be available to Pyodide, resulting in errors.

Copy link
Copy Markdown
Member Author

@paulfitz paulfitz Jan 30, 2026

Choose a reason for hiding this comment

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

Added.

// Copying import files in and dropping read permissions isn't
// workable in that case, since the same sandbox is re-used for
// subsequent imports. The files would only be copied once on
// startup, meaning the files for these later imports won't be
// available to Pyodide, resulting in errors.
await this.pyodide.FS.mkdir("/import");
await this.pyodide.FS.mount(this.pyodide.FS.filesystems.NODEFS, {
root: process.env.IMPORTDIR,
}, "/import");
}
}

Expand Down Expand Up @@ -146,11 +154,22 @@ async function main() {

if (isDeno) {
// Revoke write permissions now that packages are loaded.
// Revoke read access as well, why not.
// eslint-disable-next-line no-undef
await Deno.permissions.revoke({ name: "write" });
// eslint-disable-next-line no-undef
await Deno.permissions.revoke({ name: "read" });

// Read access has been limited quite a lot already.
// We need to keep access to the import directory, but can shed
// everything else. See --allow-read in SandboxPyodide.ts
const readDir = fs.realpathSync(__dirname);
const gristDir = fs.realpathSync(path.join(__dirname, "..", "grist"));
const reqFile = fs.realpathSync(path.join(__dirname, "..", "requirements.txt"));
Copy link
Copy Markdown
Collaborator

@fflorent fflorent Jan 28, 2026

Choose a reason for hiding this comment

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

For my own understanding, why is --allow-read=/PATH/TO/sandbox/requirements.txt added in a first place in SandboxPyodide.ts?

I think I see the reason for adding --allow-read=/PATH/TO/sandbox/grist: it is used to make a copy in loadCode. Deno.permissions.revoke() allows here to revoke permissions previously granted by the --allow-read arguments, right?

(Thanks for the comment BTW, it is very helpful to understand a bit more the context!)

Copy link
Copy Markdown
Member Author

@paulfitz paulfitz Jan 28, 2026

Choose a reason for hiding this comment

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

For requirements.txt, because that is how the code figures out the libraries Grist needs

grist-core/sandbox/pyodide/packages.js

Line 5 in 122d2fe

const txt = fs.readFileSync(path.join(__dirname, "..", "requirements.txt"), "utf8");

This could be done a different way, but is what is there right now.

Copy link
Copy Markdown
Member Author

@paulfitz paulfitz Jan 28, 2026

Choose a reason for hiding this comment

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

Correct about the rest.

for (const dir of [readDir, gristDir, reqFile]) {
// eslint-disable-next-line no-undef
await Deno.permissions.revoke({
name: "read",
path: dir
});
}
console.error("[pyodide sandbox]", "revoked read and write permissions.");
}

Expand Down
2 changes: 1 addition & 1 deletion test/nbrowser/Importer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ describe("Importer", function() {
"Africa", "Eastern Africa", "Burundi",
"Africa", "Eastern Africa", "Comoros"]);
await driver.get(`${docUrl}`);
await gu.acceptAlert();
await gu.acceptAlert({ ignore: true });
await gu.waitForDocToLoad();
});

Expand Down