Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prefer headers['alg'] to algorithm parameter in encode(). #673

Merged
merged 6 commits into from Aug 6, 2021
Merged

Prefer headers['alg'] to algorithm parameter in encode(). #673

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4056d8
Prefer headers['alg'] to algorithm parameter in encode().
dajiaji Jul 22, 2021
a466a9a
Fix lack of @crypto_required.
dajiaji Jul 22, 2021
c59fe58
Prefer headers['alg'] to algorithm parameter in encode().
dajiaji Jul 22, 2021
a52d53b
Prefer headers['alg'] to algorithm parameter in encode().
dajiaji Jul 22, 2021
e95389a
Merge master to resolve conflict.
dajiaji Jul 30, 2021
9aaac14
Make algorithm parameter of encode() Optioanl explicitly.
dajiaji Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/api.rst
View file
Open in desktop
Expand Up @@ -13,8 +13,9 @@ API Reference
* for **asymmetric algorithms**: PEM-formatted private key, a multiline string
* for **symmetric algorithms**: plain string, sufficiently long for security

:param str algorithm: algorithm to sign the token with, e.g. ``"ES256"``
:param dict headers: additional JWT header fields, e.g. ``dict(kid="my-key-id")``
:param str algorithm: algorithm to sign the token with, e.g. ``"ES256"``.
If ``headers`` includes ``alg``, it will be preferred to this parameter.
:param dict headers: additional JWT header fields, e.g. ``dict(kid="my-key-id")``.
:param json.JSONEncoder json_encoder: custom JSON encoder for ``payload`` and ``headers``
:rtype: str
:returns: a JSON Web Token
Expand Down
4 changes: 4 additions & 0 deletions jwt/api_jws.py
View file
Open in desktop
Expand Up @@ -86,6 +86,10 @@ def encode(
if algorithm is None:
algorithm = "none"

# Prefer headers["alg"] if present to algorithm parameter.
if headers and "alg" in headers and headers["alg"]:
algorithm = headers["alg"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would type definitions for algorithm need updating?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If header["alg"] is always preferred to algorithm, then no. It can remain a str.

But lines 86-87 (if algorithm is None: ...) are unneeded. I'd remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since algorithm parameter is practically optional and for keeping backward-compat, I don't think its type definition should be updated at least for now.

But lines 86-87 (if algorithm is None: ...) are unneeded. I'd remove that.

As the current spec, algorithm=None is interpreted to {"alg": "none"} and related tests exist. So it should not be changed for keeping backward-compat for now. It needs another PR if needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it then already be of type Optional[str]? 🤔 Passing algorithm=None fails the typecheck with the current annotation. Keeping it annotated as a str only makes sense if that option will (eventually) be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it then already be of type Optional[str]? Passing algorithm=None fails the typecheck with the current annotation.

I agree with you and I have recognized this problem. I doubt this fix need to be done in this PR but I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpadilla I fixed the type definitions. I'm glad if you check and merge it.


if algorithm not in self._valid_algs:
pass

Expand Down
26 changes: 26 additions & 0 deletions tests/test_api_jws.py
View file
Open in desktop
Expand Up @@ -166,6 +166,32 @@ def test_encode_algorithm_param_should_be_case_sensitive(self, jws, payload):
exception = context.value
assert str(exception) == "Algorithm not supported"

def test_encode_with_headers_alg_none(self, jws, payload):
msg = jws.encode(payload, key=None, headers={"alg": "none"})
with pytest.raises(DecodeError) as context:
jws.decode(msg, algorithms=["none"])
assert str(context.value) == "Signature verification failed"

@crypto_required
def test_encode_with_headers_alg_es256(self, jws, payload):
with open(key_path("testkey_ec.priv"), "rb") as ec_priv_file:
priv_key = load_pem_private_key(ec_priv_file.read(), password=None)
with open(key_path("testkey_ec.pub"), "rb") as ec_pub_file:
pub_key = load_pem_public_key(ec_pub_file.read())

msg = jws.encode(payload, priv_key, headers={"alg": "ES256"})
assert b"hello world" == jws.decode(msg, pub_key, algorithms=["ES256"])

@crypto_required
def test_encode_with_alg_hs256_and_headers_alg_es256(self, jws, payload):
with open(key_path("testkey_ec.priv"), "rb") as ec_priv_file:
priv_key = load_pem_private_key(ec_priv_file.read(), password=None)
with open(key_path("testkey_ec.pub"), "rb") as ec_pub_file:
pub_key = load_pem_public_key(ec_pub_file.read())

msg = jws.encode(payload, priv_key, algorithm="HS256", headers={"alg": "ES256"})
assert b"hello world" == jws.decode(msg, pub_key, algorithms=["ES256"])

def test_decode_algorithm_param_should_be_case_sensitive(self, jws):
example_jws = (
"eyJhbGciOiJoczI1NiIsInR5cCI6IkpXVCJ9" # alg = hs256
Expand Down