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
cookielib doesn't handle URLs with / in parameters #47954
Comments
cookielib doesn't handle URLs like "http://server/script? My suggested fix (attached, line 1465-1468) is to first strip off |
attached is a patch with the suggested fix along with a unit test. |
The patch and tests look fine to me, Gregory. I verified it against the |
yep it applies to all releases. anyways, it won't make 2.6/3.0 but it |
Do we have an RFC 3986 URI parser in the stdlib now? It would be better I'll try and add a patch that does that and check what Firefox does to |
John, bpo-3647 tries relative url parsing and joins to be RFC3986 |
I can confirm that the patch applies cleanly to the release26-maint However, the entire approach seems wrong to me: the patched method A better fix would be either to call urlparse.urlparse directly, or Attaching a patch which implements the latter strategy. Greg's new |
It looks to me that it's just request_path that's wrong, so no need to add extra arguments to that function. It should discard the query and fragment (still keeping the "parameters" -- using urlparse.urlsplit instead of urlparse.urlparse would make that simpler). request_path is only called in three places:
Finally, even RFC 2965, which nobody cares about, and which does include the path check on setting the cookie, refers to RFC 2396 for the definition of request-URI, and both RFC 2396 and RFC 3986, which obsoletes it, agree that the path doesn't include the query (nor the fragment). Incidentally: the request_path function docstring claims to return the request-URI, but obviously the docstring should say it returns the path component of the request-URI. |
As long as we don't care about preserving backward compatibility, we |
I'll upload a patch when I'm back home (bugs.python.org went down yesterday). Will turn docstring into comment. |
Just re-read your comment, Tres. Since when do docstrings determine whether a stdlib function is public? If it's documented in the docs, it's public. If not, it's not. This function isn't, so it's not public. It's also not in __all__, FWLTW. |
Didn't bother changing docstring to comment, since that would be inconsistent with rest of module. |
FWIW, the "certain semantics" that request_path "promises" are 1. that it returns the RFC 2965 request-URI (which has never been true -- it returns the path component of the request-URI instead) and 2. that that request-URI is as defined in RFC 2965, and this bug is about fixing the function so that that's true for the case where the URI has a query component. So there's absolutely no reason for not changing the function. |
There is a reason, and that is that it may break existing code in the field relying on the current behavior. This is (unfortunately) true regardless of whether the function is public or private, though the fact that it is ostensibly private is likely to reduce the amount of breakage. The fix needs to be evaluated to try to guess whether breakage is likely, and if it is, it would not be a candidate for backport to 2.6 or 3.1. |
Hmm. I didn't read your comment carefully enough before I replied. I think you are saying that the bug fix is confined to the routine in question and doesn't change even its API, in which case the nature of the function doesn't come in to it at all. The fix still needs to be evaluated for backport, though, since it is a behavior change. (For example, in another issue Senthil fixed parsing of unknown schemes to be RFC compliant, but we may need to back that fix out of 2.6 since it changes behavior and will most likely break currently working 2.6 code). |
What specific breakage do you expect resulting from my patch being backported? There is no behaviour change here, except to the minimal extent that all bug fixes involve behaviour change. This seems a clear-cut backport candidate. It's not a surprise to me that changing module urlparse to be compliant with RFC 3986 breaks code -- I suggested adding a new module instead for that reason. |
I don't expect anything; I had written that it looked OK to me but apparently I accidentally deleted that text before posting. But I'm not someone who has ever programmed using cookielib so I wouldn't expect my opinion to count for too much. |
Could we please have an update from people who have been involved on this issue as to whether it can be taken forward, closed as to no longer relevant, or whatever. |
My patch should be applied. |
Patched the unit test, then ran the test before applying the fix which failed, after applying the fix the test ran successfully. Tested on Windows Vista 32 bit against 2.7 maintainance release. The patches are short and sweet, I see no reason why they can't go forward. |
jjlee's bpo-3704.patch has been committed to py3k (3.2) in r82985. It could still use backporting to 2.6, 2.7 and 3.1. |
Hmm, I never tested with Python 3, though I assume the forward-port was straightforward. The patch was created against (2.x) trunk, so indeed it should be committed there also. Deselecting 2.6 since I assume no more maintenance backports will be made to 2.6 aside from security fixes -- right? |
Until 2.6.6 is released bug fixes can still be backported to 2.6 but it is at the committer's option. Most likely this one won't be. |
Can we have the patch committed to 2.7 and 3.1 please. |
release27-maint r83145 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: