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

build: bringing lxml latest version. #309

Closed
wants to merge 2 commits into from

Conversation

awais786
Copy link

Latest version of python3-saml downgrading lxml version.

@nijel
Copy link

nijel commented Jul 15, 2022

The restriction is there for a reason, see #292

On the other side, rebuilding from source seems to fix lxml behavior, so it's questionable whether this should be addressed by the dependency restrictions.

Anyway, there is now lxml 4.9.1 with security fixes (see GHSA-wrxv-2j5q-m38w).

nijel added a commit to WeblateOrg/docker that referenced this pull request Jul 15, 2022
Recent python3-saml versions block lxml upgrade, which in turn contains
security fixes for  GHSA-wrxv-2j5q-m38w.
The version restriction seems to be caused by binary wheel not
compatible with some distros (see
SAML-Toolkits/python3-saml#292). As we build lxml
from the source, we're not affected by this.

Once python3-saml raises the restriction (for example by
SAML-Toolkits/python3-saml#309), we can switch back
to the latest version.
@tahayk
Copy link

tahayk commented Jul 21, 2022

any idea when this will be merged ? we need the lxml security fix. thanks.

setup.py Outdated
@@ -39,7 +39,7 @@
},
test_suite='tests',
install_requires=[
'lxml<4.7.1',
'lxml<=4.9.0',
Copy link

@andersk andersk Jul 30, 2022

Choose a reason for hiding this comment

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

I think you meant lxml >= 4.9.0 or lxml ~= 4.9 or simply lxml? It makes no sense to allow 4.9.0 but disallow 4.9.1.

Copy link
Author

Choose a reason for hiding this comment

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

I am adding pin to avoid any breaking change in any upcoming release. Just update the pin as per new release.

Copy link

Choose a reason for hiding this comment

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

Pinning the version like this does not work well for projects using python3-saml. In order for a project to update lxml for a security update you have to wait for a new release of python3-saml. It is a trade off between ensuring that the specified version works and allowing flexibility for users. Ideally we would assume that future versions of lxml work correctly, and make a patch release rejecting certain releases if an issue is identified.

@nosnilmot
Copy link
Contributor

Alternative in #323 that attempts to address the original reason for pinning lxml version with an alternative workaround

@@ -39,7 +39,7 @@
},
test_suite='tests',
install_requires=[
'lxml<4.7.1',
'lxml<=4.9.1',

Choose a reason for hiding this comment

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

Ideally, python-saml would not be pinned quite this tight. To address the CVE, maybe switch back to:

Suggested change
'lxml<=4.9.1',
'lxml>=4.9.1',

@pitbulk
Copy link
Contributor

pitbulk commented Dec 25, 2022

Closing due #323

@pitbulk pitbulk closed this Dec 25, 2022
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.

None yet

8 participants