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

Support clearing/unsetting directives via decorator #201

Open
tim-schilling opened this issue Dec 21, 2023 · 7 comments
Open

Support clearing/unsetting directives via decorator #201

tim-schilling opened this issue Dec 21, 2023 · 7 comments

Comments

@tim-schilling
Copy link
Contributor

Our application is served within an iframe. All but the initial authentication views need to have a frame-ancestors keyed to the tenants domain. We use a separate middleware to interact with the _csp_update API. To poke a hole for these authentication views, I've created a csp_clear decorator that clears the frame_ancestors directive.

I'm unsure if this has utility outside of our particular need. I'll be posting the PR shortly for you to review and decide if it's helpful or not.

tim-schilling added a commit to tim-schilling/django-csp that referenced this issue Dec 21, 2023
This allows a directive to be cleared / unset entirely for a
particular view.

Closes mozilla#201
@robhudson
Copy link
Member

I see that @csp_replace(FRAME_ANCESTORS=None) would not work since there's an if v is not None check there. I'd be curious if changing that to allow None values and then popping them would make sense?

In other words, we could add a new decorator, or use an existing decorator for the same purpose. I'd be curious of which seems clearer.

@tim-schilling
Copy link
Contributor Author

Alternatively, we could define a sentinel CLEAR = object() and use that instead of None.

@robhudson
Copy link
Member

If we like the idea of reusing the decorators we have, I like the sentinel idea. It makes it a bit more clear and intentional.

@tim-schilling
Copy link
Contributor Author

I'm not sure if you're asking me Rob, but I like the approach of a sentinel with csp_replace rather than a new decorator.

@robhudson
Copy link
Member

I'm looking at this again now that the big refactor is merged.

By the way, I was wrong in my comment above, this worked before my refactor, it was just confusing with the None check which stops it from being added to the dict of directives, but by not being added it effectively clears it. There's a test for it as well.

In the PR you shared a test that has both a _csp_update and _csp_clear attribute, suggesting that maybe this view is already decorated and you're needing to clear a directive after it has already been decorated? Or was that test just showing that the clear takes precedence?

Given that @csp_replace with a None value will clear the directive, I'm not sure of the benefit of another decorator or sentinel to explicitly do this. But if this doesn't fit your need I'd be interested in hearing more.

@tim-schilling
Copy link
Contributor Author

In the PR you shared a test that has both a _csp_update and _csp_clear attribute, suggesting that maybe this view is already decorated and you're needing to clear a directive after it has already been decorated? Or was that test just showing that the clear takes precedence?

We have a case where the middleware was applying a policy via _csp_update but then a view needed to remove it in an exception flow. I wanted to cover something similar in the upstream PR.

Given that @csp_replace with a None value will clear the directive, I'm not sure of the benefit of another decorator or sentinel to explicitly do this. But if this doesn't fit your need I'd be interested in hearing more.

Since update is applied after replace in build_policy, it doesn't act as a true clear directive when there are multiple operations.

@tim-schilling
Copy link
Contributor Author

I'm good with closing this issue too as a "Won't fix for now". If there are others out there that could benefit from it, they can upvote it. It may not be worth adding to your maintenance workload.

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 a pull request may close this issue.

2 participants