jpadilla / pyjwt

JSON Web Token implementation in Python
https://pyjwt.readthedocs.io
MIT License
5.05k stars 676 forks source link

Add `sort_headers` parameter to `api_jwt.encode` #832

Closed evroon closed 1 year ago

evroon commented 1 year ago

This allows you to not sort headers, which prevents a breaking change between v2.4.0 and v2.5.0

fixes https://github.com/jpadilla/pyjwt/issues/811

Should I create a unittest for this?

evroon commented 1 year ago

@auvipy I have added a unittest, but it only makes sure that encoding and decoding is the same when using sort_headers=True and sort_headers=False, as well as testing that the encoded payload is not the same between those two options. But I don't see a way to check that the headers are properly sorted before the payload is encoded.

Do you have an idea to improve the test?

akx commented 1 year ago

@evroon

Do you have an idea to improve the test?

Pass in an OrderedDict where you've ensured the keys are in non-sorted order (e.g. OrderedDict([("b", "1"), ("a", "2")])), then decode the base64-encoded header and ensure that .index("b") < .index("a") in the JSON?

evroon commented 1 year ago

@evroon

Do you have an idea to improve the test?

Pass in an OrderedDict where you've ensured the keys are in non-sorted order (e.g. OrderedDict([("b", "1"), ("a", "2")])), then decode the base64-encoded header and ensure that .index("b") < .index("a") in the JSON?

@akx That sounds like a good idea, but the decoding uses json.loads, which returns a regular dict instead of an OrderedDict, so I can't assert the ordering after decoding:

https://github.com/evroon/pyjwt/blob/2ccd0e4110aa539163de8c1a8a72c40a1f7aac2e/jwt/api_jws.py#L261-L264

If it would return an OrderedDict, I would add a test like this:

    def test_sorting_headers(self, jws, payload):
        headers = OrderedDict([("b", "1"), ("a", "2")])
        secret = "\xc2"
        jws_message_unsorted = jws.encode(payload, secret, headers=headers, sort_headers=False)
        jws_message_sorted = jws.encode(payload, secret, headers=headers, sort_headers=True)

        decoded_unsorted = jws.decode_complete(
            jws_message_unsorted, secret, algorithms=["HS256"]
        )
        decoded_sorted = jws.decode_complete(
            jws_message_unsorted, secret, algorithms=["HS256"]
        )

        assert decoded_unsorted['header'].index('a') > decoded_unsorted['header'].index('b')
        assert decoded_sorted['header'].index('a') < decoded_sorted['header'].index('b')
akx commented 1 year ago

@evroon You could split the JWT string "by hand" and just base64-decode the header segment so you get a string of JSON, no need to use the library's decoder for the test :)

evroon commented 1 year ago

Ah yes of course

I have now improved the test. I chose to assert that the whole base64 byte string matches the expectation, instead of parsing the json and looking at the ordering of keys, since I wanted to avoid json.loads.

I'm not sure what the problem is with CI though.

auvipy commented 1 year ago

ping me when CI is green

evroon commented 1 year ago

@auvipy the CI is green now

evroon commented 1 year ago

Thanks!