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.

Sign up for GitHub

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

Jump to bottom

Add csp rules for api #1882

Merged
merged 7 commits into from
Jun 24, 2023
Merged

Add csp rules for api #1882

merged 7 commits into from
Jun 24, 2023

Conversation

mingan666
Copy link
Contributor

@mingan666 mingan666 commented Jun 17, 2023 •
edited
Loading

This adds some new CSP rules needed to be able to view the API docs.

@codecov
Copy link

codecov bot commented Jun 17, 2023 •
edited
Loading

Codecov Report

Merging #1882 (245747d) into master (6da4fd0) will decrease coverage by 0.47%.
The diff coverage is 88.88%.

Additional details and impacted files

ildyria
ildyria approved these changes Jun 17, 2023
View reviewed changes
Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

LGTM.

d7415
d7415 reviewed Jun 17, 2023
View reviewed changes
config/secure-headers.php Outdated Show resolved Hide resolved
@d7415
Copy link
Contributor

d7415 commented Jun 17, 2023

Is this only relevant for dev builds? If so, do we really want to add this noise for everyone?

@ildyria
Copy link
Member

ildyria commented Jun 17, 2023 •
edited
Loading

Is this only relevant for dev builds? If so, do we really want to add this noise for everyone?

It is not much noise though.

But a full proper approach would be checking the route and disabling the CSP on the fly (see e.g. for log-viewer).
On this specific case, we are a bit less constrained and can directly white list 2 urls instead of doing a full disable.

@ildyria ildyria added this to the 4.9.3 milestone Jun 20, 2023
@ildyria ildyria requested a review from d7415 June 21, 2023 07:30
@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Jun 21, 2023
@ildyria
Copy link
Member

ildyria commented Jun 21, 2023

Made the pull request cleaner:

  • no more hacky functions
  • use middleware to disable the CSP when necessary
  • enable CSP by default, disable programatically.

@ildyria ildyria requested a review from qwerty287 June 23, 2023 13:15
qwerty287
qwerty287 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

Untested

@ildyria ildyria mentioned this pull request Jun 23, 2023
Open
d7415
d7415 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@d7415 d7415 left a comment

Choose a reason for hiding this comment

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

Only skimmed, but it feels neater than the previous options.

@ildyria ildyria merged commit 055d4e1 into LycheeOrg:master Jun 24, 2023
ildyria added a commit that referenced this pull request Jun 24, 2023
@mingan666 @ildyria @d7415
Add csp rules for api (#1882)
f3ff892 
* disable csp when query for docs/api

---------

Co-authored-by: Benoît Viguier <[email protected]>
Co-authored-by: Martin Stone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ildyria ildyria ildyria approved these changes

@d7415 d7415 d7415 approved these changes

@qwerty287 qwerty287 qwerty287 approved these changes

Assignees
No one assigned
Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Milestone
4.9.3
Development

Successfully merging this pull request may close these issues.
4 participants
@mingan666 @d7415 @ildyria @qwerty287