Skip to content

Conversation

@mrvanes
Copy link
Contributor

@mrvanes mrvanes commented Feb 5, 2019

This fixes an unescaped user input vulnerability

This fixes an unescaped user input vulnerability
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 81.531% when pulling 4d8133a on mrvanes:patch-1 into 2cb735c on IdentityPython:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 81.531% when pulling 4d8133a on mrvanes:patch-1 into 2cb735c on IdentityPython:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 81.531% when pulling 4d8133a on mrvanes:patch-1 into 2cb735c on IdentityPython:master.

from .i18n import language
from . import samlmd
import six
from cgi import escape
Copy link
Member

@c00kiemon5ter c00kiemon5ter Feb 5, 2019

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/cgi.html#cgi.escape

Deprecated since version 3.2: This function is unsafe because quote is false by default, and therefore deprecated. Use html.escape() instead.

Maybe, we should use just html.escape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but it is only available from 3.2 and pyFF is still in 2.7 land...

Copy link
Contributor

@leifj leifj Feb 5, 2019

Choose a reason for hiding this comment

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

pyFF is 3.x and 2.x - all builds are built on 2.7, 3.5, 3.6, 3.7 - is there a six move we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in master now, but our deploy is still a 2.7 venv. I expect many others are, due to legacy. Do you want to break all those installations without warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry. I took your comment to mean that pyFF didn't do 3.x but you meant that pyFF still needs to maintain backwards compat with 2.x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know six-fu, please consider this PR a best-effort favor to the community...

Copy link
Member

Choose a reason for hiding this comment

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

import six
if six.PY2:
    from cgi import escape
else:
    from html import escape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is this compatible with the quote=True named parameter in the call later for html.escape?

Copy link
Member

Choose a reason for hiding this comment

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

yes, both interfaces are the same. The difference is that the default value for the quote parameter is False for cgi.escpae and True for html.escape. Setting the quote parameter yourself works for both modules ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine merging this and fixing the six move myself

@leifj
Copy link
Contributor

leifj commented Feb 5, 2019 via email

@leifj
Copy link
Contributor

leifj commented Feb 5, 2019

Kudos to Tirtha Mandal mandaltirtha17@gmail.com and Jonas Lejon jonas@triop.se who independently discovered this. Closes #160

@leifj leifj closed this Feb 5, 2019
@leifj leifj reopened this Feb 5, 2019
@leifj leifj merged commit b58a594 into IdentityPython:master Feb 5, 2019
@mrvanes mrvanes deleted the patch-1 branch April 12, 2019 09:19
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.

4 participants