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.

Sign up for GitHub

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

Jump to bottom

jsAttributesHandler only applied to scripts with a src location #1835

Closed
dten opened this issue Jun 6, 2024 · 9 comments
Closed

jsAttributesHandler only applied to scripts with a src location #1835

dten opened this issue Jun 6, 2024 · 9 comments

Comments

@dten
Copy link
Contributor

dten commented Jun 6, 2024 •
edited
Loading

This very much follows on from this other ticket from some years back btw #1621

We are trying to add a CSP nonce value to the script produced by widgets with julius, but the attributes provided by jsAttributesHandler
are only applied if that scripts has a srcLoc, which is only possible if addStaticContent was used to cache that script somewhere.

The related code is here https://github.com/dten/yesod/blob/master/yesod-core/src/Yesod/Core/Class/Yesod.hs#L596

it looks like this MR was the first to add the attributes to only the ones with a location #1308

So as far as I can see at the moment there's no way to get attributes to the inline script created from julius and so no way to add a csp nonce.

I'm proposing adding the same jsAttrs to the inline script also (as the docs seem to claim would happen) and happy to do an MR if that's acceptable. My worry is that would be a change for anyone who was only caching some of the scripts.
@dten
Copy link
Contributor Author

dten commented Jun 6, 2024 •
edited
Loading

in addition the attributes do not appear to end up on the <script> in <head> produced from using toWidgetHead with julius (though i suspect there's a way around that)

@jezen
Copy link
Member

jezen commented Jun 11, 2024

I can confirm that this works:

toWidget [julius|alert('!!');|]

…but this doesn't:

toWidgetHead [julius|alert('!!');|]

The framework should make it frictionless to implement an appropriate CSP for any given page, so a pull request to improve this situation is certainly welcome 🙂

@dten
Copy link
Contributor Author

dten commented Jun 11, 2024

hmm in our project that toWidgt julius does not turn up with jsAttributesHandler's values. I'll try make a small project to demo what I mean

@jezen
Copy link
Member

jezen commented Jun 11, 2024

That's useful context, and yes, a small demo would be a good idea.

Here are my notes from a few years ago which you may also find useful.

https://jezenthomas.com/2019/09/implementing-csp-in-yesod/

@dten
Copy link
Contributor Author

dten commented Jun 11, 2024

Yep I actually started from your blog 😊 it was very useful

@dten
Copy link
Contributor Author

dten commented Jun 18, 2024 •
edited
Loading

@jezen i've written a test that fails to add the attributes to the script

dten@eca5e59

the result is

  test/YesodCoreTest/JsAttributes.hs:33:7:
  1) Test.JsAttributes script in body gets attributes
       Expected response body "<!DOCTYPE html>\n<html><head><title></title><script>/*head*/</script></head><body><script src=\"load.js\"></script><script attr0=\"a\">/*body*/</script></body></html>", but received "<!DOCTYPE html>\n<html><head><title></title><script>/*head*/</script></head><body><script src=\"load.js\"></script><script>/*body*/</script></body></html>"

@dten
Copy link
Contributor Author

dten commented Jun 18, 2024

and adding the attributes to non-loc scripts fixes it dten@a1ea911

@dten
Copy link
Contributor Author

dten commented Jun 26, 2024

I'm happy to do a PR for this if seems acceptable

@jezen
Copy link
Member

jezen commented Jun 28, 2024

@dten Of course, that would be welcome.

@dten dten mentioned this issue Jul 8, 2024
Merged
3 tasks
@jezen jezen closed this as completed Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dten @jezen