Fix issue that occurs when using bytes as secret key.

[ghost]

The Django secret key can and should be random bytes which may or may
not be decodable to UTF-8.

[codecov-io]

Current coverage is 76.72% (diff: 100%)

Merging #890 into master will increase coverage by 0.01%

@@             master       #890   diff @@
==========================================
  Files            30         30          
  Lines          1653       1654     +1   
  Methods           0          0          
  Messages          0          0          
  Branches        245        245          
==========================================
+ Hits           1268       1269     +1   
  Misses          311        311          
  Partials         74         74          

Powered by Codecov. Last update cef000a...48ff898

[aaugustin]

I don't think that SECRET_KEY is intended to be of type bytes.

The default, empty value is str, not bytes. The value generated by startproject is also str, not bytes.

https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-SECRET_KEY doesn't say anything about bytes.

If you look at the implementation of get_random_string, it assumes the opposite (in a rarely use code path).

In a 12-factor app it's a best practice to provide secrets via environment variables and random bytes (half of which are non-ASCII) aren't safe to pass via environment variables.

That said, it's best not to crash when SECRET_KEY contains bytes. That's a good change.

I don't see the point of randomizing the SECRET_KEY in the tests setting, what problem is this solving?

[ghost]

There is no default value for SECRET_KEY. If it's left out, Django will throw an error stating the SECRET_KEY is not set. Most of the code that requires the SECRET_KEY use it in methods that only accept it as bytes. For example, see django.utils.crypto (https://github.com/django/django/blob/master/django/utils/crypto.py). Setting the secret key shouldn't be limited to setting them via environment variables. In any case, the script has to fetch the SECRET_KEY from somewhere before placing it in an environment variable.

As for why I changed the SECRET_KEY in the tests, I found that changing the key was the best way to verify make_hash still works. Leaving out the changes to make_hash and setting the SECRET_KEY to a non-UTF-8 byte string would cause the tests to fail. Placing the changes to make_hash and SECRET_KEY causes all tests to pass without modification.

[ghost]

Was there any concern still about this change?

[matthiask]

I don't see any explanation for the removal of the newline normalization. Maybe there's a good reason for that and I don't see it? Please explain.

Moving from force_text to force_bytes is certainly the right thing to do since hashlib expects bytes, and I agree that the crash should be fixed, even though I've never seen a SECRET_KEY consisting of bytes in the wild until now.

[ghost]

I must ask, why was the input to the hash being modified in the first place?

Note that replacing newlines with spaces gives you different hashes. For example...

In [1]: import hashlib

In [2]: hashlib.sha1(b'some long string').hexdigest()
Out[2]: '03058ae698770de83ff929f96773eadfc2b6aacc'

In [3]: hashlib.sha1(b'some\nlong\nstring').hexdigest()
Out[3]: 'b97515a2c115a9872a6e882202615c8c6ad41280'

[matthiask]

The idea probably was that the SQL query would pass through a textarea or something where newlines might be changed from \n to \r\n or vice versa.

I've been thinking a bit, and modifying queries in the browser does not and should not work anyway. So I'm good with this change.

[aaugustin]

Can we just remove the randomization of the secret key in tests?

I really dislike randomness in tests, no matter the reason (except, of course, when testing a statistical process or something that explicitly deals with randomness).

I thought I said that earlier but I can't find that comment 😕

[ghost]

The new secret key is not random at all. It's a byte string of bytes with values 0 through 255 in sorted order.

[ghost]

Here is what that secret key is now.

In [1]: bytes(bytearray([i for i in range(256)]))
Out[1]: b'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff'

[aaugustin]

Ah, right, I had missed that change in your patch.

However, like I explained at length earlier, putting bytes in the SECRET_KEY still goes against Django's expectations and established practice.

There's even a ticket about it: https://code.djangoproject.com/ticket/24994.

The changes to the test settings in this PR are not only useless but also wrong.

[aaugustin]

Revert this.

[ghost]

Please check the ticket you linked again (https://code.djangoproject.com/ticket/24994). They've closed the ticket without making any changes, referencing a comment I left to them (see django/django#7510).

Also note, the Django docs already recommended the use of external files for storing the SECRET_KEY, in which case it's reasonable to expect the SECRET_KEY be bytes, given the multiple ways files can be opened in Python (as bytes). See https://docs.djangoproject.com/en/stable/howto/deployment/checklist/#secret-key .

[matthiask]

open defaults to reading in text mode, which means that the snippet from the documentation returns text, not binary, which negates your point.

I can see the case for putting bytes in SECRET_KEY, especially now that the system check PR in Django has been closed. OTOH as Aymeric said it's not established practice, so please try submitting a documentation patch to Django saying bytes as SECRET_KEY is supported, and maybe even recommended. I don't see why DDT has to move first (especially since, with the rest of your changes, your use case will work perfectly).

Thanks!

[matthiask]

I went ahead and merged your change with an added commit that reverts the SECRET_KEY change.

Thanks for your contribution!