lino-levan / astral

A high-level puppeteer/playwright-like library for Deno
https://jsr.io/@astral/astral
MIT License
215 stars 10 forks source link

feat: support sandbox mode #34

Closed lowlighter closed 11 months ago

lowlighter commented 11 months ago

Towards #33

This adds a sandbox parameters to browser.newPage():

Unfortunately because of https://github.com/denoland/deno/issues/14668, I had to add a sandboxPrompt arg to toggle interactivity, since the current Deno.permissions.request will always ask for permissions even when passing --no-prompt or with DENO_NO_PROMPT=1

From the unit test I added, it seems pretty robuts, as you even if you try to use page.evaluate to redirect or fetch with JS, the network request will be intercepted (provided you pass the correct resource type in sandbox)

Small demo: ezgif-1-3ad9a18719

Side notes:

lino-levan commented 11 months ago

Unfortunately because of https://github.com/denoland/deno/issues/14668, I had to add a sandboxPrompt arg to toggle interactivity, since the current Deno.permissions.request will always ask for permissions even when passing --no-prompt or with DENO_NO_PROMPT=1

I'll just fix upstream, no need to add yet another option. Not a super hard patch.

lowlighter commented 11 months ago

Thanks for the quick review and the feedback ! I may follow this pr a bit sparsely since I'm doing other stuff in parallel but I'll try to be responsive on it 😄

lino-levan commented 11 months ago

Thank you for your work! Take your time.

lino-levan commented 11 months ago

Any ideas why tests are failing?

lowlighter commented 11 months ago

Any ideas why tests are failing?

For some reason this test does not work on windows (is ok on linux/mac os):

Deno.test.ignore("Sandbox fulfill granted read permissions", {
  permissions: {
    ...permissions,
    read: [...permissions.read, fromFileUrl(import.meta.url)],
    net: ["127.0.0.1"],
  },
}, async () => {
  const browser = await launch();
  const page = await browser.newPage(import.meta.url, { sandbox: true });
  assertStrictEquals(await page.evaluate(status), 200);
  await browser.close();
});

I'll try to debug this when I have some time. I wonder if it's because the volume name on windows make the url weird or something similar.

Btw, I think the error code 21 is actually the "lock file" of windows (since windows can create lock directly on the files). I think that's why if one of the tests fail on windows, the whole suite will start to behave weirdly and fail too, because browser.close() is called after the assertions so if one of them throw the process is actually never closed properly and it's not possible to relaunch an instance


Edit: It was because files on windows looks like C:\path\to\file but were converted to file:///C:/path/to/file with pathname equal to /C:/path/to/file so the requested path weren't the same

lino-levan commented 11 months ago

Btw, I think the error code 21 is actually the "lock file" of windows

I think that makes a lot of sense actually. Could definitely be the case.

lowlighter commented 11 months ago

Any changes you want to make before this gets merged? This LGTM. Great work!

Apart from changing Deno.permissions.query to Deno.permissions.request once it's fixed upstream not really.

For now it means that enabling sandbox will not prompt for additional permissions so users shouldn't be surprised if their requests fail if they didn't grant enough permissions

lino-levan commented 11 months ago

I think this is a reasonable limitation for now. We can change it to prompt when I patch upstream. Thanks for your work!