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

Implement get_file_hash #1044

Merged
merged 2 commits into from
Jun 9, 2020
Merged

Implement get_file_hash #1044

merged 2 commits into from
Jun 9, 2020

Conversation

dancek
Copy link
Contributor

@dancek dancek commented Jun 1, 2020

WARNING: contains a bug.

As mentioned in #519, both the cachebust commit I previously posted and this one look for static files directly in content_path when they're really in static/. I'll fix that tonight if I have time.

EDIT: it's fixed now, in the first commit.


IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

@dancek dancek force-pushed the subresource-integrity branch from 41194a6 to 671eaba Compare June 2, 2020 18:51
@dancek dancek changed the title WIP: Implement get_file_hash Implement get_file_hash Jun 2, 2020
@dancek
Copy link
Contributor Author

dancek commented Jun 2, 2020

This is now two commits, with the first one fixing my previous blunders with cachebust. I erroneously read files from content_path when static files actually are in static_path.

The implementation has different functions compute_file_sha256, compute_file_sha384 and compute_file_sha512 for different hash types, which looks a bit repetitive and dumb.

Another option is to make the calls like compute_file_hash<Sha256>, but that requires a complicated trait bound to the function:

fn compute_file_hash<T>(path: &PathBuf) -> result::Result<String, io::Error>
where
    T: Digest + io::Write,
    <T as Digest>::OutputSize: ops::Add,
    <<T as Digest>::OutputSize as ops::Add>::Output: generic_array::ArrayLength<u8>
{
    let mut file = fs::File::open(path)?;
    let mut hasher = T::new();
    io::copy(&mut file, &mut hasher)?;
    Ok(format!("{:x}", hasher.result()))
}

That would also add a direct dependency on generic_array (though sha2 depends on it anyway). I'll switch to that version if you prefer it. I just thought it's better to err on the side of readability in a FLOSS project.

Also, let me know if you'd rather have a separate PR and possibly a bug report for the first commit.

@dancek
Copy link
Contributor Author

dancek commented Jun 2, 2020

Oh boy, the cache-busting is still broken on generated assets (compiled scss).

@dancek dancek force-pushed the subresource-integrity branch from 671eaba to 7731362 Compare June 3, 2020 15:57
@dancek
Copy link
Contributor Author

dancek commented Jun 3, 2020

Ok, the final try. Both the cachebusting implementation and get_file_hash will search in static_path, output_path and content_path. If none of them works, cachebusting reverts to timestamp and logs a warning to stderr. I realized it's annoying to crash just because cachebusting couldn't be done in the best possible way.

Hope this is good.

The previous implementation looked for static files in the wrong place.
Look in static_path, output_path and content_path. If file can't be
found in any of them, print a warning to stderr and fall back to using
a timestamp.

Add a test to ensure it also works in practice, not just in theory.
@dancek dancek force-pushed the subresource-integrity branch from 7731362 to 52f7bc4 Compare June 3, 2020 21:16
@dancek
Copy link
Contributor Author

dancek commented Jun 3, 2020

Oh, build_timestamp was removed after I branched from next. Let's error out if the file is not found, then.

@dancek
Copy link
Contributor Author

dancek commented Jun 4, 2020

The windows build seems to be failing because the files in test_site have different SHA sums.

      <link href="https://replace-this-with-your-url.com/site.css?h=8661df0fd2f603ff954e7e04b66d014c90c72305855e5c360bff4813186d122b" rel="stylesheet">
...
thread 'can_cachebust_static_files' panicked at 'assertion failed: file_contains!(public, "index.html",
               "<link href=\"https://replace-this-with-your-url.com/site.css?h=83bd983e8899946ee33d0fde18e82b04d7bca1881d10846c769b486640da3de9\" rel=\"stylesheet\">")', components\site\tests\site.rs:692:5

This seems to be because git converts line endings on Windows.

$ unix2dos test_site/static/site.css && sha256sum test_site/static/site.css
unix2dos: converting file test_site/static/site.css to DOS format...
8661df0fd2f603ff954e7e04b66d014c90c72305855e5c360bff4813186d122b  test_site/static/site.css
``

I'll see if this can be fixed with `.gitattributes` later when I have time.

components/templates/src/global_fns/mod.rs Outdated Show resolved Hide resolved
docs/content/documentation/templates/overview.md Outdated Show resolved Hide resolved
@dancek
Copy link
Contributor Author

dancek commented Jun 4, 2020

Thanks for reviewing! Good points, I'll fix these.

@dancek dancek force-pushed the subresource-integrity branch from 52f7bc4 to bdcf143 Compare June 5, 2020 18:46
@dancek
Copy link
Contributor Author

dancek commented Jun 5, 2020

I tried to fix the Windows tests by forcing eol=lf in .gitattributes for the relevant files and fixed the issues from review comments.

@dancek dancek requested a review from Keats June 5, 2020 20:16
@Keats Keats merged commit 6708f76 into getzola:next Jun 9, 2020
@Keats
Copy link
Collaborator

Keats commented Jun 9, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants