Add support for attrs and update output format #69

hugorodgerbrown · 2022-11-23T11:17:45Z

This PR is for reference only - as I don't anticipate it being merged. We have forked the repo and will pin to our version as we want to have a drop-in replacement for static that covers all of our use cases.

The event that triggered this change was a critical failure on our CI environment where we do not have all of our static content (as it requires a frontend build step that is not part of the backend test suite). This caused the test suite to fail because we were using the sri_integrity_static tag in templates, which is still called when USE_SRI is False (which it is in testing).

Updating sri_integrity_static to not run when USE_SRI is False doesn't make sense, as if you are calling it explicitly then presumably you do want it to run. 🤔

The root cause of this is that we were building output elements that django-sri doesn't support (defer, async etc.) In addition, the W3C HTML validator didn't like the output:

I started by making a minimal change to the output, but tbh I was bending the existing code too far, and it was becoming overly complex to support the existing use cases along with what we wanted to do.

In this version, we can support 'simple' tag attributes (defer, async etc) as well as 'complex' attributes (type="text/javascript"):

{% load sri %}
<!DOCTYPE html>
<html lang="en"></html>
    <head>
        <title>Test page</title>
        {% sri_static "index.js" 'defer' 'async'%}
        <!-- <script crossorigin="anonymous" integrity="sha256-..." src="/static/index.js" defer async></script> -->
        {% sri_static "index.css"%}
        <!-- <link crossorigin="anonymous" href="/static/index.css" integrity="sha256-..." rel="stylesheet" type="text/css"> -->
    </head>
</html>

RealOrangeOne · 2022-11-23T13:49:19Z

sri/templatetags/sri.py

