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

dev/core#1190 Add pptx to safe file types #15047

Merged
merged 1 commit into from
Aug 17, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 15, 2019

Overview

In the last security release we added a whitelist for uploadable file types - this missed some common types. We previously added ics & this add pptx

Before

pptx files cannot be uploaded

After

They are in the whitelist

Technical Details

I feel like it would be better to store these as an array somewhere - maybe as the first step to
a metadata construct for Option groups...

Comments

I guess some more of these will pop up - we can maybe treat any found within 6 months of the change as 'regressions'

https://lab.civicrm.org/dev/core/issues/1190

@civibot
Copy link

civibot bot commented Aug 15, 2019

(Standard links)

@civibot civibot bot added the 5.17 label Aug 15, 2019
@@ -677,6 +677,7 @@ VALUES
(@option_group_id_sfe, 'xlsx', 13, 'xlsx', NULL, 0, 0, 13, NULL, 0, 0, 1, NULL, NULL, NULL),
(@option_group_id_sfe, 'odt', 14, 'odt', NULL, 0, 0, 14, NULL, 0, 0, 1, NULL, NULL, NULL),
(@option_group_id_sfe, 'ics', 15, 'ics', NULL, 0, 0, 15, NULL, 0, 0, 1, NULL, NULL, NULL),
(@option_group_id_sfe, 'pptx', 16, 'pptx', NULL, 0, 0, 16, NULL, 0, 0, 1, NULL, NULL, NULL),
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton i think we would need to run ./bin/regen.sh as we are changing what should be in civicrm_generated.mysql file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seamuslee001 ok - updated

@eileenmcnaughton eileenmcnaughton force-pushed the pptx branch 2 times, most recently from b8200ea to 4743277 Compare August 15, 2019 21:35
I feel like it would be better to store these as an array somewhere - maybe as the first step to
a metadata construct for Option groups...
@seamuslee001
Copy link
Contributor

@pfigel are you ok with this?

@eileenmcnaughton
Copy link
Contributor Author

Note that per the ticket this should be the same risk level as docx - ie. 'std Microsoft risk level' ....

@pfigel
Copy link
Contributor

pfigel commented Aug 17, 2019

Note that per the ticket this should be the same risk level as docx - ie. 'std Microsoft risk level' ....

Agreed.

@seamuslee001
Copy link
Contributor

Merging based on Patrick's comment and the fact we already list docx

@seamuslee001 seamuslee001 merged commit 40a5085 into civicrm:5.17 Aug 17, 2019
@seamuslee001 seamuslee001 deleted the pptx branch August 17, 2019 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants