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

chore: bump werkzeug and Flask #23965

Merged
merged 27 commits into from May 12, 2023

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented May 8, 2023

SUMMARY

Bump Flask and Werkzeug

https://nvd.nist.gov/vuln/detail/CVE-2023-30861

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 8, 2023
@EugeneTorap
Copy link
Contributor

Hi @dpgaspar! We have a Mypy configuration problem in #23927
Do you know how to fix it?

Basically the problem is that some dependencies (e.g. Flask since version 2.0) have stubs include in their main distribution, while other dependencies require stubs to be installed vie the typeshed distribution. (i.e. the 'types-*' packages from PyPI).

@dpgaspar
Copy link
Member Author

dpgaspar commented May 8, 2023

Hi @dpgaspar! We have a Mypy configuration problem in #23927 Do you know how to fix it?

Basically the problem is that some dependencies (e.g. Flask since version 2.0) have stubs include in their main distribution, while other dependencies require stubs to be installed vie the typeshed distribution. (i.e. the 'types-*' packages from PyPI).

not yet

@@ -309,7 +310,7 @@ vine==5.0.0
# kombu
wcwidth==0.2.5
# via prompt-toolkit
werkzeug==2.1.2
werkzeug==2.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you to pin werkzeug in setup.py because we use this lib in superset project directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, makes sense

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #23965 (d76eabc) into master (4a828f5) will decrease coverage by 11.20%.
The diff coverage is n/a.

❗ Current head d76eabc differs from pull request most recent head 5f80dcf. Consider uploading reports for the commit 5f80dcf to get more accurate results

@@             Coverage Diff             @@
##           master   #23965       +/-   ##
===========================================
- Coverage   68.18%   56.98%   -11.20%     
===========================================
  Files        1941     1941               
  Lines       75261    75261               
  Branches     8168     8168               
===========================================
- Hits        51317    42888     -8429     
- Misses      21855    30284     +8429     
  Partials     2089     2089               
Flag Coverage Δ
hive 53.18% <ø> (ø)
mysql ?
presto 53.10% <ø> (ø)
python 59.60% <ø> (-23.11%) ⬇️
sqlite ?
unit 53.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 306 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EugeneTorap
Copy link
Contributor

EugeneTorap commented May 8, 2023

@cwegener @dpgaspar I found that mypy PR python/mypy#10652 which resolves flask & werkzeug typing problems.
@dpgaspar Can you try to upgrade the mypy version?

@EugeneTorap
Copy link
Contributor

@cwegener @dpgaspar Ohh we already has v1.0.1 mypy but it downloads flask-stubs because we use additional_dependencies: [types-all] in .pre-commit-config.yaml file

@cwegener
Copy link
Contributor

cwegener commented May 8, 2023

Hi @dpgaspar! We have a Mypy configuration problem in #23927 Do you know how to fix it?

Basically the problem is that some dependencies (e.g. Flask since version 2.0) have stubs include in their main distribution, while other dependencies require stubs to be installed vie the typeshed distribution. (i.e. the 'types-*' packages from PyPI).

I'm actually not very concerned about the static type checking at the moment. I think it's a lower priority.
Getting the Async Query Runner working with Flask 2.2 is currently the main blocker for migrating to Flask 2.2

@cwegener
Copy link
Contributor

cwegener commented May 8, 2023

@cwegener @dpgaspar Ohh we already has v1.0.1 mypy but it downloads flask-stubs because we use additional_dependencies: [types-all] in .pre-commit-config.yaml file

Yes. The solution for this particular mypy issue should be to remove the types-all dependency in the pre-commit config and replace it with the curated, explicit list of all desired python type stubs packages that we know are required to successfully run static type checks.

@cwegener
Copy link
Contributor

cwegener commented May 8, 2023

Would it be better to go to Flask 2.2 before migrating to Flask 2.3?

@pull-request-size pull-request-size bot added size/L and removed size/M labels May 9, 2023
@@ -33,6 +33,7 @@ def fetch_events(self, last_id: Optional[str] = None):

@mock.patch("uuid.uuid4", return_value=UUID)
def test_events(self, mock_uuid4):
app._got_first_request = False
Copy link
Member Author

@dpgaspar dpgaspar May 11, 2023

Choose a reason for hiding this comment

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

a bit of an hack, async_query_manager.init_app(app) registers a Flask app.before_request. New Flask will only allow registering before request hooks if no request was yet processed by the app.

@@ -1188,10 +1193,10 @@ def test_data_cache_default_timeout(


def test_chart_cache_timeout(
load_energy_table_with_slice: List[Slice],
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an interesting case that surfaced on this new Flask version. Flask now is stricter, so when popping a new context from the app stack it checks if the context is the one we are expecting. load_energy_table_with_slice creates a new app_context so it needs to be first

@dpgaspar
Copy link
Member Author

Would it be better to go to Flask 2.2 before migrating to Flask 2.3?

Flask is on 2.2

@dpgaspar
Copy link
Member Author

Wow, that's impressive work! Now we need to fix the flask-stubs issue. We use additional_dependencies: [types-all] in .pre-commit-config.yaml file which downloads the old flask-stubs.

yep, thank you once more for doing that on: #24033

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, pretty big changes needed, thanks for all the work here 👍

@dpgaspar dpgaspar merged commit 97482c5 into apache:master May 12, 2023
26 checks passed
@dpgaspar dpgaspar deleted the chore/flask-vulnerability branch May 12, 2023 14:01
@EugeneTorap EugeneTorap mentioned this pull request May 12, 2023
eschutho pushed a commit that referenced this pull request May 25, 2023
eschutho pushed a commit that referenced this pull request Dec 2, 2023
@villebro villebro mentioned this pull request Feb 20, 2024
9 tasks
@mistercrunch mistercrunch added 🍒 2.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L v2.1 🍒 2.1.3 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants