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.

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

CryptoUtil code is not type-checked due to dynamic lookup/usage #5599

Closed
nabla-c0d3 opened this issue Oct 24, 2020 · 2 comments
Closed

CryptoUtil code is not type-checked due to dynamic lookup/usage #5599

nabla-c0d3 opened this issue Oct 24, 2020 · 2 comments

Comments

@nabla-c0d3
Copy link
Contributor

Description

Initial discussion at https://github.com/freedomofpress/securedrop/pull/5532/files#r495506285

The CryptoUtil class now has type annotations, but they are never actually checked because the CryptoUtil instance is dynamically attached to the Flask app, and later dynamically accessed when it is used throughout the code base:

    app.crypto_util = CryptoUtil()
    # [...]
    from flask import current_app
    current_app.crypto_util.do_something()  # Problem: code analysis tools do not know what current_app.crypto_util is

This prevents type checking and other code analysis tools from properly analyzing CryptoUtil. However this class has security-sensitive code which should be checked using all the tools available.

Steps to Reproduce

  1. Write buggy/invalid code that uses the existing pattern in the code base for accessing CryptoUtil. For example:
    from flask import current_app
    current_app.crypto_util.encrypt("missing arguments here")
  1. Run mypy on the code base.
  2. Mypy does not detect any problem although the code is wrong as it's missing arguments for the encrypt() method.

Expected Behavior

The following error when running mypy:

error: Too few arguments for "encrypt" of "CryptoUtil"

Comments

The best solution I can think of is to move-away from the following pattern:

    app.crypto_util = CryptoUtil()
    # [...]
    from flask import current_app
    current_app.crypto_util.do_something()  # Problem: not type-checked

And instead of attaching the CryptoUtil instance to the Flask app (which also significantly complicates the test suite), making it an explicit global that's independent of Flask or the Source/Journalist apps (ie. less coupling):

    crypto_util = CryptoUtil.get_default()
    crypto_util.do_something()  # Properly type-checked
@nabla-c0d3
Copy link
Contributor Author

This is now fixed 🥂

@legoktm legoktm closed this as completed May 4, 2022
@legoktm
Copy link
Member

legoktm commented May 4, 2022

(assuming it was just an oversight this issue wasn't closed earlier)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants