[ENG-2691] Replace python-jose with joserfc#7573
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughReplaces python-jose with joserfc across the project: dependency and mypy overrides updated, JWE generation/decryption reimplemented to use joserfc compact APIs and OctKey import, and exception handling updated in token utilities and a user endpoint. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as UserEndpoint
participant Utils as OAuthUtils
participant JWT as JWTModule
participant KeyLib as joserfc
Client->>Endpoint: logout request (token)
Endpoint->>Utils: extract_token_and_load_client(token)
Utils->>JWT: decrypt_jwe(token, encryption_key)
JWT->>KeyLib: OctKey.import_key(encryption_key)
KeyLib-->>JWT: ImportedKey
JWT->>KeyLib: jwe.decrypt_compact(token, ImportedKey)
KeyLib-->>JWT: plaintext
JWT-->>Utils: decoded payload
Utils-->>Endpoint: client info or error
Endpoint-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
I need to check on fidesplus as well, once all the tests are run, to see if there are any changes there that need to happen in lock-step. I don't think there are but I want to be sure. |
Greptile SummaryThis PR replaces the unmaintained Key observations:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 6bb6fda |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/fides/api/oauth/utils.py
Outdated
| except DecodeError as exc: | ||
| logger.debug("Unable to parse auth token.") | ||
| raise AuthorizationError(detail="Not Authorized for this action") from exc |
There was a problem hiding this comment.
DecodeError too narrow — non-decode JoseError escapes as 500
extract_payload (line 70) catches and re-raises the full JoseError base class. extract_token_and_load_client (and its async twin at line 520) only catches the narrower DecodeError subclass. Any JoseError that is not a DecodeError — for example a key-size/algorithm mismatch raised by joserfc during decryption — will propagate uncaught all the way up and produce an unhandled 500 error instead of a 403.
In python-jose, JWEParseError covered both structural parse failures and ciphertext corruption. In joserfc those failure modes may surface as different JoseError subclasses, so narrowing the catch to only DecodeError is not a safe like-for-like replacement.
The fix is to catch the base JoseError here (and in the async variant), consistent with how the surrounding _get_webhook_jwe_or_error, _get_request_task_jwe_or_error, and validate_download_token functions already handle it.
| except DecodeError as exc: | |
| logger.debug("Unable to parse auth token.") | |
| raise AuthorizationError(detail="Not Authorized for this action") from exc | |
| except JoseError as exc: | |
| logger.debug("Unable to parse auth token.") | |
| raise AuthorizationError(detail="Not Authorized for this action") from exc |
The same change is needed at line 520 in extract_token_and_load_client_async.
pyproject.toml
Outdated
| "pyinstrument==4.5.1", | ||
| "pymongo[srv]==4.7.3", | ||
| "python-jose[cryptography]==3.5.0", | ||
| "joserfc>=0.12.0", |
There was a problem hiding this comment.
Open-ended version constraint is inconsistent with project style
All other production dependencies in this file use pinned or tightly bounded constraints (e.g. redis==7.2.0, loguru==0.7.3). Using joserfc>=0.12.0 means any future major version of the library could be pulled in on a fresh install, potentially introducing breaking API changes in security-critical code.
Consider pinning to the version that is already locked (1.6.3), or at minimum constraining to a compatible minor range:
| "joserfc>=0.12.0", | |
| "joserfc>=1.6.3,<2", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fides/api/api/v1/endpoints/user_endpoints.py (1)
281-286:⚠️ Potential issue | 🟡 MinorCatch
JoseErrorfor consistency with otherextract_payloadcallers.The
extract_payloadfunction inoauth/utils.py(lines 70–72) catches and re-raisesJoseError, the parent exception class. However, this code at line 285 catches onlyDecodeError, a subclass. WhileDecodeErrorhandles most decryption failures, otherJoseErrorsubclasses (e.g., from invalid key handling) could be raised and remain uncaught. Additionally, elsewhere inoauth/utils.py, the same function is caught asJoseError(lines 150, 178, 222), creating inconsistency.Change line 285 from
except DecodeError:toexcept JoseError:to align with the broader error handling pattern and ensure all decryption-related failures are handled uniformly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/api/v1/endpoints/user_endpoints.py` around lines 281 - 286, Change the exception caught around the call to extract_payload(authorization, CONFIG.security.app_encryption_key) in user_endpoints.py: replace the specific DecodeError with the broader JoseError so the try/except matches other callers; locate the try block that assigns token_data via json.loads(extract_payload(...)) and update the except clause to except JoseError: to uniformly handle all Jose-related errors.
🧹 Nitpick comments (3)
src/fides/api/oauth/jwt.py (2)
25-43: Same type annotation issue and potential code duplication.
The
encryption_keyparameter has the same type annotation inconsistency asgenerate_jwe.This function has significant overlap with
extract_payloadinutils.py(lines 59-72). Both decode JWE tokens using the same pattern. Consider consolidating to avoid duplication.🔍 Potential consolidation
extract_payloadinutils.pycould potentially calldecrypt_jwefrom this module:# In utils.py from fides.api.oauth.jwt import decrypt_jwe def extract_payload(jwe_string: str, encryption_key: str) -> str: """Given a jwe, extracts the payload and returns it in string form.""" try: return decrypt_jwe(jwe_string, encryption_key) except JoseError as e: logger.debug("Failed to decrypt JWE: {}", e) raise e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/jwt.py` around lines 25 - 43, The decrypt_jwe function's encryption_key annotation should match generate_jwe (accept both str and bytes) and avoid duplicating logic present in utils.extract_payload: change decrypt_jwe's signature to accept Union[str, bytes] (or similar), handle conversion to bytes and decryption inside decrypt_jwe, then refactor utils.extract_payload to call fides.api.oauth.jwt.decrypt_jwe and handle JoseError/logging there (e.g., catch JoseError in extract_payload and re-raise after logging) so the JWE decoding logic is centralized in decrypt_jwe.
7-22: Type annotation doesn't reflect actual behavior.The function signature declares
encryption_key: str, but lines 12-16 explicitly handle bothstrandbytestypes. Consider updating the type annotation to reflect this:♻️ Suggested type annotation update
-def generate_jwe(payload: str, encryption_key: str, encoding: str = "UTF-8") -> str: +def generate_jwe(payload: str, encryption_key: str | bytes, encoding: str = "UTF-8") -> str:Or using Union for older Python compatibility:
from typing import Union def generate_jwe(payload: str, encryption_key: Union[str, bytes], encoding: str = "UTF-8") -> str:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/jwt.py` around lines 7 - 22, Update the generate_jwe signature to accept both str and bytes for encryption_key (e.g., use Union[str, bytes] or str | bytes) because the implementation in generate_jwe explicitly handles both types when building key_bytes and calling OctKey.import_key; adjust the type import (typing.Union or use PEP 604 syntax) and update the encryption_key annotation accordingly so the signature matches the runtime behavior.src/fides/api/oauth/utils.py (1)
49-49: Remove unused constantJWT_ENCRYPTION_ALGORITHM.The
JWT_ENCRYPTION_ALGORITHMconstant defined at line 49 is not used anywhere in the codebase. The actual encryption algorithm specification is handled separately injwt.pyusing_JWT_ENCRYPTION_ALGORITHM. This constant appears to be dead code and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/utils.py` at line 49, Remove the dead constant JWT_ENCRYPTION_ALGORITHM from src/fides/api/oauth/utils.py: it is unused and duplicates the actual value defined as _JWT_ENCRYPTION_ALGORITHM in jwt.py; delete the JWT_ENCRYPTION_ALGORITHM line and run tests/linting to ensure no references remain (search for JWT_ENCRYPTION_ALGORITHM and confirm code uses _JWT_ENCRYPTION_ALGORITHM in jwt.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/oauth/utils.py`:
- Around line 59-72: The extract_payload function currently catches JoseError
and re-raises it, which leaves callers that only catch DecodeError (e.g.,
extract_token_and_load_client and extract_token_and_load_client_async) exposed
to other JoseError subclasses; change extract_payload (around OctKey.import_key
and jwe.decrypt_compact usage) to catch JoseError and raise a
jose.exceptions.DecodeError (or the library's DecodeError) from the caught
exception so callers that only handle DecodeError will receive the normalized
exception; preserve the original exception as the cause when raising to keep
debugging info.
---
Outside diff comments:
In `@src/fides/api/api/v1/endpoints/user_endpoints.py`:
- Around line 281-286: Change the exception caught around the call to
extract_payload(authorization, CONFIG.security.app_encryption_key) in
user_endpoints.py: replace the specific DecodeError with the broader JoseError
so the try/except matches other callers; locate the try block that assigns
token_data via json.loads(extract_payload(...)) and update the except clause to
except JoseError: to uniformly handle all Jose-related errors.
---
Nitpick comments:
In `@src/fides/api/oauth/jwt.py`:
- Around line 25-43: The decrypt_jwe function's encryption_key annotation should
match generate_jwe (accept both str and bytes) and avoid duplicating logic
present in utils.extract_payload: change decrypt_jwe's signature to accept
Union[str, bytes] (or similar), handle conversion to bytes and decryption inside
decrypt_jwe, then refactor utils.extract_payload to call
fides.api.oauth.jwt.decrypt_jwe and handle JoseError/logging there (e.g., catch
JoseError in extract_payload and re-raise after logging) so the JWE decoding
logic is centralized in decrypt_jwe.
- Around line 7-22: Update the generate_jwe signature to accept both str and
bytes for encryption_key (e.g., use Union[str, bytes] or str | bytes) because
the implementation in generate_jwe explicitly handles both types when building
key_bytes and calling OctKey.import_key; adjust the type import (typing.Union or
use PEP 604 syntax) and update the encryption_key annotation accordingly so the
signature matches the runtime behavior.
In `@src/fides/api/oauth/utils.py`:
- Line 49: Remove the dead constant JWT_ENCRYPTION_ALGORITHM from
src/fides/api/oauth/utils.py: it is unused and duplicates the actual value
defined as _JWT_ENCRYPTION_ALGORITHM in jwt.py; delete the
JWT_ENCRYPTION_ALGORITHM line and run tests/linting to ensure no references
remain (search for JWT_ENCRYPTION_ALGORITHM and confirm code uses
_JWT_ENCRYPTION_ALGORITHM in jwt.py).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d040fcc9-9d20-4de4-acd3-c06215ea8d84
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.tomlsrc/fides/api/api/v1/endpoints/user_endpoints.pysrc/fides/api/oauth/jwt.pysrc/fides/api/oauth/utils.py
| def extract_payload(jwe_string: str, encryption_key: str) -> str: | ||
| """Given a jwe, extracts the payload and returns it in string form.""" | ||
| try: | ||
| decrypted_payload = jwe.decrypt(jwe_string, encryption_key) | ||
| return decrypted_payload.decode("utf-8") | ||
| except exceptions.JWEError as e: | ||
| key_bytes = ( | ||
| encryption_key.encode("utf-8") | ||
| if isinstance(encryption_key, str) | ||
| else encryption_key | ||
| ) | ||
| key = OctKey.import_key(key_bytes) | ||
| result = jwe.decrypt_compact(jwe_string, key) | ||
| return result.plaintext.decode("utf-8") | ||
| except JoseError as e: | ||
| logger.debug("Failed to decrypt JWE: {}", e) | ||
| raise e |
There was a problem hiding this comment.
Exception handling inconsistency: function raises JoseError but some callers catch DecodeError.
The extract_payload function catches and re-raises JoseError (line 70-72). However, in extract_token_and_load_client (line 447) and extract_token_and_load_client_async (line 520), only DecodeError is caught.
Since DecodeError is a subclass of JoseError, other JoseError variants (e.g., encryption errors, key errors) would propagate uncaught and potentially cause 500 errors instead of proper authorization failures.
🔧 Suggested fix for consistency
Either change extract_payload to only raise DecodeError:
- except JoseError as e:
+ except DecodeError as e:
logger.debug("Failed to decrypt JWE: {}", e)
raise eOr update the callers to catch JoseError:
# In extract_token_and_load_client (line 447)
- except DecodeError as exc:
+ except JoseError as exc:
# In extract_token_and_load_client_async (line 520)
- except DecodeError as exc:
+ except JoseError as exc:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def extract_payload(jwe_string: str, encryption_key: str) -> str: | |
| """Given a jwe, extracts the payload and returns it in string form.""" | |
| try: | |
| decrypted_payload = jwe.decrypt(jwe_string, encryption_key) | |
| return decrypted_payload.decode("utf-8") | |
| except exceptions.JWEError as e: | |
| key_bytes = ( | |
| encryption_key.encode("utf-8") | |
| if isinstance(encryption_key, str) | |
| else encryption_key | |
| ) | |
| key = OctKey.import_key(key_bytes) | |
| result = jwe.decrypt_compact(jwe_string, key) | |
| return result.plaintext.decode("utf-8") | |
| except JoseError as e: | |
| logger.debug("Failed to decrypt JWE: {}", e) | |
| raise e | |
| def extract_payload(jwe_string: str, encryption_key: str) -> str: | |
| """Given a jwe, extracts the payload and returns it in string form.""" | |
| try: | |
| key_bytes = ( | |
| encryption_key.encode("utf-8") | |
| if isinstance(encryption_key, str) | |
| else encryption_key | |
| ) | |
| key = OctKey.import_key(key_bytes) | |
| result = jwe.decrypt_compact(jwe_string, key) | |
| return result.plaintext.decode("utf-8") | |
| except DecodeError as e: | |
| logger.debug("Failed to decrypt JWE: {}", e) | |
| raise e |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/oauth/utils.py` around lines 59 - 72, The extract_payload
function currently catches JoseError and re-raises it, which leaves callers that
only catch DecodeError (e.g., extract_token_and_load_client and
extract_token_and_load_client_async) exposed to other JoseError subclasses;
change extract_payload (around OctKey.import_key and jwe.decrypt_compact usage)
to catch JoseError and raise a jose.exceptions.DecodeError (or the library's
DecodeError) from the caught exception so callers that only handle DecodeError
will receive the normalized exception; preserve the original exception as the
cause when raising to keep debugging info.
| extract_payload(authorization, CONFIG.security.app_encryption_key) | ||
| ) | ||
| except jose.exceptions.JWEParseError: | ||
| except DecodeError: |
There was a problem hiding this comment.
see the robot's comments about catching JoseError instead :)
src/fides/api/oauth/jwt.py
Outdated
| from joserfc.jwk import OctKey | ||
|
|
||
| _JWT_ENCRYPTION_ALGORITHM = ALGORITHMS.A256GCM | ||
| _JWT_ENCRYPTION_ALGORITHM = "A256GCM" |
There was a problem hiding this comment.
u_u sad it's not a constant anymore
src/fides/api/oauth/utils.py
Outdated
| JWT_ENCRYPTION_ALGORITHM = ALGORITHMS.A256GCM | ||
| JWT_ENCRYPTION_ALGORITHM = "A256GCM" | ||
|
|
There was a problem hiding this comment.
can we define this constant in a single module and use it from there instead of having it duplicated ?
src/fides/api/oauth/utils.py
Outdated
| key_bytes = ( | ||
| encryption_key.encode("utf-8") | ||
| if isinstance(encryption_key, str) | ||
| else encryption_key | ||
| ) | ||
| key = OctKey.import_key(key_bytes) | ||
| result = jwe.decrypt_compact(jwe_string, key) | ||
| return result.plaintext.decode("utf-8") |
There was a problem hiding this comment.
this code is the same as decrypt_jwe right? can we call that function instead of duplicating it?
…t miss broader JOSE exceptions, consolidated the encryption string and functions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fides/api/oauth/utils.py (1)
135-140:⚠️ Potential issue | 🟡 MinorCatch
ValueErroron these token parsing branches too.These
tryblocks wrap bothextract_payload(...)andjson.loads(...).JSONDecodeErrorand decode-relatedValueErrors will currently bypass these handlers and bubble out as 500s, while other auth-token handlers in the same file already normalize both exception types.🛡️ Suggested fix
- except JoseError: + except (JoseError, ValueError):Apply the same change at lines 135-140, 163-168, and 207-212.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/utils.py` around lines 135 - 140, Update the try/except blocks that call extract_payload(...) followed by json.loads(...) so they catch decode-related errors: change except JoseError: to except (JoseError, ValueError): and re-raise AuthorizationError(detail="Not Authorized for this action"); apply this same change to each token-parsing block in the file where extract_payload and json.loads are used (the three similar try/except sites) so JSONDecodeError/ValueError no longer bubble up as 500s.
🧹 Nitpick comments (1)
src/fides/api/oauth/utils.py (1)
57-61: Use bareraiseto re-raise the caught exception.In Python, bare
raiseis the idiomatic way to re-raise an exception without modification. It maintains the original traceback context with clearer intent thanraise e.♻️ Suggested fix
except JoseError as e: logger.debug("Failed to decrypt JWE: {}", e) - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/utils.py` around lines 57 - 61, The except block catching JoseError in the decrypt_jwe call should re-raise the original exception using a bare raise to preserve the original traceback; update the except clause in the code handling decrypt_jwe to call logger.debug("Failed to decrypt JWE: {}", e) (or preferably logger.debug with proper formatting) and replace "raise e" with a bare "raise" so the original traceback is retained when re-raising the caught JoseError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/fides/api/oauth/utils.py`:
- Around line 135-140: Update the try/except blocks that call
extract_payload(...) followed by json.loads(...) so they catch decode-related
errors: change except JoseError: to except (JoseError, ValueError): and re-raise
AuthorizationError(detail="Not Authorized for this action"); apply this same
change to each token-parsing block in the file where extract_payload and
json.loads are used (the three similar try/except sites) so
JSONDecodeError/ValueError no longer bubble up as 500s.
---
Nitpick comments:
In `@src/fides/api/oauth/utils.py`:
- Around line 57-61: The except block catching JoseError in the decrypt_jwe call
should re-raise the original exception using a bare raise to preserve the
original traceback; update the except clause in the code handling decrypt_jwe to
call logger.debug("Failed to decrypt JWE: {}", e) (or preferably logger.debug
with proper formatting) and replace "raise e" with a bare "raise" so the
original traceback is retained when re-raising the caught JoseError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d3c1b1e-6bc5-4169-8066-25c53f00f73c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/fides/api/oauth/jwt.pysrc/fides/api/oauth/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
erosselli
left a comment
erosselli
left a comment
There was a problem hiding this comment.
We're still catching DecodeError in src/fides/api/api/v1/endpoints/user_endpoints.py , should that be JoseError? Don't forget the changelog entry
…changelog yaml and mypy fix
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fides/api/oauth/jwt.py (1)
7-22: Implementation correctly uses the joserfc API.The function properly:
- Converts the encryption key to bytes
- Imports it as an
OctKeyfor symmetric encryption- Calls
encrypt_compactwith the correct parameters: protected header dict, plaintext bytes, and key objectMinor note: The type annotation specifies
encryption_key: str, but the code includes an else branch handling bytes (lines 14–15). If the function should accept both types, update the annotation toUnion[str, bytes]; otherwise, remove the unreachable else branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/oauth/jwt.py` around lines 7 - 22, The type annotation for generate_jwe claims encryption_key: str but the implementation accepts bytes too; update the signature to accept Union[str, bytes] (import Union from typing) so the parameter type matches the branch that handles both str and bytes, leaving the existing conversion logic (the isinstance check and OctKey.import_key call) intact; alternatively, if you only want str, remove the else branch that handles bytes—reference generate_jwe and the encryption_key parameter when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fides/api/oauth/jwt.py`:
- Around line 7-22: The type annotation for generate_jwe claims encryption_key:
str but the implementation accepts bytes too; update the signature to accept
Union[str, bytes] (import Union from typing) so the parameter type matches the
branch that handles both str and bytes, leaving the existing conversion logic
(the isinstance check and OctKey.import_key call) intact; alternatively, if you
only want str, remove the else branch that handles bytes—reference generate_jwe
and the encryption_key parameter when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7107ed3a-be00-4876-902b-ea60a863b82b
📒 Files selected for processing (3)
changelog/7573.yamlsrc/fides/api/api/v1/endpoints/user_endpoints.pysrc/fides/api/oauth/jwt.py
✅ Files skipped from review due to trivial changes (1)
- changelog/7573.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fides/api/api/v1/endpoints/user_endpoints.py
2 participants
Ticket ENG-2691
Description Of Changes
python-jose, which we use for encrypting tokens, is subject to a CVE that will not be fixed (as stated by the authors and the maintainers of the underlying CVE-related library) so we are replacing it withjoserfcwhich does not use the vulnerable encryption library and instead uses one that is maintained and secure.Code Changes
Steps to Confirm
No manual tests needed
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit