feat(unstable): Support data: urls #5157

SyrupThinker · 2020-05-08T16:56:22Z

Notes:

percent-encoding is already an indirect dependency of url
The disk cache name might need revising
Graceful failure is probably desired in extract_data_url, what kind of Error should be returned?

kitsonk

What happens with media types we don't support, like text/plain? It would be good to have some non-happy path tests for this.

SyrupThinker · 2020-05-08T21:47:25Z

What happens with media types we don't support, like text/plain? It would be good to have some non-happy path tests for this.

Currently they get treated as JavaScript because it falls through here:

deno/cli/global_state.rs

Lines 146 to 149 in fca204d

    
           _ => Ok(CompiledModule { 
        
             code: String::from_utf8(out.source_code)?, 
        
             name: out.url.to_string(), 
        
           }),

kitsonk · 2020-05-08T21:52:41Z

Hrmmm... I didn't think we allowed media types we didn't know. @bartlomieju are we sure we want that? Seems like a big security hole.

Non-happy path tests would still be good.

SyrupThinker · 2020-05-08T21:56:08Z

Non-happy path tests would still be good.

Working on that and checking whether removing that fallback causes regressions

SyrupThinker · 2020-05-08T22:28:31Z

I'd like to have some feedback on how to handle extract_data_url and unknown media type failure first because the non-happy test results depend on that.

ry

This is an interesting feature. The patch looks cleanly implemented.

How are data-urls used with javascript typically? I don't understand the use case.

Do data urls work like this in Chrome?

cli/tests/integration_tests.rs

SyrupThinker · 2020-05-09T10:38:20Z

How are data-urls used with javascript typically?

The usages I encounter are generally data embedding.

Do data urls work like this in Chrome?

Both Firefox and Chrome allow imports of that form.
This implementation diverges in behaviour when the data url is malformed or has an incompatible media type.
The browsers throw a type error, this currently panics for the former and treats it as JavaScript in the latter.

ry · 2020-05-09T11:24:57Z

Ok sounds good.

I think we should make this an unstable feature for the time being. I'd leave it up to you to figure out how to do that exactly (if you don't mind). Here's reference:

deno/cli/state.rs

Lines 236 to 248 in 2b02535

    
             /// Quits the process if the --unstable flag was not provided. 
        
             /// 
        
             /// This is intentionally a non-recoverable check so that people cannot probe 
        
             /// for unstable APIs from stable programs. 
        
             pub fn check_unstable(&self, api_name: &str) { 
        
               // TODO(ry) Maybe use IsolateHandle::terminate_execution here to provide a 
        
               // stack trace in JS. 
        
               let s = self.0.borrow(); 
        
               if !s.global_state.flags.unstable { 
        
                 exit_unstable(api_name); 
        
               } 
        
             } 
        
           }

bartlomieju · 2020-05-09T11:47:33Z

Thanks for the patch @SyrupThinker; however I think we should wait on this PR after 1.0 is released and compiler refactor is finished.

It introduces yet another layer to consider and if I'm not mistaken data imports are dependent on location/origin which we currently don't support.

Hrmmm... I didn't think we allowed media types we didn't know. @bartlomieju are we sure we want that? Seems like a big security hole.

That's how it always worked: unknown media types are treated as if they were JS which in most cases ends up in an error from V8 or TS compiler.

SyrupThinker · 2020-05-09T11:50:40Z

Ok, I'll continue working on this after the refactor then.

SyrupThinker · 2020-06-06T18:57:09Z

It introduces yet another layer to consider and if I'm not mistaken data imports are dependent on location/origin which we currently don't support.

That is correct; we certainly don't want to allow circumventing restrictions by imports like https > data > file or https > data > http.

This is done to prevent modules from being able to proxy forbidden imports through data. This should be replaced with proper origin tracking at a later time.

CLAassistant · 2020-07-27T23:51:08Z

All committers have signed the CLA.

ry · 2020-08-18T18:56:35Z

@SyrupThinker I'm sorry we didn't get this landed earlier. If you can get this rebased on master, I would include it in 1.4.0 (to be released September 13)

ry · 2020-08-31T14:48:10Z

Since the main use case for this feature is for loading workers, it would be good to have a test case demonstrating that you can start a worker with a data url.

SyrupThinker · 2020-09-08T15:58:14Z

I don't know how to resolve the tsc errors (or how to debug them), could use some help for that.

For example I can't figure out how to log stuff in cli/tsc/00_typescript.js.

bartlomieju

MDN tells that data: URLs are not allowed at top level due to phishing vulnerability. I wonder if we also should disallow them as entry point 🤔

EDIT: I guess it doesn't make much difference - we still require --allow-* flags.

cli/tests/integration_tests.rs

bartlomieju · 2020-09-09T10:02:30Z

cli/tsc.rs

