django-commons / django-debug-toolbar

A configurable set of panels that display various debug information about the current request/response.
https://django-debug-toolbar.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8.08k stars 1.05k forks source link

Quick hack for including csp_nonces from requests into script tags #1975

Closed karolyi closed 2 months ago

karolyi commented 2 months ago

This should fix #1723 as it optionally includes the csp_nonce from the django-csp module.

matthiask commented 2 months ago

Thanks. Looks good. I'd like to have some tests for this; django-csp could be made a dependency for the testsuite so that we can actually verify that the code works as intended.

karolyi commented 2 months ago

What bothers me more is that normally, link/style tags would require the nonces too. But since at its current state, 'strict-dynamic'-enabled script-src scripts can't inject styles which a lot of frameworks do), everyone just enables inlining styles and sameorigins in practice.

If this ever changes and someone enables 'strict-dynamic' for style-src, that will override 'self' on style CSPs and every link[rel=stylesheet]/style tag will require a nonce too from that point on.

I might add that preventative measure. The tests can be organized.

matthiask commented 2 months ago

Sure! Feel free to include that.

karolyi commented 2 months ago

So, tests are there, all working.

I'm totally against the enforcing of opinionated formatting on someone else's code (it's like no one is allowed to have their own coding style), but so be it. I can just as much format the code back automatically when doing free and voluntary contributions to other projects.

In fact, I timed the dict() vs {} call with timeit and the latter seemed faster, so as per empirical evidence, I went with that.

Please merge and do a timely release.

matthiask commented 2 months ago

Thanks for your contribution!

We're using code formatters so that we do not have to discuss code style over and over, as do many other projects. Also, we're not being paid for the work we're doing on this project either, free and voluntary here too.

The PR looks good to me at first sight, thanks. I'll take a closer look probably tomorrow.

tim-schilling commented 2 months ago

@matthiask I'm unsure about the type definition changes here. If you're good with them, I'll let it be.

My specific concern is that we don't have a formal policy for them and I'm not sure how this current state improves things. I'd prefer to be consistent in the library so it's easier for folks to navigate around.

matthiask commented 2 months ago

@tim-schilling My general opinion is that type annotations can be noisy, and not really worth it unless a big part of the code base is typed, but we have already started moving on this https://github.com/jazzband/django-debug-toolbar/issues/1705 so I'm neutral to slightly fine with it.

karolyi commented 2 months ago

For context, I'm using pyright and with these added typings, my test and everything I did, checks out perfectly.

Don't be such an enemy of typing that will point out errors early with static analysis.

tim-schilling commented 2 months ago

But the project doesn't use pyright, so it's possible for this to get out of date and cause confusion down the line.

karolyi commented 2 months ago

You are confusing python typing and static analysis with pyright. The former will be valid even when the latter changes, as it's defined in python itself, and not the module.

There is only one comment in my code related to pyright, since I didn't want to go and add a missinfg type annotation in Django itself. The rest is standard python typing stuff.

tim-schilling commented 2 months ago

Sorry, I shouldn't have even started us down that discussion. Thank you for the PR, this will be great to have.

tim-schilling commented 2 months ago

@matthiask when you have time again, take a look at the revised PR. I'm now good with merging this.

karolyi commented 2 months ago

Whoa, ruff was pretty bold here removing the HttpRequest import :D Hence why everything's failing.

matthiask commented 2 months ago

Thanks!

karolyi commented 2 months ago

@matthiask you're welcome.

Can I have a release please, so I could use the actual official package, instead of my fork?

matthiask commented 2 months ago

@karolyi We're currently in the middle of a GSoC project adding async support to the toolbar, so I'm not sure if we want to do a release in the next few days without some additional testing first.

I generally think releasing quickly and trusting the test suite is the way to go, but I'm a bit wary now because of this larger project.

karolyi commented 2 months ago

I see thanks. I've already set a notification on the releases of this project, so I'll be notified if there is a new one.

Until then, I'll keep using my fork.