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

Investigate if the named resource transformation locks can be made more robust #8370

Open
cmahnke opened this issue Mar 28, 2021 · 15 comments
Open

Comments

@cmahnke
Copy link

cmahnke commented Mar 28, 2021

What version of Hugo are you using (hugo version)?

$ hugo v0.82.0+extended darwin/amd64 BuildDate=unknown

Does this issue reproduce with the latest release?

Yes, Hugo 0.81.0 extended build from GitHub works without any issues

After upgrading Hugo to 0.82.0 the following statement(s) don't work anymore as expected:

{{- $mainJs = slice $main | resources.Concat "js/main.js" | minify | fingerprint -}}
<script src="{{ $mainJs.RelPermalink }}" integrity="{{ $mainJs.Data.Integrity }}"></script>

changing the second to the following (as suggested here for a similar problem) doesn't work either:


<script src="{{ $mainJs.RelPermalink }}" integrity="{{ $mainJs.Data.Integrity | html }}"></script>

After upgrading Chrome and Firefox refuses to load the JS source.

Has there been a change how to use the fingerprinting or might this be a bug?

I haven't been able to investigate the issue any further (yet). I've downgraded back to 0.81.0...

@davidsneighbour
Copy link
Contributor

What does "Chrome and Firefox refuses to load the JS source" mean? There are specific message in both browsers in the console. Please post those here to be sure what "refusing" refers to.

@cmahnke
Copy link
Author

cmahnke commented Mar 28, 2021

Chrome:

Failed to find a valid digest in the 'integrity' attribute for resource 'http:https://localhost:1313/js/main-iiif-i.min.e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855.js' with computed SHA-256 integrity 'V7GxigWQHoUh3pHo53ufoSPL+puqczzyd4PkUu8eufc='. The resource has been blocked.

Firefox (In German):

Keine der "sha256"-Hashes im "integrity"-Attribut stimmen mit dem Inhalt der Subressource überein.

It's basically the same problem, the generated fingerprint doesn't match the resource...

@bep bep added this to the v0.83 milestone Mar 28, 2021
@bep
Copy link
Member

bep commented Mar 28, 2021

So, there are certainly enough sites in the wild that are still working, so fingerprinting isn't broken. What happens in your situation is a little hard to say without additional context, e.g. the site source.

@davidsneighbour
Copy link
Contributor

Where are you hosting? There MUST NOT be any code optimization/minification AFTER you fingerprint the hash. If you host for instance on Netlify you MUST NOT enable minification of CSS and JS on Netlify. That is the most common cause why the hash differs.

@cmahnke
Copy link
Author

cmahnke commented Mar 29, 2021

I host here on GitHub, but it also happens when serving from Hugo (hugo server).

@beb: I've created a (initial) striped down test site here: cmahnke/hugo-0.82-test
Make sure to install node dependencies (yarn i). It can certainly be striped down further for easier analysis...
This throws the described errors in browser when run by 0.82.0 but is fine with 0.81.0.

@jmooring
Copy link
Member

I can reproduce the problem, but I don't understand why you minify, concatenate, and then minify again.

This will fix the problem, but I'm not sure why. Change:

{{- $mainOpts := dict "targetPath" "js/main.js" "defines" $defines "minify" true -}}

To:

{{- $mainOpts := dict "targetPath" "js/main.js" "defines" $defines "minify" false -}}

See https://github.com/cmahnke/hugo-0.82-test/blob/main/layouts/partials/head.html#L48

@anthonyfok
Copy link
Member

Totally a wild guess (so probably a wrong guess):

github.com/tdewolff/minify/v2 was upgraded from v2.9.13 to v2.9.15 between Hugo 0.81.0 and 0.82.0 in order to fix #8332.
Release notes are here at https://github.com/tdewolff/minify/releases

I suppose compiling Hugo 0.82.0 with go.mod pointing to github.com/tdewolff/minify/v2 v2.9.13 may reveal if this issue and the minify v2.9.15 upgrade is related?

@jmooring
Copy link
Member

jmooring commented Mar 29, 2021

Hugo 0.82.0 with go.mod pointing to github.com/tdewolff/minify/v2 v2.9.13 --> PASS
Hugo 0.82.0 with go.mod pointing to github.com/tdewolff/minify/v2 v2.9.14 --> FAIL
Hugo 0.82.0 with go.mod pointing to github.com/tdewolff/minify/v2 v2.9.15 --> FAIL

@anthonyfok
Copy link
Member

Hi @tdewolff! When you have time, could you please help take a look at this? Many thanks!

@jmooring
Copy link
Member

jmooring commented Mar 29, 2021

@anthonyfok and @tdewolff,

I am unable to replicate the test results from my previous comment, so please hold off on investigating tdewolff/minify v2.9.15.

@jmooring
Copy link
Member

jmooring commented Mar 30, 2021

