Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

SRI by default #20

Merged
merged 1 commit into from
Jul 19, 2015
Merged

Conversation

jonathanKingston
Copy link
Member

@jonathanKingston
Copy link
Member Author

/cc @stefanpenner @rwjblue @metromoxie

@stefanpenner
Copy link
Contributor

It would be great for future work to integrate with ember-deploy, which I believe could automatically enable SRI for assets deployed to cdn.

It would be good if that deploy step would also valid the deploy validates.

Something worth adding to this rfc. The progress, browser support and potential for spec completion and future browser adoption.

@stefanpenner
Copy link
Contributor

@lukemelia I would love your thoughts on the ember deploy side of things (or maybe you can pull in the appropriate people)

@stefanpenner
Copy link
Contributor

It is also worth noting, a segment on the site will need to be added if we pull this in.

@lukemelia
Copy link

The next version of ember-cli-deploy 0.5.0 is based on a pipeline + plugins architecture. A plugin would be be an excellent fit for adding SRI attributes.

If we went that route, we would potentially lose the "SRI-by-default" benefit. But, personally, I would like to see the fingerprinting and prepending also become deploy pipeline plugins eventually. I think understanding their order of execution would be a lot easier that way.

/cc @achambers @LevelbossMike

@achambers
Copy link
Member

@lukemelia We want to prepackage the build plugin by default so that ember-cli-deploy can always at least do that. We could do the same thing with the SRI plugin too right? So, by default your deploy will build with SRI attributes.

@jonathanKingston
Copy link
Member Author

Something worth adding to this rfc. The progress, browser support and potential for spec completion and future browser adoption.

Will add to both the plugin README and RFC tonight.

It is also worth noting, a segment on the site will need to be added if we pull this in.

100% and mostly this should be a copy and paste from the README

I would like to see the fingerprinting and prepending also become deploy pipeline plugins eventually.

Can I suggest that this will lock out people with custom build processes like ourselves, if instead an addon could also be a plugin this might make a transition simpler.
Ultimately if Ember is removing this, then I will likely be forced to either hack into our deploy step ember-build or somehow make our own with the underlying broccoli plugins.

So if an addon could know when it was being used as a build process plugin it could give a better experience there. So for example if told by ember-build the origin and the crossorigin status of the CDN domain the code in its current state would give the best experience. The addon would just need to know when it is going to be used in this manner.

Ultimately the addon is only a paper thin bit of code passing through to the broccoli however does ember really want an addon and a build plugin for everything similar to this?

Either that or the ember build command itself always uses deploy and by default builds to dist/.

@stefanpenner
Copy link
Contributor

Ultimately if Ember is removing this, then I will likely be forced to either hack into our deploy step ember-build or somehow make our own with the underlying broccoli plugins.

Is there a path where ember-deploy can live healthily in you existing flow? I can't help but feel we can do an even better job with more input.

Ultimately, I don't want to force these concepts to be entangled if doing so causes many users grief.

@jonathanKingston
Copy link
Member Author

Is there a path where ember-deploy can live healthily in you existing flow?

Pretty much the best you could do is make the default config use the build/ dir. I have thought about it at great length and there are lots of issues that I can see (some which may be resolvable but others are unlikely)

We are likely an edge case however here is a few issues I see that don't fit:

  • Config is available to all developers, this doesn't work for us
  • Build systems like docker/chef/puppet are not hooked in easily
  • QA process can't be done by a developer so giving them ability to put straight to live isn't ok
  • Ultimately in a security environment we wish to prove that what was tested on staging is the same package of files that goes onto live
  • Without a robust API version system on backend they need to roll out together (for us this risk isn't worth it)
    • Which means that redis store would need to be replaced with something that can speak to the backend build process (which seems to defeat the point of the whole system here).
  • Customising the hooks to fit how we build our app will take a whole heap of time
  • Giving another application write access to our CDN isn't good
  • Requiring a new NoSQL for index storage for the server isn't good - we would likely need to write a new storage to use symlinks instead

@stefanpenner
Copy link
Contributor

We are likely an edge case however here is a few issues I see that don't fit:

we should revisit this outside of this RFC, sorry for bringing it up. But thanks for the details.

@jonathanKingston
Copy link
Member Author

@stefanpenner yeah I was going to move that to be a issue on ember-cli-deploy but I don't really know the roadmap if ember build is being ripped out of ember-cli.

However the code be it in ember-cli core or ember-cli-deploy can be moved to the new way later.

Right now ember-cli could give relative path SRI out the box with zero config safely.

The transition to deploy would not impact a developer using ember-cli as the attributes should be the same.

@stefanpenner
Copy link
Contributor

if ember build is being ripped out of ember-cli.

I don't see that happening. But clearly some better coordiation between deploy + build is needed.

@jonathanKingston
Copy link
Member Author

Some further outstanding issues:

  • Non stylesheet resources need to not be given an integrity attribute (This may be added to a later spec however it isn't supported so developers shouldn't be given false hope) - I will be patching this once my test pass
  • The spec may be changing to fail-close on lack of crossorigin attribute for CORS which will lead to simpler documentation on our side 👍

@jonathanKingston
Copy link
Member Author

@stefanpenner here are the commits as mentioned:
jonathanKingston/ember-cli@0eecd25
jonathanKingston/ember-cli@2599b6e

This expects version 1.0.0 of the addon which has not been published yet, however this should be ready by the end of the day.

The change for the specification to fail-close should make no difference to us as it should not ever happen. Mailing list

ember-cli-sri uses: broccoli-sri-hash uses: sri-toolbox

@lukemelia
Copy link

@jonathanKingston what is the relationship between SRI and gzipping? Is the provided fingerprint expected to be after decoding content or before?

In practical terms, will gzipping images, css & js as part of a deployment process invalidate the SRI fingerprints?

@jonathanKingston
Copy link
Member Author

@lukemelia it is checked after gzipping (I agree the algo is not so clear - I'll raise that as an issue).

The company I work for is using a slightly older version of the addon: https://portal.cyber-ami.com/ (icons should not have the integrity) however we use gzip and the page gets checked correctly in the latest chrome.

Images are not checked in the latest version of the specification as that will be a level 2 bit of work along with nested images within CSS.

@mozfreddyb
Copy link

@lukemelia SRI is applied on the representation data, i.e., it undoes transport encoding like gzip.

@lukemelia
Copy link

@jonathanKingston @mozfreddyb Thanks, I've been working on the next version of ember-cli-deploy, whose plugin ecosystem will include https://github.com/lukemelia/ember-cli-deploy-gzip. It doesn't sound like that will interfere with SRI providing gzipping occurs after SRI runs. 👍

@jonathanKingston
Copy link
Member Author

@mozfreddyb thanks, I was looking purely at the fetch algo changes and wasn't seeing it. But then transport encoding doesn't seem to be mentioned in fetch either?

stefanpenner added a commit that referenced this pull request Jul 19, 2015
@stefanpenner stefanpenner merged commit 4d59271 into ember-cli:master Jul 19, 2015
@jonathanKingston
Copy link
Member Author

@stefanpenner thanks 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants