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

refactor(middleware): Refactor internals of CSPMiddleware so that it's easier to extend existing logic without copy/pasting it into subclass #237

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

crbunney
Copy link
Contributor

The current implementation of csp.middleware.CSPMiddleware doesn't lend itself to extending the functionality of it or the csp.contrib.rate_limiting.RateLimitedCSPMiddleware without copy/pasting the implementation of .build_policy() and .build_policy_ro()

This is a bit of an issue, because it means any updates to the base implementation of the superclass has to be manually copied into any subclasses.

For example, for my multi-tenant webapp project, we use the RateLimitedCSPMiddleware and have an additional requirement to generate a CSP that contains some per-tenant configuration. So I've created a custom middleware by copying the code from RateLimitedCSPMiddleware and adding my extra logic in.

This isn't great because if the implementation of RateLimitedCSPMiddleware changes, for example to fix this issue #231, then I have to remember to check for changes and manually copy them into my middleware.

So, I'm proposing a change to how CSPMiddleware works to provide a better hook into the existing implementation and allow subclasses to modify things before the policy string is built.

As you can see from the diff, this reduces the lines of code required to create a CSPMiddleware subclass, even more so when extending the RateLimitedCSPMiddleware.

The build_policy and build_policy_ro methods are essentially redundant with this PR, but I wasn't too sure about removing outright, since it would a breaking change, so would like the maintainers' input on that.

Finally, I haven't attempted to address any of the known issues already raised in other issues/PRs

@robhudson
Copy link
Member

I like where this is going. I have also noticed the copy/paste needed to extend the middleware. Thanks for submitting a patch.

I would support leaving build_policy and build_policy_ro for now, as you've done, and full removal in 4.1.0. But I would also like some documentation added to the migration docs on this change.

csp/contrib/rate_limiting.py Outdated Show resolved Hide resolved
csp/middleware.py Outdated Show resolved Hide resolved
csp/middleware.py Outdated Show resolved Hide resolved
csp/middleware.py Outdated Show resolved Hide resolved
csp/middleware.py Outdated Show resolved Hide resolved
@crbunney crbunney force-pushed the easier_middleware_subclassing branch 2 times, most recently from c5237c1 to 2f2d1ae Compare July 11, 2024 10:54
…s easier to extend existing logic without copy/pasting it into subclass
@crbunney crbunney force-pushed the easier_middleware_subclassing branch from 2f2d1ae to 20b8683 Compare July 15, 2024 11:27
@crbunney
Copy link
Contributor Author

I would also like some documentation added to the migration docs on this change.

I've added a section to the migration guide now

Copy link
Member

@robhudson robhudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you for working through the requested changes.

@robhudson robhudson merged commit fe19cdb into mozilla:main Jul 15, 2024
9 checks passed
@crbunney crbunney deleted the easier_middleware_subclassing branch July 16, 2024 11:25
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