I built Hugo from source at 7ed56c6, tested with cmahnke's repository, no problems.
I then built Hugo from source at the next commit (35dedf1), tested with cmahnke's repository, and the problem appeared.

So the problem appears to be related to the change from tdewolff/minify 2.9.13 to 2.9.15 (we skipped 2.9.14).

But I still don't understand:

  1. Why cmahnke minifies, then minifies again.

  2. How the minified version of the file we fingerprint could be different than the minified file written to disk. For example:

    sha256sum public/js/main.min.e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855.js 
    

    returns

    4d51c8612d759229d3f4dd336f03d5522017bc1711afb1635bb31ae33672b546
    

    instead of

    e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
    

@cmahnke
Copy link
Author

cmahnke commented Mar 30, 2021

@jmooring: The answer to question one is quite simple: Probably it's just a lack of knowledge / wrong assumptions:
I suspected that passing minify to js.Build triggers an algorithm within Esbuild, which might does other things (like dead code path / branch / variable removal, since it works on another level) then it would be possible within the minify function.
So I can certainly remove it from my templates, but my assumption / expectation still is, that it shouldn't matter at all. Thanks for you help!

@jmooring
Copy link
Member

@cmahnke: It looks like esbuild's minify doesn't do anything special. See https://esbuild.github.io/api/#minify, in particular:

The minification algorithm in esbuild does not yet do advanced code optimizations. In particular, the following code optimizations are possible for JavaScript code but are not done by esbuild (not an exhaustive list):

  • Dead-code elimination within function bodies
  • Function inlining
  • Cross-statement constant propagation
  • Object shape modeling
  • Allocation sinking
  • Method devirtualization
  • Symbolic execution
  • JSX expression hoisting
  • TypeScript enum detection and inlining

So until this issue gets further attention, don't ask esbuild to minify.

my assumption / expectation still is, that it shouldn't matter at all.

I agree.

@tdewolff
Copy link

tdewolff commented Apr 2, 2021

Thanks for referring me to the issue. I'm not sure I can help on this one though, this seems to be a problem in Hugo or in the setup more than a problem of minify, or am I mistaken? Keep me updated if this requires changes to minify!

@bep
Copy link
Member

bep commented Apr 19, 2021

So, this construct looks funky:

https://github.com/cmahnke/hugo-0.82-test/blob/main/layouts/partials/head.html#L38

I'll paste in the entire construct:

{{- $defines := dict "process.env.NODE_ENV" `"production"` -}}
{{- $mainOpts := dict "targetPath" "js/main.js" "defines" $defines "minify" true -}}
{{- $main := resources.Get "js/main.js" | js.Build $mainOpts -}}

{{- $main = slice $main | resources.Concat "js/main.js" | minify | fingerprint -}}

I will keep this issue open to see if we can improve/fix this (I suspect the caveat is the reuse of js/main.js as the target path for the two pipes above), but there are several issues with the above constructs that is just superflous and fixing those would get rid of the error. This should work:

{{- $defines := dict "process.env.NODE_ENV" `"production"` -}}
{{- $mainOpts := dict  "defines" $defines "minify" true -}}
{{- $main := resources.Get "js/main.js" | js.Build $mainOpts | fingerprint -}}

@bep bep changed the title Fingerprinting broken with minify in 0.82.0? Investigate if the named resource transformation locks can be made more robust Apr 19, 2021
@bep bep modified the milestones: v0.83, v0.84 May 1, 2021
@bep bep modified the milestones: v0.84, v0.85 Jun 18, 2021
@bep bep modified the milestones: v0.85, v0.86 Jul 5, 2021
@bep bep modified the milestones: v0.86, v0.87, v0.88 Jul 26, 2021
@bep bep added this to the v0.115.0 milestone Jun 13, 2023
@bep bep modified the milestones: v0.115.0, v0.116.0 Jun 30, 2023
@bep bep modified the milestones: v0.116.0, v0.117.0 Aug 1, 2023
@bep bep modified the milestones: v0.117.0, v0.118.0 Aug 30, 2023
@bep bep modified the milestones: v0.118.0, v0.119.0 Sep 15, 2023
@bep bep modified the milestones: v0.119.0, v0.120.0 Oct 4, 2023
@bep bep modified the milestones: v0.120.0, v0.121.0 Oct 31, 2023
@bep bep modified the milestones: v0.121.0, v0.122.0 Dec 6, 2023
@bep bep modified the milestones: v0.122.0, v0.123.0, v0.124.0 Jan 27, 2024
@bep bep modified the milestones: v0.124.0, v0.125.0 Mar 4, 2024
@bep bep modified the milestones: v0.125.0, v0.126.0 Apr 23, 2024
@bep bep modified the milestones: v0.126.0, v0.127.0 May 15, 2024
@bep bep modified the milestones: v0.127.0, v0.128.0 Jun 8, 2024
@bep bep modified the milestones: v0.128.0, v0.129.0 Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants