Adding support for rootURL option #30

jasonmit · 2016-10-18T00:09:58Z

Currently, applications that depend on rootURL do not work well with ember-cli-sri. This PR enables support for rootURL.

The other half: jonathanKingston/broccoli-sri-hash#25

jonathanKingston · 2016-10-18T01:21:08Z

Hey!

It's been a while since I have touched Ember however this article suggests that the rootURL is where the assets of the applications live for dev and live, is this not the case?

From a quick glance it doesn't look like you are catering for the CDN code path: https://github.com/jasonmit/broccoli-sri-hash/blob/dec523b00597643e23b027a74fcfdbf784f49f96/index.js#L228 (the non external code path is actually just to teach Ember developers about SRI whilst in development)

Could you give me some sample output before the SRI code is applied and after?

jasonmit · 2016-10-18T01:47:58Z

From a quick glance it doesn't look like you are catering for the CDN code path: https://github.com/jasonmit/broccoli-sri-hash/blob/dec523b00597643e23b027a74fcfdbf784f49f96/index.js#L228 (the non external code path is actually just to teach Ember developers about SRI whilst in development)

Since there are many possibilities that (rootURL, fingerprint prefix, fingerprint prefix + rootURL), perhaps we expose a method that extends the responsibility of determining what the prefix is to the consuming app. By default, it will remain fingerprint.prefix but could be overridden to implement complex decisions.

SRI: {
  // scenario where all three possibilities can be handled
  prefix(hrefSrc) {
    const rootURL = '/home';
    const cdn = 'https://site.com';

    if (hrefSrc.startsWith(cdn)) {
      if (hrefSrc.substr(0, cdn.length).startsWith(rootURL)) {
        return cdn + rootURL;
      }

      return cdn;
    }

    return rootURL;
  }
}

Could you give me some sample output before the SRI code is applied and after?

<!-- before -->
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>SriExampleApp</title>
    <meta name="description" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1">

    {{content-for "head"}}

    <link rel="stylesheet" href="{{rootURL}}assets/vendor.css">
    <link rel="stylesheet" href="{{rootURL}}assets/sri-example-app.css">

    {{content-for "head-footer"}}
  </head>
  <body>
    {{content-for "body"}}

    <script src="{{rootURL}}assets/vendor.js"></script>
    <script src="{{rootURL}}assets/sri-example-app.js"></script>

    {{content-for "body-footer"}}
  </body>
</html>

<!-- after -->
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>SriExampleApp</title>
    <meta name="description" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <meta name="sri-example-app/config/environment" content="%7B%22modulePrefix%22%3A%22sri-example-app%22%2C%22environment%22%3A%22production%22%2C%22rootURL%22%3A%22/home%22%2C%22locationType%22%3A%22auto%22%2C%22EmberENV%22%3A%7B%22FEATURES%22%3A%7B%7D%7D%2C%22APP%22%3A%7B%22name%22%3A%22sri-example-app%22%2C%22version%22%3A%220.0.0+53d2d8de%22%7D%2C%22exportApplicationGlobal%22%3Afalse%7D" />
    <link rel="stylesheet" href="/home/assets/vendor-d41d8cd98f00b204e9800998ecf8427e.css" integrity="sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= sha512-z4PhNX7vuL3xVChQ1m2AB9Yg5AULVxXcg/SpIdNs6c5H0NE8XYXysP+DGNKHfuwvY7kxvUdBeoGlODJ6+SfaPg==" >
    <link rel="stylesheet" href="/home/assets/sri-example-app-d41d8cd98f00b204e9800998ecf8427e.css" integrity="sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU= sha512-z4PhNX7vuL3xVChQ1m2AB9Yg5AULVxXcg/SpIdNs6c5H0NE8XYXysP+DGNKHfuwvY7kxvUdBeoGlODJ6+SfaPg==" >
  </head>
  <body>
    <script src="/home/assets/vendor-5e921d36d45861ec49e17e670290803f.js" integrity="sha256-ELwDpLGC8S7kHpTHN7+9sk80T4hXoyDT/ElRaiW4hKI= sha512-pbtpLYyEb2KfHtwADvD4iG28+g92kloMpuOXZzatoqx6BId87tQZc7QxDtflJxX9LzntCeDd5b3jnV+BjM946g==" ></script>
    <script src="/home/assets/sri-example-app-a2d5bce0ba33db7479a6697901cdd499.js" integrity="sha256-/77Nqfo+mI8ojYXIYngAq0Nl9VUMB/9xi565YUhhHn4= sha512-Kk+lMiZlIdc7L0z9BJoKTqK18hdjK+rWkeay7h19rUCC8zLcHh0Ix3FPz5Z8oTmnZ3QuwS+uD3aaGV9lnTUL0Q==" ></script>
  </body>
</html>

jonathanKingston · 2016-10-18T07:50:21Z