-def sri_static(path: str, algorithm: Optional[str] = None):
-    algorithm_type = Algorithm(algorithm or DEFAULT_ALGORITHM)
+def sri_static(
+    path: str, *simple_attrs: str, algorithm: str | None = None, **complex_attrs: str


Suggestion: It would be nice to require the algorithm be first before the simple_attrs so the order is more deterministic. Alternatively, forcing algorithm to be a kwarg could also be nice and explicit.

This is an interesting one - when I was looking at it I couldn't think of a scenario where you would use different algorithms within the same project (i.e. SHA-256 for some files, SHA-384 for others). Is this a real use case?

There are three possible forms:

# least favourite as you have to be explicit on *every* use def sri_static(path: str, algorithm: str, *simple_attrs, **complex_attrs) -> str: """Algorithm as required argument.""" # second least favourite - it's neat, but it does mix function kwargs with tag attrs. def sri_static(path: str, *simple_attrs, **complex_attrs) -> str: """Algorithm read out from kwargs.""" algorithm = complex_attrs.pop("algorithm", DEFAULT_ALGORITHM.value) # personal favourite - it's explicit, it doesn't mix with tag attrs, it's optional def sri_static(path: str, *simple_attrs, algorithm: str = DEFAULT_ALOGITHM.value, **complex_attrs) -> str: """Algorithm as named kwarg."""

My thinking was more:

def sri_static(path: str, algorithm: str = DEFAULT_ALOGITHM.value, *simple_attrs, **complex_attrs) -> str:

Calling becomes the same, but I think it prevents {% sri_static "index.js" "defer" algorithm="sha512" %} which looks messy

If you specify algorithm before *args it will always interpret the first unnamed argument as the algorithm, so you have to include it if you have any other unnamed arguments:

>>> def sri_static(path, algo = "sha256", *args, **kwargs): ... print(f"path: {path}\nalgo: {algo}\nargs: {args}\nkwargs: {kwargs}") ...

>>> sri_static("index.js") path: index.js algo: sha256 args: () kwargs: {}

>>> sri_static("index.js", "defer") path: index.js algo: defer args: () kwargs: {}

>>> sri_static("index.js", "defer", algo="sha384") Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: sri_static() got multiple values for argument 'algo' >>>

I guess it comes down to use cases.

Selfishly, we have hundreds of static includes and none of them would require the algorithm to be explicit as we only use the same DEFAULT_ALGORITHM for everything, but we do have a whole smorgasbord of attrs (empty and not empty).

Perhaps something like:

def sri_static(path: str, *simple_attrs, *, algorithm: str = DEFAULT_ALOGITHM.value, **complex_attrs) -> str:

@RealOrangeOne this is what it looks like after the last update?

It does, almost. The main change is the extra *, forcing algorithm to be specified as a keyword argument.

Ah yes - good spot - apologies - updating now.

Turns out you can't do that:

Keyword-only argument separator not allowed after "*" parameter

sri/templatetags/sri.py

RealOrangeOne

I would definitely be happy with merging this, I think it looks great and is a nice generic solution to the problem.

The HTML validation changes are slightly separate to the core problem, as it depends what content you're validating against (HTML vs XHTML for example), but I don't feel very strongly one way or the other, so I think this is fine.

Nit comment, but otherwise looks good.

hugorodgerbrown · 2022-11-23T15:29:42Z

@RealOrangeOne I've updated the names of the args/kwargs to more closely represent what they are (w3c refers to "empty" attributes), removed the extraneous function, and fixed a couple of linting issues.

Fix black issue Fix up import sorting issues Update sri.py Update `sri_static` to ensure that "algorithm" is a kwarg. Revert "Update sri.py" This reverts commit 1b036e3.

hugorodgerbrown force-pushed the master branch 2 times, most recently from d8642c9 to c544330 Compare November 23, 2022 11:51

RealOrangeOne reviewed Nov 23, 2022

View reviewed changes

sri/templatetags/sri.py Outdated Show resolved Hide resolved

RealOrangeOne reviewed Nov 23, 2022

View reviewed changes

hugorodgerbrown force-pushed the master branch from 8b64167 to 1cbfbd0 Compare November 23, 2022 15:26

hugorodgerbrown requested a review from RealOrangeOne November 23, 2022 15:32

hugorodgerbrown added 4 commits December 6, 2022 09:13

Add support for simple and complex attributes

d88206b

Add support for other file types

0f885d6

Update sri_static signature

3ebcf99

Rename args, kwargs as used in the sri functions

17f3d7f

Fix black issue Fix up import sorting issues Update sri.py Update `sri_static` to ensure that "algorithm" is a kwarg. Revert "Update sri.py" This reverts commit 1b036e3.

hugorodgerbrown force-pushed the master branch from cdf7824 to 17f3d7f Compare December 6, 2022 09:15

RealOrangeOne merged commit 4dea73e into RealOrangeOne:master Dec 8, 2022

This was referenced Dec 8, 2022

Omit "type" attribute from script tags #68

Closed

Preload, defer attributes #64

Closed

Support for defer/async/preload/prefetch attributes #65

Closed

Add ico support (for favicon mainly) #67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for attrs and update output format #69

Add support for attrs and update output format #69

hugorodgerbrown commented Nov 23, 2022

RealOrangeOne Nov 23, 2022

hugorodgerbrown Nov 23, 2022

RealOrangeOne Nov 23, 2022 •

edited

Loading

hugorodgerbrown Nov 23, 2022

hugorodgerbrown Nov 23, 2022

RealOrangeOne Nov 27, 2022

hugorodgerbrown Dec 5, 2022

RealOrangeOne Dec 5, 2022

hugorodgerbrown Dec 6, 2022

hugorodgerbrown Dec 6, 2022

RealOrangeOne left a comment

hugorodgerbrown commented Nov 23, 2022

Add support for attrs and update output format #69

Add support for attrs and update output format #69

Conversation

hugorodgerbrown commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealOrangeOne Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealOrangeOne left a comment

Choose a reason for hiding this comment

hugorodgerbrown commented Nov 23, 2022

RealOrangeOne Nov 23, 2022 •

edited

Loading