-
-
Notifications
You must be signed in to change notification settings - Fork 525
Django 6.0: Add stubs for built-in CSP support #2931
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
Conversation
|
Apparently the stubtest job needs the Django 6.0 dependency update to be merged first. |
9666acd to
295a04f
Compare
|
|
Please look at stubtest output https://github.com/typeddjango/django-stubs/actions/runs/20098667950/job/57663468906?pr=2931 And update stubs or stubtest allowlist files |
8a5d299 to
79dac0f
Compare
79dac0f to
20f0bb7
Compare
|
Should I include undocumented functions like |
|
Yes, add them to stubs. If you know they are undocumented, it's worth adding a code comment about it. |
| SECURE_CSP: dict[str, Any] = {} | ||
| SECURE_CSP_REPORT_ONLY: dict[str, Any] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SECURE_CSP: dict[str, Any] = {} | |
| SECURE_CSP_REPORT_ONLY: dict[str, Any] = {} | |
| SECURE_CSP: dict[str, Any] | |
| SECURE_CSP_REPORT_ONLY: dict[str, Any] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might also type it as Sequence[str] instead of Any maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently SECURE_CSP_REPORT_ONLY = {"report-uri": "/path/to/reports-endpoint/"} is a valid value, so it's not always Sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would Sequence[str] | str cover all use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. Check the docs and if that is inconclusive, maybe check source too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs:
The setting must be a mapping (typically a dictionary) of directive names to their values. Each key should be a valid CSP directive such as default-src or script-src. The corresponding value can be a list, tuple, or set of source expressions or URLs to allow for that directive. If a set is used, it will be automatically sorted to ensure consistent output in the generated headers.
Based on this, I updated the type for the policies to be Mapping[str, Collection[str] | str]. Let me know if that works.
intgr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
abf2201 to
1bbb8a4
Compare
a659ed1 to
388cf6a
Compare
388cf6a to
47e021e
Compare
|
I've updated the PR based on the review comments. All build jobs pass now. |
intgr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for your help.
Documentation: https://docs.djangoproject.com/en/6.0/ref/csp/
I reused a trick from
django-stubs/db/models/enums.pyito make it compatible with Python < 3.11.