Right, isn't the expectation that rootURL has the same path in dev too? So it shouldn't start with a / etc?

The SRI script will be receiving code like: <script src="home/assets/vendor-5e921d36d45861ec49e17e670290803f.js" and the file will be at: myproject/home/assets/vendor.js.

Then when deployed you would get: <script src="https://example.com/prepend/something/home/assets/vendor-5e921d36d45861ec49e17e670290803f.js"

Could you confirm @Turbo87?

Turbo87 · 2016-10-18T08:17:46Z

I have to admit I haven't used the SRI addon much yet so it's a bit hard for me to follow what's going on here. What you commented seems to make sense to me although nothings stopping you from using a different rootURL in production and development mode.

jonathanKingston · 2016-10-18T08:30:23Z

@Turbo87 I'm kinda lost why the file prefix needs to be taken into account here, the code currently strips off CDN prefixes as they won't be local file paths however the rootURL surely is part of the application structure is it not?

Turbo87 · 2016-10-18T08:33:22Z

are we talking about the structure on the file system? the rootURL is only related to the path at which the app is deployed on the server, meaning the URL part, not necessarily the file system.

jonathanKingston · 2016-10-18T09:17:54Z

Surely that is the use case for fingerprint.prefix?

If that is the case @jasonmit the patch can probably just concat fingerprint.prefix and rootURL to the prefix and not touch the other lib.

Turbo87 · 2016-10-18T09:43:35Z

@jonathanKingston no, you need to think of assets and the ember router in different ways. rootURL is mainly used for the router to generate URLs, and in the default configuration it is also used for the assets URLs, but those can also be somewhere else in production due to the fingerprint.prefix.

If that is the case @jasonmit the patch can probably just concat fingerprint.prefix and rootURL to the prefix and not touch the other lib.

I think that depends on whether the user has {{rootURL}} included in the links in their index.html file

jonathanKingston · 2016-10-18T11:51:54Z

So in the case where user has:

<script src="{{rootURL}}/hey/index.js" ></script>

and:

{
  rootURL: 'path/'
}

Would files would be nested in:

myemberapp/path/hey/index.js
myemberapp/hey/index.js

If 1. then the code should be functioning correctly.
However if 2. then the correct solution is probably to pass to broccoli-sri-hash and a lot of changes will likely need to happen:

both paths would need to be checked as to if they exist as this is possible: {rootURL: '/lib'} <script src="{{rootURL}}/index.js" ></script> where both: /lib/index.js exists and /index.js
- I'm not sure if it would be safe to assume that stripping the rootURL would be always safe as users could choose to duplicate the rootURL in the file system causing the wrong file to be checked (perhaps ember-cli should warn instead if it finds a path matching rootURL?).
both paths would need to be checked as the code could also have: {rootURL: '/lib'} <script src="{{rootURL}}/index.js" ></script> and <script src="/lib/index.js" ></script> where both were intended.
The function mentioned above should be part of the code, I'm not convinced opening up more avenues for how this works makes the code better (however happy to hear otherwise)

Turbo87 · 2016-10-18T12:38:36Z

I think the answer to that question is 1)

jonathanKingston · 2016-10-18T13:20:10Z

Hmm so it should be working correctly from my old understanding of the code.
SRI would be given one of these:

<script src="https://myexample.com/path/to/file/index-md5sum.js"></script>
<script src="/path/to/file/index-md5sum.js"></script>

And it would check the file myemberapprepo/path/to/file/index.js and generate:

<script src="https://myexample.com/path/to/file/index-md5sum.js" integrity="..." crossorigin="anonymous"></script>
<script src="/path/to/file/index-md5sum.js" integrity="..." crossorigin="anonymous"></script>

jasonmit · 2016-10-18T14:09:35Z

The rootURL does not indicate where the files are on the file system. So in my experience, it is #2.

Consider an nginx server sitting in front of your app and in your nginx config you proxy all requests to /home to a webserver. The rootURL for your ember app needs to be /home so assets sources get routed to the correct app through nginx and URL get prefixed with /home so that URLs generated are mapped correctly.

I don't think it's safe to assume fingerprint.prefix + rootURL since not all assets will have rootURL. And not all rootURL paths will have fingerprint.prefix (consider the exclude option to asset-rev).

jonathanKingston · 2016-10-18T15:46:36Z

@jasonmit can't you change your live vs dev prefix? If you can't I'm not sure why you would need to stick the prefix on the file paths either.

I just get the feeling this will add either lots more complexity or slow down everyone elses setups as you have to check two paths rather than one.

jasonmit · 2016-10-18T15:57:27Z

The live and dev rootURLs are identical in this case since nginx is used within development.

As for the complexity concern, I agree and that is why I propose adding a method where the consumer can implement the complexity on their end. By default the current functionality will remain the default implementation.

