cmorten / opine

Minimalist web framework for Deno ported from ExpressJS.
https://github.com/cmorten/opine/blob/main/.github/API/api.md
MIT License
854 stars 43 forks source link

Network access is required to run? #96

Closed mocoso closed 3 years ago

mocoso commented 3 years ago

Issue

Setup:

Please replace this line with a short description of the issue.

Details

Since v0.24.0 Opine seems to required network access to run.

Steps to reproduce

Step 1: Create an app.ts file with the following contents

import { opine } from "https://deno.land/x/opine@1.0.0/mod.ts";

const app = opine();

app.get("/", function(req, res) {
  res.send("Hello World");
});

app.listen(3000);

Step 2: Run deno cache app.ts to cache the dependencies

Step 3: Run `deno run app.ts' and get the following error

error: Uncaught (in promise) PermissionDenied: network access to "https://deno.land/x/denoflate@1.1/pkg/denoflate_bg.wasm", run again with the --allow-net flag
    at processResponse (deno:core/core.js:223:11)
    at Object.jsonOpAsync (deno:core/core.js:240:12)
    at async fetch (deno:op_crates/fetch/26_fetch.js:1278:29)
    at async read (mod.ts:19:15)
    at async init (https://deno.land/x/denoflate@1.1/pkg/denoflate.js:217:45)
    at async mod.ts:23:1

I would have expected this to run without an error.

abbasogaji commented 3 years ago

the option flag, "--allow-net --allow-read" is required to run any deno application (or library) that requires network access, because by default there is No file, network, or environment access, unless explicitly enabled.

as it was stated on https://deno.land/

@mocoso can try running it with the flag

   deno run --allow-net --allow-read app.ts
cmorten commented 3 years ago

Thanks for raising this @mocoso!

You’re quite right, in 0.24.0 I introduced the denoflate module to handle gzip and deflate. (REF: https://github.com/asos-craigmorten/opine/blob/main/.github/CHANGELOG.md#0240---17-10-2020)

This particular module uses wasm and curiously this is requested dynamically during runtime rather than an install... hence why when you run the application, it requests network access to download this wasm file 😅

I’ve prioritised features over experience til now, but it would be good to start closing in on these security / developer experience aspects as well 😊. If your server doesn’t need net access then the library shouldn’t force you to!

To begin I’m curious if this happens on the run of a hello world Opine example, or only with servers making use of the body parsers given it is in these that the gzip/deflate code lies?

As a starter for 10, I wonder if specifying the wasm import in the deps.ts would suffice to cache the wasm at installation so it is not refetched at runtime, but I am doubtful. In which case the follow-up would be to follow up with the library author, or find an appropriate module substitute!

For now you can limit the security impact of this by specifying deno.land in the —allow-net allowlist.

Open to suggestions/ PRs! 🚀

cmorten commented 3 years ago

As alluded in the comment before mine, I’m fairly certain you can’t run an http server in Deno without allowing network access for the local IP and port you are listening on, e.g. --allow-net 0.0.0.0:3000 so can’t see dropping the flag completely ever being an option? (Except in the cases where your uses the middleware capabilities of this lib, but using alternative code for starting up the server)

mocoso commented 3 years ago

Thanks for your thoughtful responses

To begin I’m curious if this happens on the run of a hello world Opine example, or only with servers making use of the body parsers given it is in these that the gzip/deflate code lies?

It happens with the hello world example above and so affects all opine servers

For now you can limit the security impact of this by specifying deno.land in the —allow-net allowlist.

From a security perspective this seems like a reasonable work around.

I’m fairly certain you can’t run an http server in Deno without allowing network access for the local IP and port you are listening on, e.g. --allow-net 0.0.0.0:3000 so can’t see dropping the flag completely ever being an option?

There are two non-security reasons that make it useful to run code that imports Opine without it requiring network access (which was possible before https://github.com/asos-craigmorten/opine/pull/81)

  1. For unit testing. ideally unit tests can be run without requiring access to the internet because requiring access to the internet slows the running of tests down and makes them less stable. The same issue is encountered running deno test on a project which imports Opine as is outlined here with deno run.

  2. For packaging an app to deploy. I deploy my app as a container and would ideally like that container to already include all the code it requires to run (to make start up faster). Previously deno cache was sufficient to achieve this but now it is not.

With this in mind, perhaps, my initial naming of this issue was unhelpful.

As a starter for 10, I wonder if specifying the wasm import in the deps.ts would suffice to cache the wasm at installation so it is not refetched at runtime, but I am doubtful. In which case the follow-up would be to follow up with the library author, or find an appropriate module substitute!

I am not quite sure if/how this can be achieved but I will investigate

mocoso commented 3 years ago

Asking about this on Discord got the following response

wasm imports can solve it (https://github.com/denoland/deno/issues/5609) but not here yet. for my wasm based module, i just download file locally and load local wasm if its present download it otherwise

cmorten commented 3 years ago

Nice sleuthing @mocoso!

I think there is sufficient use-case to keep this open, just not sure of the best way forward... we could even store the wasm in Opine for now but I don’t think that would solve the issue as the runtime request would just be passed onto the opine module rather than within denoflate.

The issue feels to be around the use of fetch within the module to grab the wasm following the lack of import support, if this were to be instead wrapped in “cache-fetch” like method then we’d be sorted... complications arising with where and how this is cached. The Deno directory would make most sense, but to know where that is require env var access and it opens up a whole other can of worms for doing this well - feels almost like a library in itself! All to solve an issue which really Deno should handle through wasm imports, which maybe will drop soon?

Not sure what to suggest for now... we could try and bypass the whole issue through feature switching gzip and deflate so this usage can be switched off (though doesn’t really solve the issue, for your tests it would simply replace a flag with a config setting and you may still want compression support!), or we can explore non-wasm libraries (or write one if doesn’t exist)?

cmorten commented 3 years ago

For some prior art, the deno-canvas authors have opted to download and cache the wasm file relatively. REF: https://github.com/DjDeveloperr/deno-canvas/blob/master/lib.js#L4

An update to denoflate’s entrypoint could handle this in the same fashion, though feels a little clunky for wasm files to be popping up in peoples codebases (I guess just explicitly rather than hidden in a Deno directory so maybe not so bad)? Nothing that couldn’t be solved with some documentation I suppose 🤔

cmorten commented 3 years ago

Just found https://github.com/deno-library/compress/tree/v0.3.6 which appears to be a typescript implementation of compression libs which we may be able to use instead of denoflate, will spike out this weekend and see if is workable

cmorten commented 3 years ago

Hey @mocoso, can you try out 1.1.0 and see if that has solved it? Please reopen if not!

mocoso commented 3 years ago

It works very nicely. Thank you for resolving this.