@@ -687,6 +687,7 @@ impl TsCompiler {
      self.file_fetcher.clone(),
      global_state.maybe_import_map.clone(),
      permissions.clone(),
+      global_state.flags.unstable,


Not sure if we need to hide it behind unstable

bartlomieju · 2020-09-11T09:42:59Z

@SyrupThinker I have fixed the tests. TS failures were caused by #7392

bartlomieju

LGTM, thank you @SyrupThinker

This reverts commit e3319f3.

Add support for importing data url's

1f1010a

Closes #5059

kitsonk reviewed May 8, 2020

View reviewed changes

ry reviewed May 9, 2020

View reviewed changes

cli/tests/integration_tests.rs Outdated Show resolved Hide resolved

bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) labels May 15, 2020

bartlomieju added this to the 1.1.0 milestone May 21, 2020

Merge branch 'master' into data_imports

5e172fa

ry requested a review from bartlomieju June 6, 2020 17:52

SyrupThinker added 4 commits June 6, 2020 23:09

Minor fixes and cleanup

a17eb80

Forbid import from data:// modules

025574c

This is done to prevent modules from being able to proxy forbidden imports through data. This should be replaced with proper origin tracking at a later time.

Mark data imports unstable

58c18d1

Add non-happy test

f11a0f1

ry modified the milestones: 1.1.0, future Jun 10, 2020

ry modified the milestones: future, 1.4.0 Aug 18, 2020

SyrupThinker added 2 commits September 8, 2020 00:49

Merge branch 'master' into data_imports

7f5e169

Merge branch 'master' into data_imports

035c77d

Add worker test

bfffbd6

Fix lint

de890ff

bartlomieju reviewed Sep 9, 2020

View reviewed changes

bartlomieju added 3 commits September 11, 2020 11:38

rewrite as itests

bb6ea02

Merge branch 'master' into data_imports

3b478fc

fix remaining test

ad262f9

bartlomieju changed the title ~~Add support for importing data url's~~ feat: Support data: urls Sep 11, 2020

bartlomieju approved these changes Sep 11, 2020

View reviewed changes

Merge branch 'master' into data_imports

e4b44f5

bartlomieju changed the title ~~feat: Support data: urls~~ feat(unstable): Support data: urls Sep 11, 2020

bartlomieju added 2 commits September 11, 2020 18:38

Merge branch 'master' into data_imports

2430393

fix

ad5974f

bartlomieju merged commit e3319f3 into denoland:master Sep 11, 2020

SyrupThinker deleted the data_imports branch September 11, 2020 21:13

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Sep 12, 2020

Revert "feat(unstable): Support data: urls (denoland#5157)"

9127222

This reverts commit e3319f3.

bartlomieju mentioned this pull request Sep 12, 2020

Reland data: url support #7433

Closed

bartlomieju added a commit that referenced this pull request Sep 12, 2020

Revert "feat(unstable): Support data: urls (#5157)" (#7432)

60dbc6b

This reverts commit e3319f3.

kitsonk mentioned this pull request Dec 23, 2020

feat(cli): support data urls #8866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(unstable): Support data: urls #5157

feat(unstable): Support data: urls #5157

SyrupThinker commented May 8, 2020

kitsonk left a comment

SyrupThinker commented May 8, 2020

kitsonk commented May 8, 2020

SyrupThinker commented May 8, 2020

SyrupThinker commented May 8, 2020

ry left a comment

SyrupThinker commented May 9, 2020 •

edited

Loading

ry commented May 9, 2020

bartlomieju commented May 9, 2020

SyrupThinker commented May 9, 2020 •

edited

Loading

SyrupThinker commented Jun 6, 2020

CLAassistant commented Jul 27, 2020 •

edited

Loading

ry commented Aug 18, 2020

ry commented Aug 31, 2020

SyrupThinker commented Sep 8, 2020

bartlomieju left a comment •

edited

Loading

bartlomieju Sep 9, 2020

bartlomieju commented Sep 11, 2020

bartlomieju left a comment

feat(unstable): Support data: urls #5157

feat(unstable): Support data: urls #5157

Conversation

SyrupThinker commented May 8, 2020

kitsonk left a comment

Choose a reason for hiding this comment

SyrupThinker commented May 8, 2020

kitsonk commented May 8, 2020

SyrupThinker commented May 8, 2020

SyrupThinker commented May 8, 2020

ry left a comment

Choose a reason for hiding this comment

SyrupThinker commented May 9, 2020 • edited Loading

ry commented May 9, 2020

bartlomieju commented May 9, 2020

SyrupThinker commented May 9, 2020 • edited Loading

SyrupThinker commented Jun 6, 2020

CLAassistant commented Jul 27, 2020 • edited Loading

ry commented Aug 18, 2020

ry commented Aug 31, 2020

SyrupThinker commented Sep 8, 2020

bartlomieju left a comment • edited Loading

Choose a reason for hiding this comment

bartlomieju Sep 9, 2020

Choose a reason for hiding this comment

bartlomieju commented Sep 11, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

SyrupThinker commented May 9, 2020 •

edited

Loading

SyrupThinker commented May 9, 2020 •

edited

Loading

CLAassistant commented Jul 27, 2020 •

edited

Loading

bartlomieju left a comment •

edited

Loading