bigskysoftware / htmx

</> htmx - high power tools for HTML
https://htmx.org
Other
38.57k stars 1.31k forks source link

Need help replicating: script-loading issue with nonce-based CSP #2831

Open ehenighan opened 3 months ago

ehenighan commented 3 months ago

Hi,

I previously hit an issue with nonce-based CSP in v1 where the code in evalScript wasn't behaving as I expected - because the browser had moved the nonce value from the attribute into the DOM during node insert (correctly, that's in the spec), the step to copy the attributes wasn't copying the nonce across, and therefore the re-inserted script was failing CSP checks.

I added (locally) this else if:

if (htmx.config.inlineScriptNonce) {
    newScript.nonce = htmx.config.inlineScriptNonce;
} else if (script.nonce) {
    //'script' is still the new script element we're inserting into the DOM - we're just reinserting it
    // to make sure it fires. As a result we need to carry over the internal nonce if present as the CSP
    // may have moved it there from the attribute values
    newScript.nonce = script.nonce;
}

And that dealt with the problem. However, I've just come to try to write a formal unit test for it so I can submit it as a bugfix, and now I can't replicate the problem any more - the attribute always seems to still be there. Is there anyone who can tell me whether or not I'm going mad/help me replicate the problem again?

MichaelWest22 commented 3 months ago

One thing I've run into before is that depending on how you set the CSP via header or with the alternative meta tag it can change the behavior of the browser so test with both methods of setting the CSP. I can't repro the issue you found when I just tried in htmx 2.0.2 with the proper CSP header set though.

The code just above your change has:

      forEach(script.attributes, function(attr) {
        newScript.setAttribute(attr.name, attr.value)
      })

which in my testing sets the attribute value just fine. But I do remember having some weird issues around this previously but I can't make it happen now sorry.

It sounds like you are trying to set nonces properly in your server instead of using the inlineScriptNonce override to replace the nonce? I have an htmx extension https://github.com/MichaelWest22/htmx-extensions/tree/main/src/safe-nonce that can allow you to set nonces properly in both the initial page load and in the ajax incremental page updates that still handles proper nonces. It works by stripping script tags from parital ajax server responses if they don't have a nonce matching the unique nonce in the partial request which allows you to use inlineScriptNonce in a safer way.

MichaelWest22 commented 3 months ago

I think the one place I know I have seen this issue is when pushing back button and reloading content from history as the save to history function does not store nonce data as it should when CSP headers are used instead of meta CSP tag. The history it saves out the raw rendered HTML which then has nonce="" and does not contain the hidden cryptographicnonce stored in the .nonce setting. It would make things much better if the save to history could be improved to copy this value back into the nonce attribute during save. Then the history could be trusted to run scripts without having to just use inlineScriptNonce

MichaelWest22 commented 3 months ago

Just tested adding three lines into the history prep function which solved that issue I was having with back button

   function cleanInnerHtmlForHistory(elt) {
      const className = htmx.config.requestClass
      const clone = /** @type Element */ (elt.cloneNode(true))
      forEach(findAll(clone, '.' + className), function(child) {
        removeClassFromElement(child, className)
      })
      // remove the disabled attribute for any element disabled due to an htmx request
      forEach(findAll(clone, '[data-disabled-by-htmx]'), function(child) {
        child.removeAttribute('disabled')
      })
      // copy nonce back into nonce attribute before history save
      forEach(findAll(clone, 'script'), function(child) {
        child.nonce && child.setAttribute('nonce',child.nonce)
      })
      return clone.innerHTML
    }
ehenighan commented 3 months ago

Hmm, I don't think this can be my problem as I've got history completely disabled for other reasons and still encountered the issue. It's good to know that you've hit similar now-unreplicated behaviour though so I'm not going mad. I don't really need it fixed under normal circumstances now because script-src strict-dynamic takes care of the problem, but I did want to make sure it was resolved for older browsers. Might be that I just need to switch over to inlineScriptNonce as the fallback and forget about it, because for all there's been some noise about that option, it seems to me like it basically just replicates the strict-dynamic behaviour anyway...

ehenighan commented 3 months ago

BTW @MichaelWest22 that extension looks like a really handy option. Will look into it more!

MichaelWest22 commented 3 months ago

Found a better way to handle the history back button issue i was having and released an updated version of my safe-nonce extension now.