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

Added security checks for str.format. [5.0] #1912

Merged
merged 1 commit into from
Jan 30, 2017
Merged

Added security checks for str.format. [5.0] #1912

merged 1 commit into from
Jan 30, 2017

Conversation

mauritsvanrees
Copy link
Member

@mauritsvanrees mauritsvanrees commented Jan 18, 2017 •
edited
Loading

Part of PloneHotfix20170117.

@mauritsvanrees mauritsvanrees changed the title Added security checks for str.format. Part of PloneHotfix20170117. [5.0] Added security checks for str.format. [5.0] Jan 18, 2017
@mauritsvanrees
Copy link
Member Author

@vangheem This merges the hotfix into 5.0.

@mauritsvanrees
Copy link
Member Author

Note that this adds the formatting function and classes from the hotfix into utils.py. So third party code might start relying on this. If we do not want this, then we had better change that now, before merging.

@jensens
Copy link
Member

jensens commented Jan 26, 2017

Well, utils.py is probably the right location. if we want to indicate this a thing not to use outside we could prefix all with _ - but I am not sure if this is needed/ what we want.

@jensens
Copy link
Member

jensens commented Jan 30, 2017

Since nobody has an opposite opinion on this I will merge it.

@jensens jensens merged commit ae77d3a into 5.0.x Jan 30, 2017
@mauritsvanrees mauritsvanrees deleted the apply-plonehotfix-20170717-50 branch February 1, 2017 13:31
This was referenced Feb 1, 2017
Merged
Merged
thet
thet reviewed Feb 7, 2017
View reviewed changes
Copy link
Member

@thet thet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@mauritsvanrees - since I'm not totally sure about the intentions of your PR I ask you to correct the wrong import in mixins.py.

Products/CMFPlone/resources/browser/mixins.py
@@ -67,9 +68,10 @@ def __call__(self):
less_vars_params[name] = value

for name, value in registry.items():
t = value.format(**less_vars_params)
t = SafeFormatter(value).safe_format(**less_vars_params)
Copy link
Member

Choose a reason for hiding this comment

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

@mauritsvanrees you have imported safe_format but using SafeFormatter. This leads to an error mentioned here: #1938 (comment)

Products/CMFPlone/resources/browser/mixins.py
@@ -115,7 +117,7 @@ def __call__(self):
less_vars_params[name] = value

for name, value in registry.items():
t = value.format(**less_vars_params)
t = SafeFormatter(value).safe_format(**less_vars_params)
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@mauritsvanrees
Copy link
Member Author

Huh, strange that this is not caught by the tests.
I have added pull request #1944 which fixes the import and hopefully makes things a bit clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thet thet thet left review comments

Assignees
No one assigned
Labels
01 type: bug 24 status: ready 32 needs: review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mauritsvanrees @jensens @thet