Skip to content

Minor SAML/Django-CSP fixes#129

Merged
tymees merged 4 commits intodevelopfrom
fix/saml-csp
Feb 27, 2026
Merged

Minor SAML/Django-CSP fixes#129
tymees merged 4 commits intodevelopfrom
fix/saml-csp

Conversation

@tymees
Copy link
Member

@tymees tymees commented Jul 23, 2025

As promised, I looked into the possible Django-CSP and DjangoSAML2 issues. Turns out, nothing is broken!

So this PR is limited to a few minor changes related to all this:

  • Updated the dev-project to use Django CSP 4
  • Fixed an issue in DjangoSAML2 in which it could forget it's session locally
  • Updated the requirements to explicitly require a DjangoSAML2 version compatible with Django-CSP 4

Comment on lines +249 to +251
if not SESSION_COOKIE_SECURE:
# Needed on dev
SAML_SESSION_COOKIE_SAMESITE = 'Lax'
Copy link
Member Author

Choose a reason for hiding this comment

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

So this is the fix about forgetting sessions I talked about.

If this is not here, the SAML library doesn't set it's session cookie in local dev environments. (Because by default this requires the session-cookie secure flag to be set).

I would recommend apps to set something similar

@tymees tymees requested a review from miggol July 23, 2025 10:32
@tymees
Copy link
Member Author

tymees commented Jul 23, 2025

Oh, btw, the new version of DjangoSAML2 is compatible with Django CSP < 4 as well. So the mandatory upgrade should not break apps with a < 4 constraint

Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Requirements borked as usual

Otherwise looks good to me! Sensible defaults

build==1.2.2.post1
# via pip-tools
certifi==2025.1.31
cdh-django-core[all,core,dev,docs,federated-auth,files,mail,recommended,rest,vue] @ file:///home/ty/projects/web/django-shared-core
Copy link
Contributor

Choose a reason for hiding this comment

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

--find-links file:///../. cdh-django-core[all,core,dev,docs,federated-auth,files,mail,recommended,rest,vue]

Works for me here. But it's still that annoying there's no way to get this generated from requirements.in

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the offending line; it isn't required anyways...

The problem with the absolute path seems to stem from some internal pip logic. I know poetry can actually deal with this, but switching to that might be a bit extreme...

@tymees tymees merged commit 699ee41 into develop Feb 27, 2026
1 of 3 checks passed
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