SAML-Toolkits / python3-saml

MIT License
672 stars 302 forks source link

Handle unicode characters gracefully in python 2 #317

Closed ananduri9 closed 1 year ago

ananduri9 commented 1 year ago

Hello there! We've come across some bugs like the following using this library's add_sign function:

File /opt/virtualenv/monolith/local/lib/python2.7/site-packages/onelogin/saml2/utils.py line 740 in add_sign
elem = OneLogin_Saml2_XML.to_etree(xml)
File /opt/virtualenv/monolith/local/lib/python2.7/site-packages/onelogin/saml2/xml_utils.py line 68 in to_etree
return OneLogin_Saml2_XML._parse_etree(compat.to_bytes(xml), forbid_dtd=True)
File /opt/virtualenv/monolith/local/lib/python2.7/site-packages/onelogin/saml2/compat.py line 42 in to_bytes
return str(data)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xc9' in position 2593: ordinal not in range(128)

It looks like we're getting this error when a user's name or other data has a non-ascii character that goes through this function - and we're still using Python 2 for this.

I believe my fix should allow for gracefully handling unicode characters in Python 2. Let me know if you have any questions/concerns.

akx commented 1 year ago

This is python3-saml – you should use https://github.com/onelogin/python-saml/ for Python 2...

paulsaccount commented 1 year ago

python3-saml is still installable via pip with Python 2 because it isn't enforcing a minimum Python. In addition, this library python3-saml wrote a compatibility file compat.py to handle issues like the one mentioned in the original issue description.

This bug is still valid for users who are using Python 2.7 and the latest version of python3-saml.

If there is a desire to stop supporting Python 2.7 support for this library, you could deprecate support and version the python3-saml so it will not install on Python 2.7 environments. However, shouldn't prevent the fix presented from being added.

alimpon commented 1 year ago

@akx would you be able to merge this fix in for us?

akx commented 1 year ago

@alimpon No, I'm not a maintainer of this project.

alimpon commented 1 year ago

oh sorry. I think @eriktalvi is a maintainer, could you please help merge this in?

alimpon commented 1 year ago

@eriktalvi bump again to merge this please