Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SRI #20

Closed
jonathanKingston opened this issue Jul 22, 2015 · 73 comments
Closed

Add support for SRI #20

jonathanKingston opened this issue Jul 22, 2015 · 73 comments

Comments

@jonathanKingston
Copy link

As a CDN provider it will soon become good practice to provide integrities.
See:
jsdelivr/jsdelivr#6029
cdnjs/cdnjs#4599

For all static assets that never change can integrities be added?

I'm here to help if anything is needed.

@scottgonzalez
Copy link
Member

Doesn't seem like it'd be too hard to add. Are there any browsers that support SRI yet? I wouldn't want to add this until we can properly test it.

@acorncom
Copy link

Looks like it's currently in the nightlies for both Firefox and Chrome, but hasn't hit stable yet ...

@jonathanKingston
Copy link
Author

Chrome unstable has it in.

The readme of my add-on might be useful here: https://www.npmjs.com/package/ember-cli-sri

@mozfreddyb
Copy link

Bootstrap CDN already shows integrity attributes in its HTML examples, see http://www.bootstrapcdn.com/ and click on the arrows.

Any chances you may want to show these too in your website?

cc @aulvi, whith whom I spoke before about adding Access-Control-Allow-Origin headers to code.jquery.com.

@kborchers
Copy link
Member

Can we circle back to this now? Chrome stable now supports this and Firefox will support it in 43 (December 2015).

@jdorfman
Copy link

@kborchers @scottgonzalez getting this out would be a huge win for Web Application security. Please let me know if I can be of help in anyway.

@scottgonzalez
Copy link
Member

Sure, you can send a PR. Everything is built in the Gruntfile. Thanks!

@jdorfman
Copy link

@scottgonzalez sweet I will see what I can do.

@scottgonzalez
Copy link
Member

Let me know if you have any questions about how everything is built.

@jdorfman
Copy link

@scottgonzalez will do. Going to consult with @jonathanKingston regarding UX. Do you have any preferences in terms of how the code snippets (with the SRI hash) should look? Here is how we do it on BootstrapCDN:

bootstrapcdn_by_maxcdn

@scottgonzalez
Copy link
Member

I don't have any preference, but I expect this to require a major redesign.

@jdorfman
Copy link

@jmervine and I are going to come up with something simple and elegant that will work with the current design. First step will be configuring the Gruntfile.

@jmervine
Copy link
Contributor

@scottgonzalez @jdorfman Generating shouldn't be an issue -- it will require a host that supports running openssl (see: bootstrap-cdn:scripts/integrity.js implementation).

@jdorfman Any direction on updating the UI would be fantastic, otherwise, I'll just drop it in there somewhere and we can discuss making it pretty as a follow up.

@jonathanKingston
Copy link
Author

Sorry for the delay here, what https://www.jsdelivr.com/ do is quite nice and slightly less invasive explaining technology that people might not understand. Requiring developers to enable could actually entice more thought that fancy badges or lengthy explanations do.
I think easy copy facility would be worth having as the output is longer and potentially not completely visible.

The approach I have taken is adding in multiple SRI hashes into the integrity attribute which I feel somewhat explains the fail over principle of the algo used even though totally not required (However my use case installs it into the application anyway). So feel free to use one here if it simplifies the output.

I think a press release informing the user that SRI is good and that the CDN is going out of their way to protect the users of websites should reduce the number of complaints if any.

Other than that, I think keeping it simple actually works in selling this (developers mostly are inquisitive of things they have not seen before)

@jdorfman
Copy link

@jonathanKingston I agree with everything you said. Hopefully we will have something ready to present soon.

@jdorfman
Copy link

@scottgonzalez @jonathanKingston please give me your feedback before we go forward. It is heavily inspired from jsDelivr, which I checked with @jimaek first and got his blessing.

jquery_cdn-sri

@scottgonzalez
Copy link
Member

I think there are a few things we can do to improve the usability of that form:

  • Add help icons with tooltips next to each option.
  • Reword "add the <script> tag" so it's clear that this is changing the text from just a URL to a full element to be placed in the markdown.
  • Add a copy button which copies the text to the clipboard.
@jdorfman
Copy link

jdorfman commented Jan 6, 2016

@scottgonzalez I will take this awesome feedback and get back asap. Thank you.

@jdorfman
Copy link

@scottgonzalez how about this?

popup

@scottgonzalez
Copy link
Member

That looks better. Maybe we can just drop the https option and always include the protocol. We also need a tooltip on the SRI option.

@jdorfman
Copy link

@scottgonzalez couldn't agree more. With that feedback could I skip the next mockup and start the code?

@scottgonzalez
Copy link
Member

I wonder what this would look like if we dropped all of the options and just provided the full set of data to the user. This would result in four copyable blocks:

  • Just the URL
  • Just the hash
  • The full script tag without SRI
  • The full script tag with SRI

