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

Remove werkzeug pin to versions < 2.4 #1171

Closed
do3cc opened this issue Oct 31, 2023 · 3 comments · Fixed by #1172
Closed

Remove werkzeug pin to versions < 2.4 #1171

do3cc opened this issue Oct 31, 2023 · 3 comments · Fixed by #1172

Comments

@do3cc
Copy link

do3cc commented Oct 31, 2023

There is a security issue in Werkzeug < 3.0.1 which triggers warnings for security checks.
Reading about the security issue, it is not relevant for the static pages. I did not check, whether it would be relevant for the dynamic editing frontend: GHSA-hrfv-mqp8-q5rw

The pull request that introduced the current pin of werkzeug<2.4 gives a good overview of why the Pin was needed and how big the effect on Plugins would be: #1144

However it also mentions another PR that addresses this and that PR has since been merged: #1143

I suspect that a werkzeug<2.4 isn't necessary any more but I haven't checked whether werkzeug 3 brought other changes that might require incorporation.

@dairiki
Copy link
Contributor

dairiki commented Oct 31, 2023

Master Branch

In the master branch, I've just tried removing the werkzeug<3 pin. All tests seem to pass, so I think the pin can be safely removed there.

I'm unsure as to whether we want to maintain a pin to prevent breakage by the next major (or even minor?) release of werkzeug. Flask and werkzeug do seem to deprecate and change functionality fairly quickly. (See this blog post and its followup for some discussion.)


Stable Branch

The stable branch (3.3-maintenance) does not contain PR #1143, so, as noted in #1144, more work would be required to avoid breaking publishing plugins.

I'm not sure this is worth the effort.
(Hopefully, the master branch will be released as stable sometime reasonably soon which will make this a non-issue.)


Security Considerations

As you say, the CVE does not affect the built static pages.

It could be an issue if Lektor's admin GUI were made accessible to untrusted parties. That is a use case that we don't explicitly support. Even so, in any reasonable public deployment of the admin GUI should be via some sort of HTTP proxy (e.g. nginx) or WSGI server (other than flask). If that proxy is appropriately configured, I think the potential for DOS is minimal. E.g. the proxy should already be configured to:

  • only allow access by trusted clients
  • limit maximum POST data size to some reasonable limit

dairiki added a commit to dairiki/lektor that referenced this issue Oct 31, 2023
@do3cc
Copy link
Author

do3cc commented Nov 4, 2023

I've tried your branch locally with the

  • lektor-atom
  • lektor-markdown-highlighter
  • lektor-webpack-support

Plugin. I saw no issues on the local server.
My take on a version pin is hugely influenced by Hyneks thoughts he blogged about on: https://hynek.me/articles/semver-will-not-save-you/
Where he makes a case against such version pins if not needed for applications.

@dairiki
Copy link
Contributor

dairiki commented Nov 4, 2023

My take on a version pin is hugely influenced by Hyneks thoughts he blogged about on: https://hynek.me/articles/semver-will-not-save-you/
Where he makes a case against such version pins if not needed for applications.

I mostly agree.

The issue is a bit murky for Lektor which is mostly an "application" (albeit distributed via PyPI package) rather than a "library". It is frustrating for users when a (given version) of Lektor stops working because of a dependency update. As you point out, though, loose pins do not definitively solve the problem. And there is no (straight-forward) way to distribute a package with "locked" dependencies. (Such locks would be python-version, ABI, and platform dependent, anyway.)

dairiki added a commit that referenced this issue Nov 4, 2023
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 a pull request may close this issue.

2 participants