Skip to content

fix: CVE fixes, update werkzeug #1967

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

Closed
wants to merge 1 commit into from

Conversation

mfmarche
Copy link
Contributor

Updates werkzeug for CVE-2024-34069

Fixes # .

Changes proposed in this pull request:

Updates werkzeug for CVE-2024-34069

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

I opened issue #1969 for exactly this problem before finding this PR. Thanks for this! I hope the maintainers review and comment soon.

@@ -8,17 +8,20 @@ envlist =
{py37}-{min,pypi,dev}
{py38}-{min,pypi,dev}
{py39}-{min,pypi,dev}
{py310}-{min,pypi,dev}
isort-check
Copy link
Contributor

@chrisinmtown chrisinmtown Aug 29, 2024

Choose a reason for hiding this comment

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

Probably should add py311 also, please. I tried that, and that brings new problems. In Py3.11 the collections module had some breaking (?) changes

ImportError while loading conftest '/Users/me/git/github/connexion_mfmarche/tests/conftest.py'.
/Users/me/git/github/connexion-mfmarche/tests/conftest.py:7: in <module>
    ???
connexion/__init__.py:14: in <module>
    from .apis import AbstractAPI  # NOQA
connexion/apis/__init__.py:16: in <module>
    from .abstract import AbstractAPI  # NOQA
connexion/apis/abstract.py:14: in <module>
    from ..exceptions import ResolverError
connexion/exceptions.py:7: in <module>
    from jsonschema.exceptions import ValidationError
.tox/py311-min/lib/python3.11/site-packages/jsonschema/__init__.py:18: in <module>
    from jsonschema.validators import (
.tox/py311-min/lib/python3.11/site-packages/jsonschema/validators.py:8: in <module>
    import requests
.tox/py311-min/lib/python3.11/site-packages/requests/__init__.py:58: in <module>
    from . import utils
.tox/py311-min/lib/python3.11/site-packages/requests/utils.py:30: in <module>
    from .cookies import RequestsCookieJar, cookiejar_from_dict
.tox/py311-min/lib/python3.11/site-packages/requests/cookies.py:164: in <module>
    class RequestsCookieJar(cookielib.CookieJar, collections.MutableMapping):
E   AttributeError: module 'collections' has no attribute 'MutableMapping'

https://stackoverflow.com/questions/70943244/attributeerror-module-collections-has-no-attribute-mutablemapping

chrisinmtown

This comment was marked as outdated.

chrisinmtown

This comment was marked as outdated.

@@ -31,7 +34,10 @@ commands=
pypi: pip install --upgrade -r {toxworkdir}/requirements-pypi.txt
dev: requirements-builder --level=dev --extras aiohttp --req=requirements-devel.txt -o {toxworkdir}/requirements-dev.txt setup.py
dev: pip install --upgrade -r {toxworkdir}/requirements-dev.txt
python setup.py test
python setup.py pytest {posargs}
Copy link
Contributor

@chrisinmtown chrisinmtown Aug 30, 2024

Choose a reason for hiding this comment

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

There's a /lot/ going on in the tox setup/run commands that I don't understand, like dynamically creating requirements files such as requirements-min.txt. The tests get quite far but still fail with the old line (just test). The change from test to pytest changes the behavior significantly, and the tests still fail. From asking the cloud, I believe the invocation python setup.py is deprecated and has to be changed at this point bcos of this warning:

/Users/me/git/github/connexion_mfmarche/.tox/py311-min/lib/python3.11/site-packages/setuptools/__init__.py:81: _DeprecatedInstaller: setuptools.installer and fetch_build_eggs are deprecated.
!!

        ********************************************************************************
        Requirements should be satisfied by a PEP 517 installer.
        If you are using pip, you can try `pip install --use-pep517`.
        ********************************************************************************

!!
  dist.fetch_build_eggs(dist.setup_requires)

@@ -10,7 +10,8 @@

import flask
import werkzeug.exceptions
from flask import json, signals
from flask import signals
import json
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding this import on this line causes the check for properly sorted imports to fail ("isort"). Still trying to figure it out tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will move that around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, this is the position that isort accepted for me:

diff --git a/connexion/apps/flask_app.py b/connexion/apps/flask_app.py
index 6a7e61d..ada9c57 100644
--- a/connexion/apps/flask_app.py
+++ b/connexion/apps/flask_app.py
@@ -3,6 +3,7 @@ This module defines a FlaskApp, a Connexion application to wrap a Flask applicat
 """
 
 import datetime
+import json
 import logging
 import pathlib
 from decimal import Decimal
@@ -11,7 +12,6 @@ from types import FunctionType  # NOQA
 import flask
 import werkzeug.exceptions
 from flask import signals
-import json
 
 from ..apis.flask_api import FlaskApi
 from ..exceptions import ProblemException

@chrisinmtown
Copy link
Contributor

chrisinmtown commented Aug 30, 2024

Finally realized I had used a hyphen in the cloned directory name and that caused problems, sorry for my confusion. I installed python3.9, now tox runs without strange messages and shows these failures for Python3.9 tests. I think this is caused by using the wrong version of Werkzeug . I'm a little wrapped around an axle here, apparently version 3.0 removed the server_name argument, so I don't understand the failure, your change seems appropriate.

https://werkzeug.palletsprojects.com/en/3.0.x/test/#werkzeug.test.Client.set_cookie

========================================================= FAILURES ==========================================================
______________________________________________ test_cookie_param[swagger.yaml] ______________________________________________

simple_app = <connexion.apps.flask_app.FlaskApp object at 0x113f65d30>

    def test_cookie_param(simple_app):
        app_client = simple_app.app.test_client()
>       app_client.set_cookie(domain="localhost", key="test_cookie", value="hello")
E       TypeError: set_cookie() missing 1 required positional argument: 'server_name'

tests/api/test_parameters.py:526: TypeError
______________________________________________ test_cookie_param[openapi.yaml] ______________________________________________

simple_app = <connexion.apps.flask_app.FlaskApp object at 0x113680a60>

    def test_cookie_param(simple_app):
        app_client = simple_app.app.test_client()
>       app_client.set_cookie(domain="localhost", key="test_cookie", value="hello")
E       TypeError: set_cookie() missing 1 required positional argument: 'server_name'

tests/api/test_parameters.py:526: TypeError

@chrisinmtown
Copy link
Contributor

I solved the mypy complaint about a syntax error with this change

diff --git a/tests/api/test_responses.py b/tests/api/test_responses.py
index da76bbf..68b9322 100644
--- a/tests/api/test_responses.py
+++ b/tests/api/test_responses.py
@@ -393,29 +393,29 @@ def test_streaming_response(simple_app):
 def test_oneof(simple_openapi_app):
     app_client = simple_openapi_app.app.test_client()
 
-    post_greeting = app_client.post(  # type: flask.Response
+    post_greeting = app_client.post(
         '/v1.0/oneof_greeting',
         data=json.dumps({"name": 3}),
         content_type="application/json"
-    )
+    )  # type: flask.Response
     assert post_greeting.status_code == 200
     assert post_greeting.content_type == 'application/json'
     greeting_reponse = json.loads(post_greeting.data.decode('utf-8', 'replace'))
     assert greeting_reponse['greeting'] == 'Hello 3'
 
-    post_greeting = app_client.post(  # type: flask.Response
+    post_greeting = app_client.post(
         '/v1.0/oneof_greeting',
         data=json.dumps({"name": True}),
         content_type="application/json"
-    )
+    )  # type: flask.Response
     assert post_greeting.status_code == 200
     assert post_greeting.content_type == 'application/json'
     greeting_reponse = json.loads(post_greeting.data.decode('utf-8', 'replace'))
     assert greeting_reponse['greeting'] == 'Hello True'
 
-    post_greeting = app_client.post(  # type: flask.Response
+    post_greeting = app_client.post(
         '/v1.0/oneof_greeting',
         data=json.dumps({"name": "jsantos"}),
         content_type="application/json"
-    )
+    )  # type: flask.Response
     assert post_greeting.status_code == 400

@@ -25,7 +25,7 @@ def read_version(package):
'PyYAML>=5.1,<7',
'requests>=2.9.1,<3',
'inflection>=0.3.1,<0.6',
'werkzeug>=1.0,<2.3',
'werkzeug>=1.0,<4.0',
Copy link
Contributor

@chrisinmtown chrisinmtown Aug 31, 2024

Choose a reason for hiding this comment

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

since werkzeug versions below 3.x have security vulnerabilities, I suggest:

'werkzeug>=3.0,<4.0',

That change brings the next challenge:

ERROR: Cannot install -r /Users/me/git/github/connexion_mfmarche/.tox/requirements-min.txt (line 12) and MarkupSafe==0.23 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested MarkupSafe==0.23
    werkzeug 3.0.0 depends on MarkupSafe>=2.1.1

@chrisinmtown
Copy link
Contributor

@mfmarche I think this PR is exactly on point and will benefit many people including me, do you still have time to push this along?

@mfmarche
Copy link
Contributor Author

@mfmarche I think this PR is exactly on point and will benefit many people including me, do you still have time to push this along?

Hi @chrisinmtown, yes have every intention to push along. I will take your comments, thank you for providing those. However, i'm not sure if this will ever get pushed by upstream into the v2 branch? Do you have suggestions how to proceed?

@chrisinmtown
Copy link
Contributor

Do you have suggestions how to proceed?

Unfortunately for all of us connexion users, I have no suggestions. You might comment on this issue that I wrote, trying to get a reply from the new maintainers: #1973

'pytest-cov>=2,<3',
'testfixtures>=6,<7',
'pytest',

Choose a reason for hiding this comment

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

Three lines above the pytest dependency is set to pytest>=6,<9, is this unpinned duplicate setting a typo?

@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 has done the work on the PR, so it really comes down to this question.

@chrisinmtown
Copy link
Contributor

It would be nice to get some guidance from the maintainers as to which direction you prefer.

Does anyone know how to contact @RobbeSneyders and @Ruwann directly to see if they're doing OK, still working in software, still interested in being maintainers of this little project? If they have decided they no longer want to be involved, that would be useful information.

@mfmarche
Copy link
Contributor Author

mfmarche commented May 1, 2025

To add more confusion, I'm going to close this MR, as the work is now in #1992. @chrisinmtown could you refer to that MR, as that has the support for more modern python, in poetry.

@chrisinmtown
Copy link
Contributor

@mfmarche I'll try to keep up :)

@chrisinmtown
Copy link
Contributor

@mfmarche I don't mean to nag you, but you said you were closing this PR, yet it remains open?

@donbowman you might want to move your comment to PR #1992 ?

@mfmarche mfmarche closed this May 5, 2025
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.

4 participants