allow fetching file urls when experimental permissions are enabled

[KhafraDev]

this enables fetching file urls when permissions are enabled and we have fs.read access to the file.

[mcollina]

lgtm

[KhafraDev]

I think there are a few rough edges to fix still, but this is fine currently.

• Should the response have headers? If so, which headers? I thought maybe content-type where we do a best-guess based on file extension, but I wasn't sure.
• I did not add an experimental warning because it's landing in undici v7 AND there's a warning given for --experimental-permission already. Two warnings for one feature felt unnecessary.
• fs.openAsBlob and permissions are both experimental.
• This requires permissions to use, but permissions are quite rigid (as they should, but it may lead to unexpected behaviors). The easy workaround is having someone use --experimental-permission --allow-fs-read=* --allow-every-other-flag... which is quite tedious given the number of permissions.
• Giving access to fetching file URLs given --allow-fs-read might be unwanted behavior.

Fetching file URLs is heavily requested and leads to things that work in the browser, Deno, and Bun, but not node (fetch(new URL('myfile.txt', import.meta.url'))). Firefox is the only browser that implements some form of fetching file URLs, but I don't know to what extent.

What I am getting at: this is practically useless in its current state. Should I remove the requirement on the permission system? Or, should we use permissions if they're available, and allow fetching any file URL if they aren't?

[mcollina]

What I am getting at: this is practically useless in its current state.

Developers put unsanitized URLs in fetch() all the time. This is security vs DX, and this time I don't think we can support DX.

Should I remove the requirement on the permission system?

We could if we added another option like enableFileURL: true. Basically protecting the users from the problem above.

I think this would be the best option. If so, print an experimental warning.

[KhafraDev]

My biggest issue is definitely the fourth point: having to explicitly enable every permission if you use any permissions. As in, to allow fetching file URLs, without changing your app's functionality, you need to do:

node --experimental-permission --allow-fs-write=* --allow-fs-read=... --allow-child-process --allow-worker --allow-wasi --allow-addons index.js

Then of course you might limit reading files, so I assume most people will --allow-fs-read=* anyways...

[KhafraDev]

Adding an option seems like a false sense of security to me. If any dependency/sub-dependency uses the option, the same vulnerabilities will exist as if we disregarded permissions. When the duplex option became required to send ReadableStream bodies, many apps/libraries ended up adding the option on every single request... I find it likely that'll end up becoming the case if we add in another option.

[mcollina]

Note that if they only limit reading the current folder, or just a few files in it, this will be safe to use.

[RafaelGSS]

I think we don't need to enforce permission model here. It should be up to the user.

My suggestion is:

• Allow user to opt-in to the fetch file URLs through a config option (as Matteo suggested)
• If permission model is enabled we check if the user has access to the file request
  • If don't, a custom message is exhibited (the current state) - Access to ${fileURL.href} is not permitted

I don't think a library should change its behavior based on permission model state. It should be transparent to users.

[mcollina]

@KhafraDev are you ok if we land this in v8?

[RafaelGSS]

permission is no longer experimental

[codecov-commenter]

Codecov Report

❌ Patch coverage is 39.28571% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.86%. Comparing base (2885361) to head (9a0e6ba).

Files with missing lines	Patch %	Lines
lib/web/fetch/index.js	39.28%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3775      +/-   ##
==========================================
- Coverage   92.90%   92.86%   -0.04%     
==========================================
  Files         112      112              
  Lines       35638    35651      +13     
==========================================
  Hits        33109    33109              
- Misses       2529     2542      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[KhafraDev]

Sorry for the late response, I am going to close this in favor of #4907