CSRF tests test_https_malformed_host and test_origin_malformed_host aren't testing what they should

[Chris Jerdonek]

I noticed that ​test_https_malformed_host() and ​test_origin_malformed_host() right after it don't seem to be testing what they should.

In both cases, if you change req.META['HTTP_HOST'] = '@malformed' to use a well-formed host, then the test still passes. This is because, unlike some of the other tests, it doesn't use DEBUG=True and check the reason for the 403. The request still results in a 403 because it lacks other needed info.

For tests like this, a perhaps better approach would be for the test to make two identical requests, but differing only in the host: one with the host well-formed and one with it malformed, and checking that the well-formed one succeeds. That could be done e.g. with a helper function that accepts just the host. That would ensure that it's really testing the effect of a malformed host. (The DEBUG=True approach won't always distinguish things in general because code paths other than the intended one can result in the same "reason" string.)

[Chris Jerdonek]

I think test_bad_origin_cannot_be_parsed() is another example of this.

[Tim Graham]

test_bad_origin_cannot_be_parsed fails like this if you remove except ValueError after parsed_origin = urlparse(request_origin):

======================================================================
ERROR: test_bad_origin_cannot_be_parsed (csrf_tests.tests.CsrfViewMiddlewareTests)
A POST request with an origin that can't be parsed by urlparse() is
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/python3.9.1/lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/opt/python3.9.1/lib/python3.9/unittest/case.py", line 593, in run
    self._callTestMethod(testMethod)
  File "/opt/python3.9.1/lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/home/tim/code/django/django/test/utils.py", line 430, in inner
    return func(*args, **kwargs)
  File "/home/tim/code/django/tests/csrf_tests/tests.py", line 614, in test_bad_origin_cannot_be_parsed
    self.assertIs(mw._origin_verified(req), False)
  File "/home/tim/code/django/django/middleware/csrf.py", line 243, in _origin_verified
    parsed_origin = urlparse(request_origin)
  File "/opt/python3.9.1/lib/python3.9/urllib/parse.py", line 390, in urlparse
    splitresult = urlsplit(url, scheme, allow_fragments)
  File "/opt/python3.9.1/lib/python3.9/urllib/parse.py", line 476, in urlsplit
    raise ValueError("Invalid IPv6 URL")
ValueError: Invalid IPv6 URL

​test_origin_malformed_host is similar. I didn't check the last one.

You rightly point out that the tests could give a false positive if they are changed incorrectly, but I wouldn't characterize that as "not testing what they should." There are probably lots of tests throughout the test suite that are like this. I'm not necessarily opposed to an improvement but I'm not sure it brings great value.

[Mariusz Felisiak]

I agree with Tim. I don't think there is anything to fix here.

[Chris Jerdonek]

Sorry, I may have phrased the issue unfairly (including unfairly to the test authors).

The reason I suggested the change is that I was experimenting with some refactoring changes, and the test suite still passed even if I made them incorrectly. That made it seem like something was missing from the tests.

I have a feeling that there can be a way of writing certain tests that ensures they will fail if the code is refactored incorrectly, even if you don't know the refactoring in advance. That could be useful for security-sensitive code. I was having a hard time articulating what that test quality is, though, exactly.