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

Fix: add to CSP_FRAME_SRC #2446

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Fix: add to CSP_FRAME_SRC #2446

merged 1 commit into from
Oct 10, 2024

Conversation

angela-tran
Copy link
Member

Closes #2445

#2445 revealed that in production, the frame-src directive only includes google.com and therefore blocks the transit processor iframe.

This is because in settings.py, the logic was to throw away the originally assigned value of CSP_FRAME_SRC and instead use whatever is in env_frame_src.

CSP_FRAME_SRC = ["*.littlepay.com"]
env_frame_src = _filter_empty(os.environ.get("DJANGO_CSP_FRAME_SRC", "").split(","))
if RECAPTCHA_ENABLED:
env_frame_src.append("https://www.google.com")
if len(env_frame_src) > 0:
CSP_FRAME_SRC = env_frame_src

This logic was fine when the originally assigned value was 'none' since the directive only allows 'none' or a space separated list of values, but in a23b51e2, we changed it to be *.littlepay.com since we know we will always need to allow that as an iframe source.

This PR fixes it so we add to CSP_FRAME_SRC instead of re-assigning it.

it made sense to re-assign it when the default value was 'none', but
now we will always have at least *.littlepay.com
@angela-tran angela-tran added the bug Something isn't working label Oct 9, 2024
@angela-tran angela-tran self-assigned this Oct 9, 2024
@angela-tran angela-tran requested a review from a team as a code owner October 9, 2024 23:18
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. labels Oct 9, 2024
Copy link

github-actions bot commented Oct 9, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py 315
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Member

@lalver1 lalver1 left a comment

Choose a reason for hiding this comment

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

This looks good 👍
I saw that before this PR, *.littlepay.com only appeared in script-src in the CSP header but now it also appears in frame-src.

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Yep, looks good 👍

Now that we're initializing the frame src with the Littlepay domain, no need to replace.

@angela-tran angela-tran merged commit 158e1b0 into main Oct 10, 2024
18 checks passed
@angela-tran angela-tran deleted the fix/csp-frame-src branch October 10, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. bug Something isn't working deployment-dev [auto] Changes that will trigger a deploy if merged to dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocked 'frame-src' from 'verify.littlepay.com'
3 participants