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

Handle unicode object ids. #181

Merged
merged 6 commits into from May 16, 2018

Conversation

Projects
None yet
4 participants
@sallner
Copy link
Contributor

sallner commented Sep 15, 2017

This solves #166.

sallner added some commits Sep 15, 2017

Allow unicode in object ids.
We only want to restrict the use of non-printable characters in object ids. General unicode characters are fine.
Handle unicodes in url path.
As the WSGI standard defines paths as latin-1 encoded strings we have to deal with it here.

@sallner sallner requested review from hannosch , icemac and tseaver Sep 15, 2017

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Sep 15, 2017

@tseaver There was this test.

def test_badid_XSS(self):
e = self.assertBadRequest('<abc>&def')
self.assertEqual(str(e),
'The id "&lt;abc&gt;&amp;def" contains characters '
'illegal in URLs.')

Do you remember why this is bad in the object id? This should be quoted by urllib anyway, shouldn't it?

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Sep 18, 2017

Actually, when using the angle brackets in an object id, it currently fails to do a rename with this patch. I have to look into a that again and produce a failing test first. But it would be still nice to know the reason behind the explicit declaration as illegal. In that case, I would not need to find the reason for the failure.

@icemac

icemac approved these changes Nov 30, 2017

Copy link
Member

icemac left a comment

LGTM. It would be nicer to have separate test methods for Python 2 resp. Python 3 which are only executed on the matching Python version (via @unittest.skipIf()). I think this easier to read than the currently used if blocks.

@icemac icemac added this to the 4.0b3 milestone Nov 30, 2017

@icemac icemac modified the milestones: 4.0b3, 4.0b4 Feb 2, 2018

@icemac icemac modified the milestones: 4.0b4, 4.0b5 Apr 23, 2018

sallner added some commits May 16, 2018

Do not allow angle brackets and ampersand.
These result in an id as  TaintedString which is not supported by `hasattr`.
@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented May 16, 2018

@icemac I added the requested changes and dropped the attempt to allow <, > and &, as they caused an error during a hasattr call. The id was present as TaintedString in this case.

else:
# Does not raise
self._callFUT(self._makeContainer(), u'abc☃')
@unittest.skipIf(PY3, 'Python 3 cannot handle bytestrings here.')

This comment has been minimized.

@ale-rt

ale-rt May 16, 2018

Member

A minor thing I also have been warned about... it is better to use "not PY2" which is more future proof, and we know we have a long future :)

This comment has been minimized.

@icemac

icemac May 16, 2018

Member

+1 maybe @unittest.skipUnless(PY2 ...could be used alternatively to prevent to use the not.

This comment has been minimized.

@sallner

sallner May 16, 2018

Author Contributor

I changed this to skipUnless. Thanks.

@icemac

icemac approved these changes May 16, 2018

Copy link
Member

icemac left a comment

LGTM.

else:
# Does not raise
self._callFUT(self._makeContainer(), u'abc☃')
@unittest.skipIf(PY3, 'Python 3 cannot handle bytestrings here.')

This comment has been minimized.

@icemac

icemac May 16, 2018

Member

+1 maybe @unittest.skipUnless(PY2 ...could be used alternatively to prevent to use the not.

@sallner sallner merged commit a24a2a4 into master May 16, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 79.094%
Details

@sallner sallner deleted the unicode-object-id branch May 16, 2018

@icemac

This comment has been minimized.

Copy link
Member

icemac commented May 16, 2018

@sallner Could you please add a change log entry on master for this PR containing a link to the PR?

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented May 16, 2018

@icemac Done.

@sallner sallner referenced this pull request May 16, 2018

Closed

Support Unicode URIs #166

pbauer added a commit to plone/Products.CMFPlone that referenced this pull request May 30, 2018

pbauer added a commit to plone/Products.CMFPlone that referenced this pull request May 30, 2018

mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request May 30, 2018

[fc] Repository: Products.CMFPlone
Branch: refs/heads/master
Date: 2018-05-30T16:49:52+02:00
Author: Philip Bauer (pbauer) <bauer@starzel.de>
Commit: plone/Products.CMFPlone@2df112f

When allowing unicode ids in py3 Zope changed the regex for allowed ids

See zopefoundation/Zope#181

Files changed:
M CHANGES.rst
M Products/CMFPlone/tests/testCheckId.py

icemac added a commit that referenced this pull request Oct 18, 2018

Fix handling of non-ASCII characters in URLs.
On Python 2 everything is fine only on Python 3 the recoding has to be done.

This regression was introduced in #181.

icemac added a commit that referenced this pull request Oct 19, 2018

Fix handling of non-ASCII characters in URLs. (#380)
On Python 2 everything is fine, no recoding is necessary.
Only on Python 3 the recoding has to be done.

This regression was introduced in #181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.