I could even see us only providing the script tag with SRI since it's more secure and doesn't cause any issues unless the user manually changes the URL to point to a different source. This would probably result in a similarly sized dialog, but removes the cognitive overhead of deciding if any options should change. This does add cognitive overhead for deciding which block to copy, but I feel like users already know whether they want just the URL or a full script tag when they come here.

What do you think?

@jdorfman
Copy link

@scottgonzalez Good call. I will get that mocked up.

@jdorfman
Copy link

@scottgonzalez after thinking about it, I think the current mockup will cause less confusion. I will still get the other mockup done so we can get some other expert opinions from the jQuery community. I am in no way saying your feedback is wrong or bad I just want a second opinion.

@scottgonzalez
Copy link
Member

No problem. I honestly don't know which will be better.

@jdorfman
Copy link

@scottgonzalez Choose one. I personally like the first image because it has less choices, making a better UX. I think ultimately we should have the jQuery community decide. Edit: There will be a tooltip for Enable SRI, just forgot to add it.

popup2

popup3

@scottgonzalez
Copy link
Member

Oy, I imagined that somehow looking less intimidating without the checkboxes. Definitely the first.

@fmarier
Copy link

fmarier commented Jan 14, 2016

Pardon me for asking a stupid question, but why not make it extremely simple and simply show the full script tag with SRI?

People who just want the URL can trivially copy just that part of the text and those who copy the whole thing get the best script tag they can.

@jonathanKingston
Copy link
Author

@fmarier 👍

Make the src option first in the tag and just use that?

<script src="https://code.jquery.com/jquery-2.1.4.min.js" integrity="sha256-..." crossorigin="anonymous" ></script>
@jdorfman
Copy link

@scottgonzalez @jonathanKingston mockup is now a MVP: http://www.justindorfman.com/temp/jquery-sri-demo.html

@jmervine will be implement it into codeorigin with the grunt tasks.

Almost there.

EDIT: removed annoying GIF

@scottgonzalez
Copy link
Member

A few comments:

  • Only a standard click would open the dialog. Ctrl/cmd+click should not be hijacked.
  • "Copy Below" isn't a very descriptive title.
  • I'd prefer if we used an existing dialog implementation that accounts for things like accessibility. Of course my preference would be jQuery UI.
  • There are some typos/grammatical errors, but we can more easily work through this stuff in a PR.
@jonathanKingston
Copy link
Author

I'd certainly change copy below to something like 'jQuery code integration' or something less stuffy. I also think the paragraph explanation is too long for SRI.

This is a little shorter:

integrity and crossorigin attributes are used for Subresource Integrity (SRI) checking. This allows browsers to ensure that resources hosted on third-party servers have not been tampered with. Use of SRI is recommended as a best-practice, whenever libraries are loaded from a third-party source.
Read more at srihash.org

@jdorfman
Copy link

@scottgonzalez @jonathanKingston great feedback. I will get this done.

@scottgonzalez
Copy link
Member

I prefer larger text, though ideally we'll hold on style discussion until there's a PR since you won't be working from a blank slate like your sample page is right now.

@jdorfman
Copy link

@scottgonzalez so does that mean we are good to go to the next stage?

@scottgonzalez
Copy link
Member

Yeah, when I said:

I think you're good to go ahead and start the implementation.

I assumed the implementation wouldn't require a prototype and you'd start on the actual implementation.

@jdorfman
Copy link

@scottgonzalez my bad, totally missed that. Will send a PR asap.

@jonathanKingston
Copy link
Author

+1 yay looks great. Looking forward to seeing this live.

@kswedberg
Copy link
Member

👍 Thanks so much for your hard work and persistence, @jdorfman !

@jdorfman
Copy link

oh-stop-it

@jdorfman
Copy link

jdorfman commented Feb 2, 2016

@scottgonzalez thoughts on jquery-wp-content/391

@jdorfman
Copy link

@jonathanKingston
Copy link
Author

I think this can be closed once the following link has SRI:
http://jquery.com/download/

There isn't any others on the site right?

Also THANK YOU!

@jdorfman
Copy link

@jonathanKingston good catch. @jmervine are you able make any changes to /download?

@scottgonzalez
Copy link
Member

Separate site, separate maintainers, separate issue tracker... You can file an issue on jquery.com to follow up.

@mgol
Copy link
Member

mgol commented Mar 14, 2016

Separate site, separate maintainers, separate issue tracker... You can file an issue on jquery.com to follow up.

Won't it need similar changes to themes/jquery.com/page.php in the jquery-wp-content repo as in jquery/jquery-wp-content#391 to themes/codeorigin.jquery.com/page.php?

@scottgonzalez
Copy link
Member

Sure, though I don't see how that has anything to do with this repo.

@razamirza
Copy link

Is there any plan to add support for SRI to jQuery.getScript()? http://api.jquery.com/jQuery.getScript/

@jonathanKingston
Copy link
Author

@razamirza to return a SRI hash or to actually do the checking? Either way a new bug exploring those would be worthwhile a discussion. Again not this repo, on: https://github.com/jquery/jquery.

I opened: jquery/jquery#3028

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet