Skip to content

Upgrade dependencies on v2 banch #1992

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

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

Conversation

mfmarche
Copy link
Contributor

@mfmarche mfmarche commented Nov 9, 2024

Fixes #1969

The main goal was upgrading werkzeug for CVE-2024-34069.
After switching to python 3.12, it proved more difficult with changes to
setuptools, etc. I decided to pull the pyproject from the main, and
utilize that, alone with updated dependencies. Small changes were needed
in various api changes, notably:

- flask change of request_ctx
- swagger_ui_bundle version change, default_template_dir change
- aiohttp middleware api slightly changed
- flask json change, using flask.json.provider

I believe these changes will have minimal impact to users, but the
changes are likely breaking for some, specifically, the move to latest
flask.

The main goal was upgrading werkzeug for CVE-2024-34069.
After switching to python 3.12, it proved more difficult with changes to
setuptools, etc. I decided to pull the pyproject from the main, and
utilize that, alone with updated dependencies. Small changes were needed
in various api changes, notably:

- flask change of request_ctx
- swagger_ui_bundle version change, default_template_dir change
- aiohttp middleware api slightly changed
- flask json change, using flask.json.provider

I believe these changes will have minimal impact to users, but the
changes are likely breaking for some, specifically, the move to latest
flask.

fixes spec-first#1969

Signed-off-by: Mike Marchetti <mfmarche@gmail.com>
aiohttp has conflicting requirements for latest updates, where python3.8
is deprecated. Remove 3.8 support to simplify the requirements to take
latest.

Signed-off-by: Mike Marchetti <mfmarche@gmail.com>
@mfmarche
Copy link
Contributor Author

mfmarche commented Nov 9, 2024

I tried to fixup the readthedocs build, i'm not sure how to resolve the issue. It could be a conflicting dependency? Is this build job required on this branch?

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @mfmarche!

I haven't gone through the code changes in detail yet, but the passing tests with limited changes look promising!

The docs pipeline is configured in this repo in .readthedocs.yaml, the dependencies are defined in pyproject.toml. The pipeline runs on ReadTheDocs. I suspect the issue is due to a mismatch in sphinx and sphinx-autoapi versions (logs).

I don't want to break people's projects relying on Connexion 2 by releasing breaking changes. However I am open to releasing this fix under the connexion2 pypi name instead.

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the old README on this branch, so the information and examples match the code.

Dockerfile3.9 Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Where is this file used?

Copy link
Member

Choose a reason for hiding this comment

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

I assume we can remove setup.cfg and setup.py now?

Comment on lines +17 to +20
3.9: py39-pypi
3.10: py310-pypi
3.11: py311-pypi,pre-commit
3.12: py312-pypi
Copy link
Member

Choose a reason for hiding this comment

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

The github actions workflow should be updated to match this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR now includes an update to the GitHub workflow for pythons 3.9 .. 3.12

pyproject.toml Outdated
PyYAML = ">= 5.1"
requests = ">= 2.27"
typing-extensions = ">= 4.6.1"
werkzeug = ">= 2.2.1"
Copy link
Member

Choose a reason for hiding this comment

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

Let's bump this to a version with the CVE fixes, since this was the main reason for this PR.
3.1.3 is the version used in the 3.9 workflow with passing tests.

- fixup doc genenerator, using config from main. Too many issues
  encountered trying to get a working autoapi version to work with
sphinx. Decided to adopt a more recent version of sphinx and utilized
Connexion/main's docs/conf.py file.
- add pipeline builds for supported python
- snap werkzeug version to address CVE's

Signed-off-by: Mike Marchetti <mfmarche@gmail.com>
@mfmarche
Copy link
Contributor Author

thanks @RobbeSneyders for the review. I addressed all issues I believe.

@ykharko
Copy link

ykharko commented Dec 1, 2024

Hey guys, sorry for chasing. What's left here? Is there a chance to get it merged and released? I believe many people are waiting for werkzeug updates because of CVEs.

Are thare any blockers here and is there a chance to release it in the nearest future?

@mfmarche
Copy link
Contributor Author

mfmarche commented Dec 2, 2024

Hi @ykharko, I believe all issues were resolved, just waiting for final review and hopefully it can be released. thanks.

@noirbee
Copy link
Contributor

noirbee commented Dec 31, 2024

Hi @mfmarche , thanks a lot for this, started working on this for the same reasons but your work is much further along, especially in terms of CI / packaging etc.

I've started testing our code base against your branch, and the first breakage I've come across is the fact that the FlaskJSONEncoder now inherits from Python's own default JSONEncoder; i.e. it does not end up calling flask.json.provider._default(o) after falling through the few types connexion explicitly handles.

Arguably the encoder could be removed as I'm not sure it was intended to be part of connexion's actual module API, unfortunately it's used by other projects, notably openapi-code-generator's flask template, which we also use.

Would you consider the following patch against your branch to keep backwards compatibility ?

diff --git a/connexion/apps/flask_app.py b/connexion/apps/flask_app.py
index e9efaef..9814a89 100644
--- a/connexion/apps/flask_app.py
+++ b/connexion/apps/flask_app.py
@@ -153,10 +153,8 @@ class FlaskApp(AbstractApp):


 class FlaskJSONProvider(DefaultJSONProvider):
-    def __init__(self, app):
-        super().__init__(app)
-
-    def default(self, o):
+    @classmethod
+    def default(cls, o):
         if isinstance(o, datetime.datetime):
             if o.tzinfo:
                 # eg: '2015-09-25T23:14:42.588601+00:00'
@@ -177,22 +175,7 @@ class FlaskJSONProvider(DefaultJSONProvider):

 class FlaskJSONEncoder(json.JSONEncoder):
     def default(self, o):
-        if isinstance(o, datetime.datetime):
-            if o.tzinfo:
-                # eg: '2015-09-25T23:14:42.588601+00:00'
-                return o.isoformat('T')
-            else:
-                # No timezone present - assume UTC.
-                # eg: '2015-09-25T23:14:42.588601Z'
-                return o.isoformat('T') + 'Z'
-
-        if isinstance(o, datetime.date):
-            return o.isoformat()
-
-        if isinstance(o, Decimal):
-            return float(o)
-
-        return json.JSONEncoder.default(self, o)
+        return FlaskJSONProvider.default(o)


 class NumberConverter(werkzeug.routing.BaseConverter):

(I can also open a separate PR against your branch if you'd rather review it separately)

Update from @noirbee to ensure that flask.json.provider.DefaultJSONProvider.default is called.
@mfmarche
Copy link
Contributor Author

mfmarche commented Jan 2, 2025

thanks @noirbee for providing the feedback. I have updated the MR with your suggested changes.

@mfmarche
Copy link
Contributor Author

mfmarche commented Jan 2, 2025

If I'm missing something else @RobbeSneyders please let me know. I wasn't sure how you would like to publish to connexion2, if you want to change the pyproject.toml? Thank you.

@hehe7318
Copy link

hehe7318 commented Feb 17, 2025

Hi team, hi @mfmarche, hi @RobbeSneyders, This PR will help a lot to upgrade werkzeug for CVE-2024-34069. Do you have an estimate time to release it?

@gmarciani
Copy link

Hi team,

what's the timeline to merge this PR and release a new 2.x version?
Thanks

@mfmarche
Copy link
Contributor Author

Hi @gmarciani , @hehe7318 , I have no idea about timeline. I would also like to get this merged, important for security fixes. thanks.

hehe7318 added a commit to hehe7318/aws-parallelcluster that referenced this pull request Feb 20, 2025
@gmarciani
Copy link

Reached out directly to @RobbeSneyders to have updates on the new release.

@sebastien-boulle
Copy link

Hi Team,

It would be very nice for security reason to merge it and release it :)
Especially because it's also blocking Werkzeug security update.

Any timeline would be appreciate.
Thanks

@advance512
Copy link

Hello @RobbeSneyders @chrisinmtown , could we proceed with merging this in and releasing a new v2 version?

It is important and apparently a mere approving review away to patch the security issues for us long-time loyal users.
We really do want to upgrade to Connexion v3 but that path is long & windy. Current security vulnerabilities are critical.

Is there anything we could do to help with getting this over the finish line?

Thank you so much @mfmarche for your hard work here.

@chrisinmtown
Copy link
Contributor

I support merging this too! Please note I'm a fellow user and community member, not a maintainer.

@chrisinmtown
Copy link
Contributor

@mfmarche would you please suggest the most obvious way to build/test an existing project that's based on Connexion v2 with the version in this PR? I'd like to try that for my two critical projects. FWIW I am blocked on the upgrade to V3 due to bugs that remain unresolved, like the problem with processing allOf in OpenAPI specs. Thanks in advance.

@mfmarche
Copy link
Contributor Author

mfmarche commented Apr 9, 2025

Hi @chrisinmtown , you can use my fork on branch "upgrade_deps" to test it. For example, in pyproject.toml:

connexion = { git = "https://github.com/mfmarche/connexion", branch="upgrade_deps"}

I am also blocked on the upgrade. The move to uvicorn has implications for performance, since we heavily used gthreads (greenlets), so to achieve similiar performance, the WSGI app really needs to be fully async now.

@@ -9,7 +9,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.7, 3.8, 3.9]
python-version: ['3.9', '3.10', '3.11', '3.12']
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, since you are updating the list, I would include 3.13



class FlaskJSONEncoder(json.JSONEncoder):
def default(self, o):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please add a comment explaining why this tiny class must exist and override the default method?

mv
commands=
min: cp pyproject.toml .pyproject.toml
min: sed -i -E 's/"(\^|~|>=)([ 0-9])/"==\2/' pyproject.toml
Copy link
Contributor

@chrisinmtown chrisinmtown May 3, 2025

Choose a reason for hiding this comment

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

This file has no -min suffix for any of the pythons so the min: lines in this [testenv] section are not currently used. I was checking because this sed line does not work on MacOS, please see PR #2047 for a fix. You might want to consider whether that min test is important. I'm still trying to understand the intent of it.

@chrisinmtown
Copy link
Contributor

How should I run the tests? I ran tox and all tests passed, but I don't see any invocations of mypy, ruff, isort, flake8 or other code format and style checkers. That seems like a miss, but maybe I'm doing something wrong?

@chrisinmtown
Copy link
Contributor

chrisinmtown commented May 4, 2025

I don't see any invocations of mypy, ruff, isort, flake8 or other code format and style checkers.

I see the 3.x branch has file .pre-commit-config.yaml which is missing here. Also the required lines at the end of file tox.ini are commented out here. I copied over the yaml file and changed the tox lines. Then all the checks run at the end, and the result is 108 files are updated with minor format changes such as this:

diff --git a/tests/test_utils.py b/tests/test_utils.py
index 72c9e27..42e9921 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -7,14 +7,14 @@ from connexion import utils
 
 
 def test_get_function_from_name():
-    function = utils.get_function_from_name('math.ceil')
+    function = utils.get_function_from_name("math.ceil")
     assert function == math.ceil
     assert function(2.7) == 3

Please consider how much of this you want to incorporate. I tend to like the style standardization, especially in an open-source repo. But I realize I'm raising the bar here. You figured, just upgrade the dependencies, and here comes this guy who says you should reformat all the files lol. But with the immensely frustrating utter silence from the current maintainers, I'm viewing this PR as the brave new start of a hard fork for version 2.

@donbowman
Copy link

IMHO the best thing for the community would be to accept this PR onto the V2 branch, and publish as a new version on 2.x on connexion existing pypi, allowing those folks who are working on the major rework required for 3.x to work around the severe security issues that are currently pinned to v2.

Failing that, the next best alternative is to create a hard fork and keep it on the v2 compatibility, publish that to pypi as an alternate name. In which case we would need at least 2 maintainers who are independent of each other.

It would be nice to get some guidance from the maintainers as to which direction you prefer. If you are against the idea of updating the current v2 branch dependencies, then we can proceed with the hard fork. If its just a resourcing issue, is there something one of us can do to assist?

I believe @mfmarche and @chrisinmtown have done the work on the PR, so it really comes down to this question.

@advance512
Copy link

I agree with @donbowman. +1

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

Successfully merging this pull request may close these issues.

10 participants