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

Fixed #19508 -- Non UTF-8 now remain url encoded. #2919

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@coder9042
Contributor

coder9042 commented Jul 14, 2014

  • When having urls like /test/~%A9, we get 400 when project is deployed as WSGIHandler has fallback for 400 in case of UnicodeDecodeError.
  • In development we get 500 because StaticFilesHandler is used and it does not have that fallback.
  • It cannot be reproduced in tests because the ClientHandler again raises UnicodeDecodeError in get_path_info().
  • The problem arises when in get_path_info(), we decode the url in utf-8.
  • When we pass a url, it passes through unquote() in urllib where it converts all percent encodings, even those which should remain url-encoded.

Its not ready for merge, work still in progress.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 14, 2014

Contributor

@timgraham @loic @aaugustin
There will be some test failures on this PR.

IMO All the above mentioned tests depict the issue which we are fixing here. All give 404 now, so I want to know whether to keep these tests by changing the status codes or remove them completely as now these tests have no meaning or these tests should not be failing.

Contributor

coder9042 commented Jul 14, 2014

@timgraham @loic @aaugustin
There will be some test failures on this PR.

IMO All the above mentioned tests depict the issue which we are fixing here. All give 404 now, so I want to know whether to keep these tests by changing the status codes or remove them completely as now these tests have no meaning or these tests should not be failing.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 14, 2014

Contributor

@timgraham @loic @aaugustin
Any comments on the implementation are welcome.

Contributor

coder9042 commented Jul 14, 2014

@timgraham @loic @aaugustin
Any comments on the implementation are welcome.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 15, 2014

Contributor

Changed the implementation. I have tested on py 2.7 +sqlite, now there are no failures and required behaviour is also achieved.

Contributor

coder9042 commented Jul 15, 2014

Changed the implementation. I have tested on py 2.7 +sqlite, now there are no failures and required behaviour is also achieved.

@loic

View changes

Show outdated Hide outdated tests/handlers/tests.py Outdated
@loic

View changes

Show outdated Hide outdated django/core/handlers/wsgi.py Outdated

@coder9042 coder9042 changed the title from Fixed #19508 to Fixed #19508 -- Converted non utf-8 back to percent encodings. Jul 15, 2014

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

buildbot, test this please.

Contributor

coder9042 commented Jul 16, 2014

buildbot, test this please.

@coder9042

View changes

Show outdated Hide outdated tests/handlers/tests.py Outdated
@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

buildbot, test this please.

Contributor

coder9042 commented Jul 16, 2014

buildbot, test this please.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

@loic
You can test the PR on actual project, the results are now the same on both py2 and 3.
Why it is different in tests on py3, have a look on my inline comment above.
Any idea on how to fix the client to work similarly.

Contributor

coder9042 commented Jul 16, 2014

@loic
You can test the PR on actual project, the results are now the same on both py2 and 3.
Why it is different in tests on py3, have a look on my inline comment above.
Any idea on how to fix the client to work similarly.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

@loic
Finally got it.
In py3, unquote_to_bytes is used instead of unquote. So changed that in client.
Now all tests passing on both py2&3. Also required behaviour also acheived as we now get same results on py2&3 and also both on tests or actual project.

Contributor

coder9042 commented Jul 16, 2014

@loic
Finally got it.
In py3, unquote_to_bytes is used instead of unquote. So changed that in client.
Now all tests passing on both py2&3. Also required behaviour also acheived as we now get same results on py2&3 and also both on tests or actual project.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

Fixed flake8 and added comments.
Will push in the docs update after PR is reviewed.

Contributor

coder9042 commented Jul 16, 2014

Fixed flake8 and added comments.
Will push in the docs update after PR is reviewed.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 16, 2014

Contributor

buildbot, test this please.

Contributor

coder9042 commented Jul 16, 2014

buildbot, test this please.

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 17, 2014

Contributor

@loic
The added tests all pass.
Do we need more tests? I tried to cover several combinations.

Contributor

coder9042 commented Jul 17, 2014

@loic
The added tests all pass.
Do we need more tests? I tried to cover several combinations.

@coder9042 coder9042 changed the title from Fixed #19508 -- Converted non utf-8 back to percent encodings. to Fixed #19508 -- Non UTF-8 now remain url encoded. Jul 17, 2014

@coder9042

This comment has been minimized.

Show comment
Hide comment
@coder9042

coder9042 Jul 19, 2014

Contributor

@loic
I have opened another PR #2932 with a slight different implementation at the environ level and written a uri_to_iri function as per RFC.
Whichever seems better.

Contributor

coder9042 commented Jul 19, 2014

@loic
I have opened another PR #2932 with a slight different implementation at the environ level and written a uri_to_iri function as per RFC.
Whichever seems better.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 14, 2014

Member

Closing in lieu of #2932, I assume that's the preferred on at this point. If not, we can reopen this.

Member

timgraham commented Aug 14, 2014

Closing in lieu of #2932, I assume that's the preferred on at this point. If not, we can reopen this.

@timgraham timgraham closed this Aug 14, 2014

defrex added a commit to defrex/django that referenced this pull request Aug 15, 2014

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