Media Type enforcement #1020

kitsonk · 2018-10-19T11:36:00Z

Fixes #702

This PR is a WIP for media type enforcement.

When remote modules are retrieved, the privileged resolves the media type of the file. First it respects the Content-Type header if present. If no header present, it will utilise the extension. For local modules the privileged side always uses the extension to determine the media type.

The Deno compiler then respects that media type when providing file type information to TypeScript.

Things not done yet:

When the code is cached, the media type needs to be cached as well, so when it is retrieved it can be passed on to userland.
Tests.
gist returns text/plain for files. At the moment, that assigns it a MediaType.Unknown which the compiler assumes is then JavaScript. What to do with media types that are not "valid" needs to be figured out.

ry · 2018-10-19T14:23:47Z

src/deno_dir.rs

@@ -104,26 +105,59 @@ impl DenoDir {
    }
  }

+  // convert a ContentType string into a enumerated MediaType
+  fn map_content_type(self: &DenoDir, path: &Path, content_type: Option<&str>) -> msg::MediaType {


It doesn't seem like this needs to be a method, since it doesn't use self. Break this out into a standalone function. Add a test for it in this file.

Ok, I will, I just was struggling with how to invoke it. I will figure that out... I guess it needs to go outside of the impl. It is obvious now that I say it.

ry

Awesome - I'm glad you're working on this!
Regarding testing I think tools/http_server.py should be modified to have a handler that returns different content-type headers.
Looks like a good start - ping me when I should review again.

kitsonk · 2018-10-20T01:51:05Z

@ry do you have an opinion on how we should resolve unknown media types? Specifically, gist returns things as text/plain. Currently Rust indicates it is MediaType.Unknown but Deno compiler assumes that anything unknown is JavaScript.

A couple solutions I can think of:

We really follow the Content-Type strictly, so if we have a media type that is not known that we strictly pass Unknown along and the Deno compiler will throw terminally if an unknown module is resolved.
If we find a Content-Type that we don't recognise, Rust then falls back to looking at the extension. Currently if we don't get a Content-Type or it is a local file, this is how it is done.
We specifically only look at extension for a couple key media types, like text/plain.

Browsers tend to be really really strict about it, so it feels like we should be too. It does mean that we can't host .ts or .js files on Gist (or we have to point at other URIs that proxy it and provide the correct media types).

ry · 2018-10-20T05:13:32Z

@kitsonk Looking at the extension should solve it for gists ? I think we should use the extension. Otherwise I have no opinion.

kitsonk · 2018-10-21T04:40:54Z

@ry ok, I think this is now complete and ready for review.

So for the "gist" issue, if the media type is text/plain then Deno reverts to looking at the extension. If it isn't one of the known media types though, privileged will pass MediaType.Unknown and the Deno compiler with terminally throw indicating it can't deal with an unknown media type.

The media type that is received for remote modules is cached along side the source code with a .mime extension, which is then read back in when a cached remote module is read. Local modules have their type determined simply by extension.

ry

Looks good! Works for me locally

tools/http_server.py

src/http_util.rs

ry · 2018-10-21T15:30:19Z

src/http_util.rs

@@ -27,7 +28,7 @@ pub fn get_client() -> Client<Connector, hyper::Body> {

 // The CodeFetch message is used to load HTTP javascript resources and expects a
 // synchronous response, this utility method supports that.
-pub fn fetch_sync_string(module_name: &str) -> DenoResult<String> {
+pub fn fetch_sync_string(module_name: &str) -> DenoResult<(String, String)> {


How about returning DenoResult<(String, msg::MediaType)> ?

I took a look at this and I decided not to change it... within http_util it doesn't really have a concept of the enum yet, as that is decided in deno_dir.rs. So a decision here, might not be right when combined with other logic contained in deno_dir.rs, so I would like to leave it as (String, String).

src/deno_dir.rs

js/os.ts

ry · 2018-10-21T15:38:15Z

Let's try to land this before #1039

kitsonk · 2018-10-22T09:44:42Z

Hmmm... the appveyor ./tools/format.py reformatted one of the files, but when I run it locally, I don't get any changes.

ry · 2018-10-22T15:44:04Z

@kitsonk Hmm - strange - make sure you're rebased on master and using the rustfmt in third_party.

kitsonk · 2018-10-22T18:38:48Z

@ry I was/am and I was using ./tools/format.py and validated it used the one in third_party. It could have been any number of things with the way GitHub was performing. I will double check again in a few hours.

kitsonk · 2018-10-22T23:16:39Z

@ry 😖 😕

$ git log
commit 080431e5c7acd6e558d6cdbdb8c199c622e6899b (HEAD -> media-types, origin/media-types)
Author: Kitson Kelly <[email protected]>
Date:   Mon Oct 22 13:14:27 2018 +1100

    Enforce media types

commit c4bddc4651f583241193b80634a4f22abb02d582 (upstream/master, origin/master, origin/HEAD, master)
Author: ztplz <[email protected]>
Date:   Mon Oct 22 05:42:18 2018 +0800

    Implemente  clone for FetchResponse (#1054)

commit 47c96a61527f56aa6dfc5a8fc44b13a1db4f4da5
Author: Ryan Dahl <[email protected]>
Date:   Sun Oct 21 13:43:47 2018 -0400

    CI should fail when code isn't formatted.

commit ae0dec39dacbfa4279d1d708bf6b7488c230c272
Author: Ryan Dahl <[email protected]>
Date:   Sun Oct 21 16:09:05 2018 -0400
$ ./tools/format.py
... snip ...
third_party/rustfmt/mac/rustfmt --config-path /Users/kkelly/github/deno/tools/rustfmt.toml src/flags.rs src/tokio_write.rs src/version.rssrc/http_util.rs src/deno_dir.rs src/errors.rs src/main.rs src/tokio_util.rs src/fs.rs src/resources.rs src/isolate.rs src/ops.rs src/libdeno.rs
$ git status
On branch media-types
Your branch is up to date with 'origin/media-types'.

nothing to commit, working tree clean

kitsonk · 2018-10-22T23:56:31Z

Well, and #1065 which currently includes the commit here passed CI on appveyor. Seems to be some sort of odd issue while GitHub was 💥. I suspect restarting the CI would cause the issue to disappear (as well as complete the Travis CI as well).

ry

LGTM - very nice! it seems like the import system is actually functioning quite well now !

- Add URLSearchParams (denoland#1049) - Implement clone for FetchResponse (denoland#1054) - Use content-type headers when importing from URLs. (denoland#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (denoland#1068) - Add separate http/https cache dirs to DENO_DIR (denoland#971) - Support https in fetch. (denoland#1100) - Add chmod/chmodSync on unix (denoland#1088) - Remove broken features: --deps and trace() (denoland#1103) - Ergonomics: Prompt TTY for permission escalation (denoland#1081)

- Add URLSearchParams (#1049) - Implement clone for FetchResponse (#1054) - Use content-type headers when importing from URLs. (#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (#1068) - Add separate http/https cache dirs to DENO_DIR (#971) - Support https in fetch. (#1100) - Add chmod/chmodSync on unix (#1088) - Remove broken features: --deps and trace() (#1103) - Ergonomics: Prompt TTY for permission escalation (#1081)

ry reviewed Oct 19, 2018

View reviewed changes

kitsonk force-pushed the media-types branch 3 times, most recently from 9800db8 to 16b3e62 Compare October 21, 2018 04:34

kitsonk changed the title ~~[WIP] Media Type enforcement~~ Media Type enforcement Oct 21, 2018

ry reviewed Oct 21, 2018

View reviewed changes

kitsonk force-pushed the media-types branch from 16b3e62 to 080431e Compare October 22, 2018 02:16

kitsonk mentioned this pull request Oct 22, 2018

Allow imports of JSON #1065

Merged

Enforce media types

e6ee66c

kitsonk force-pushed the media-types branch from 080431e to e6ee66c Compare October 23, 2018 03:48

ry approved these changes Oct 23, 2018

View reviewed changes

ry merged commit 8ef7da2 into denoland:master Oct 23, 2018

ry mentioned this pull request Oct 27, 2018

v0.1.10 #1109

Merged

kitsonk deleted the media-types branch August 2, 2022 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media Type enforcement #1020

Media Type enforcement #1020

kitsonk commented Oct 19, 2018 •

edited

Loading

ry Oct 19, 2018

kitsonk Oct 19, 2018

ry left a comment

kitsonk commented Oct 20, 2018

ry commented Oct 20, 2018

kitsonk commented Oct 21, 2018

ry left a comment

ry Oct 21, 2018

kitsonk Oct 23, 2018

ry commented Oct 21, 2018

kitsonk commented Oct 22, 2018

ry commented Oct 22, 2018

kitsonk commented Oct 22, 2018

kitsonk commented Oct 22, 2018

kitsonk commented Oct 22, 2018

ry left a comment

Media Type enforcement #1020

Media Type enforcement #1020

Conversation

kitsonk commented Oct 19, 2018 • edited Loading

ry Oct 19, 2018

Choose a reason for hiding this comment

kitsonk Oct 19, 2018

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

kitsonk commented Oct 20, 2018

ry commented Oct 20, 2018

kitsonk commented Oct 21, 2018

ry left a comment

Choose a reason for hiding this comment

ry Oct 21, 2018

Choose a reason for hiding this comment

kitsonk Oct 23, 2018

Choose a reason for hiding this comment

ry commented Oct 21, 2018

kitsonk commented Oct 22, 2018

ry commented Oct 22, 2018

kitsonk commented Oct 22, 2018

kitsonk commented Oct 22, 2018

kitsonk commented Oct 22, 2018

ry left a comment

Choose a reason for hiding this comment

kitsonk commented Oct 19, 2018 •

edited

Loading