Skip to content

Conversation

g-k
Copy link
Contributor

@g-k g-k commented Jul 29, 2021

refs: #36

Making a WIP PR, so I can start looking at it without adding Dylan's fork as a remote.

Copy link
Contributor

@DylanYoung DylanYoung left a comment

Choose a reason for hiding this comment

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

Notes for finishing up.

csp/utils.py Outdated
)
# Technically we're modifying Django settings here,
# but it shouldn't matter since the end result of either will be the same
deprecation._handle_legacy_settings(custom_definitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Design Decision: Should we call _handle_legacy_settings based on whether the user has set CSP_POLICY_DEFINITIONS? i.e. only use the legacy settings if the new settings aren't used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably best, but it require updating a whack-ton of tests, so maybe it could come in a separate PR.

headers[header].append(csp)

for header, policies in headers.items():
# TODO: Design Decision do we want the optional whitespace?
Copy link
Contributor

Choose a reason for hiding this comment

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

Design Decision: do we want the optional whitespace? (See convo on issue)

return response

response[header] = self.build_policy(request, response)
# These won't be ordered properly on Cpython 3.5 or below
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove October 2020 comments and OrderedDict (not supporting python 3.5 anymore?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Done


def csp_update(**kwargs):
update = dict((k.lower().replace('_', '-'), v) for k, v in kwargs.items())
def csp_reorder(order):
Copy link
Contributor

Choose a reason for hiding this comment

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

Big thing left to do is clarify how the decorators should work and write tests for them.

setup.cfg Outdated

[tool:pytest]
addopts = -vs --tb=short --pep8 --flakes
addopts = -vs --tb=short --pep8 --flakes --ignore docs/conf.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what I was doing here: addopts = -vs --tb=short --pep8 --flakes --ignore docs/conf.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Update docs

csp/utils.py Outdated
if not isinstance(v, (list, tuple)):
v = (v,)
csp[k] = v
# TODO: design decision
Copy link
Contributor

Choose a reason for hiding this comment

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

Design Decision: Do we need this append functionality or should we just error out if the policy we're updating doesn't exist? I guess the issue is handling legacy calls?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've simplified this significantly. csp_update and csp_replace no longer support appending. I added a new csp_append decorator, but not sure if it's desirable.

Design Decision: csp_update and csp_append using the old capitalized kwarg style only apply to the "default" policy by default, do we want them to apply to all policies? I think not because it could be a surprise that bites someone.

rf = RequestFactory()


def get_headers(response):
Copy link
Contributor

Choose a reason for hiding this comment

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

Move get_headers to csp/tests/utils.py


``CSP_POLICIES``
A list or tuple specifying which definitions will be applied by default and
defining an order on those policies. *['default']*
Copy link
Contributor

Choose a reason for hiding this comment

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

Order doesn't really matter I think, remove from docs on CSP_POLICIES

@robhudson
Copy link
Collaborator

Closing old PR. #219 has been merged which fixes this same issue. Thank you!

@robhudson robhudson closed this Jun 6, 2024
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.

3 participants