Remove test broken by Python CVE-2021-23336 fix

[andersk]

In Python 3.6.13, 3.7.10, 3.8.8, and 3.9.2, urllib.parse.parse_qsl no longer treats ; as a separator by default.

Fixes #164.

[codecov]

Codecov Report

Merging #165 (78054f1) into master (910ff63) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files           7        7           
  Lines         483      483           
  Branches       99       99           
=======================================
  Hits          461      461           
  Misses         15       15           
  Partials        7        7           

[Gallaecio]

I think it would be best to keep the test with updated expectations depending on the Python version being used, and include a link to https://python-security.readthedocs.io/vuln/urllib-query-string-semicolon-separator.html in a comment.

Based on https://bugs.python.org/issue42967#msg387756:

url = 'http://domain/test?arg1=v1;arg2=v2'
# https://python-security.readthedocs.io/vuln/urllib-query-string-semicolon-separator.html
if 'separator' in inspect.signature(urllib.parse.parse_qs).parameters:
    expected_url = 'http://domain/test?arg1=v3'
else:
    expected_url = 'http://domain/test?arg1=v3&arg2=v2'
self.assertEqual(add_or_replace_parameter(url, 'arg1', 'v3'), expected_url)

[andersk]

TBH I don’t see the advantage in asserting that the insecure version is insecure, but if you do, feel free to modify.

[wRAR]

I'm in favor of just removing the test, especially as we won't run it with non-patched Python versions on CI anyway. I see #166 instead moves it to an xfail test which is fine for me too.

[Gallaecio]

I don’t think tests should fail if run on a Python version without the security patch. We could force an unpatched Python version in CI to make sure we test both behaviors.

I don’t have a strong opinion on it, though. I’m fine with xfail or removing.

And just to be clear, I think we should eventually extend w3lib to support the old behavior, because I expect corner cases where users may need it. Not just the new option, which allows to choose between & and ;, but support for interpreting either as a parameter separator.

In fact, I expect w3lib users like Scrapy to go for the old behavior by default. I don’t think Scrapy is affected by the vulnerability, but not supporting the flexibility of the standard can potentially require a custom middleware to deal with these characters in order to crawl some websites.

[wRAR]

It's an interesting point about supporting old behavior.

[Gallaecio]

This was technically fixed by #166 with the xfail approach.