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

Update Flask version to 2.0.*, along with associated requirements #6154

Closed
3 of 5 tasks
zenmonkeykstop opened this issue Oct 26, 2021 · 3 comments · Fixed by #6217
Closed
3 of 5 tasks

Update Flask version to 2.0.*, along with associated requirements #6154

zenmonkeykstop opened this issue Oct 26, 2021 · 3 comments · Fixed by #6217
Assignees
Milestone

Comments

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Oct 26, 2021

Description

Core requirements (Flask, Werkzeug, Jinja, etc) are lagging behind their most up-to-date available versions by quite a bit. This complicates the process of upgrading other requirements with shared dependencies (e.g. the click package).

An attempt at updating these requirements to recent versions has been started in https://github.com/freedomofpress/securedrop/tree/add-https-to-dev - this has uncovered some juicy issues:

  • some application requirements are duplicated in test requirements, and as test requirements are installed last in the dev Docker container, application-side updates to those requirements are not reflected in dev (current workaround in branch is to swap the order in which requirements are installed)
  • Pylint now complains with an E0237: assgning-non-slot error when attributes are assigned to the flask.g object - see Assigning to attribute flask.g.[...] not defined in class slots (assigning-non-slot) pallets/flask#4020 for more. (The simplest workaround for this is to disable pylint assigning-non-slot warnings, either selectively or globally)
  • There are some new mypy errors of note:

A more informed attempt would probably start by cleaning up the existing requirements so that the test requirements are in sync with securedrop-app-code requirements, and then address the mypy globals issue, since that is likely to have the biggest impact on application code.

Prerequisite tasks for this dependency update:

@nabla-c0d3
Copy link
Contributor

nabla-c0d3 commented Oct 30, 2021

A few things I thought of:

Adding attributes to the Flask object (to make them "global") throws an error like error: "Flask" has no attribute "instance_config"

Dynamically adding attributes to the Flask object (or any object) is a bit of an anti-pattern as it causes various problems, one of them being that the dynamically-added attribute is not type-checked at all. Hence, in the context of type-checking, mypy is "right" to complain about it.

I described this problem in more details in #5599. I am close to being done with the CryptoUtil refactoring to specifically address that (next PR at #6087, and after that one there will probably be one more).

I think it would be valuable to remove dynamically-added attributes from the code base. After CryptoUtil, I think there will be a couple more (I remember Storage being one of them).

Pylint now complains with an E0237: assgning-non-slot error when attributes are assigned to the flask.g object.

Similarly, using flask.g is a bit of anti-pattern too as it cannot be type-checked either (the content of g is fully dynamic).
Even Flask recommends against using it in https://flask.palletsprojects.com/en/1.1.x/design/#micro-with-dependencies and I also talked a little bit about it in #5694 . I think the usage of flask.g should be reduced to the minimum.

Overall, both patterns (dynamically adding attributes and using flask.g) are "incompatible" with type-checking (and have other problems). I think there's value in moving away from them. Not really what this GItHub issue is about but I thought I'd give my opinion 😅.

@zenmonkeykstop
Copy link
Contributor Author

Thanks @nabla-c0d3 - it may not explicitly be the topic of the issue but it does seem necessary for the version bump and is extremely useful feedback! Getting type checking working against CryptoUtil is probably the most critical part security-wise - thanks for the work you've done already. IIRC storage and instance_config are also dynamically attached, and can probably be refactored out as globals in much the same way.

I'll take a look at #6087 on Monday and maybe also see about the requirements cleanup mentioned above (which isn't related to the code refactoring, but probably still needs doing).

@zenmonkeykstop zenmonkeykstop added this to the 2.2.0 milestone Nov 4, 2021
@zenmonkeykstop
Copy link
Contributor Author

One gotcha here is that flask-babel's Babel object attaches itself as the babel_instance attribute to the Flask app, so for that one an approach other than the one in the CryptoUtil refactor may be required (or we could suppress that single mypy error).

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