The reason is, I would have to check at least 3 locations on the file system and even then I feel we're making too many assumption as people may have they're own aliases for asset paths. For example, an image resizing/compression service sitting in front of some image sources. ember-cli-sri would have no way of knowing how to look those up on the filesystem.

jonathanKingston · 2016-10-18T18:41:18Z

You likely won't want it working on this compression service to be fair.

I'm not convinced by the function however happy to review patches when you have them :)

jasonmit · 2016-10-18T18:45:44Z

You likely won't want it working on this compression service to be fair.

Good point. Contrived example but you can imagine someone setting up redirect middleware minus mutating the asset.

Do you have any suggestions of an approach you would be more open on? I figured it offered the most flexibility without the perf hit of checking many permutations against the fs.

I do believe some degree of rootURL support is needed as more will be flocking to it with base deprecated. I want to help get there with the least amount of churn

jonathanKingston · 2016-10-19T10:02:54Z

Er I would keep the same default behaviour (up until someone points out this is a common use case etc). Then pass the function through to the other lib and move the code looking for files out into it's own function checking for a prefix function.
There likely will be a fair bit of moving code as the code should still do the external url checks ensuring the developer has specified crossorigin for an external domain (where the application domain doesn't matter)

app on localhost doesn't need to CORS check localhost files
app on example.com doesn't need to CORS check relative path files
app on example.com doesn't need to CORS check example.com files
app on example.com does need to CORS check example.co.uk files

The code tries to do as many shortcuts as possible that are safe defaults as to give SRI to as many developers as possible without breaking their setups via options.crossorigin.

I actually can't remember why this code path exists: https://github.com/jonathanKingston/broccoli-sri-hash/blob/master/index.js#L202 perhaps that could be cleaned up to remove an aditional file read too?!
However certainly another read file could be removed by adding an optional assetSource arg to this function for when used from checkExternal: https://github.com/jonathanKingston/broccoli-sri-hash/blob/master/index.js#L149 (I think I was just adverse to this when writing to keep the functions simpler - perhaps the code could be simplified so long as files are not read when it's not advantageous based on CORS as the early return statements in checkExternal: https://github.com/jonathanKingston/broccoli-sri-hash/blob/master/index.js#L191)

Feel free to ping me if none of this make sense / you need help etc :)

jasonmit · 2016-10-21T04:07:28Z

Er I would keep the same default behaviour (up until someone points out this is a common use case etc).

Unsure what you mean, are you wondering why someone would change the rootURL to anything other than /? That is the case I'm trying to solve for, anything other than slash currently will cause SRI to skip the asset since it doesn't find it on the file system.

The more complex case is handling rootURL + a CDN. Still unclear how to solve that elegantly, so I might defer on fixing that this time around. We'll likely need to tackle this anyway in the future, so happy to pick it back up in a month or so to land that feature.

jonathanKingston · 2016-10-21T09:34:04Z

As in if you are going to provide an override function for developers by default it should just behave how the code currently does and not check more files than it needs to etc.

kellyselden · 2017-03-17T16:46:51Z

I was just hit hard by this bug, and Jason's branches work perfectly. I think the fundamental issue is that rootURL has nothing to do with path on disk. In an app with a rootURL of "web", ember build still puts everything in random_broccoli_path/assets/vendor.js and this always fails to find it at random_broccoli_path/web/assets/vendor.js.

There may be some more discussion about edge cases in this thread, but I believe these PRs are the right starting point.

Also, I removed my rootURL and these branches still pick up the file.

Adding support for rootURL option

1c0ff77

jasonmit mentioned this pull request Oct 18, 2016

Adding support for rootURL option jonathanKingston/broccoli-sri-hash#25

Open

jasonmit force-pushed the rooturl branch from 968976d to 1c0ff77 Compare October 18, 2016 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for rootURL option #30

Adding support for rootURL option #30

jasonmit commented Oct 18, 2016 •

edited

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 •

edited

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 •

edited

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 •

edited

jonathanKingston commented Oct 19, 2016

jasonmit commented Oct 21, 2016 •

edited

jonathanKingston commented Oct 21, 2016

kellyselden commented Mar 17, 2017

Adding support for rootURL option #30

Are you sure you want to change the base?

Adding support for rootURL option #30

Conversation

jasonmit commented Oct 18, 2016 • edited

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 • edited

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

Turbo87 commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 • edited

jonathanKingston commented Oct 18, 2016

jasonmit commented Oct 18, 2016 • edited

jonathanKingston commented Oct 19, 2016

jasonmit commented Oct 21, 2016 • edited

jonathanKingston commented Oct 21, 2016

kellyselden commented Mar 17, 2017

jasonmit commented Oct 18, 2016 •

edited

jasonmit commented Oct 18, 2016 •

edited

jasonmit commented Oct 18, 2016 •

edited

jasonmit commented Oct 18, 2016 •

edited

jasonmit commented Oct 21, 2016 •

edited