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

html.validator: Remove dependency on log4j #5716

Conversation

matthiasblaesing
Copy link
Contributor

This commit modifies the patched version of the html.validator not to rely on log4j anymore.

There is no security gain estimated. This commit exists only to silence "security" teams, that have some strange interpretation of security. They assume, that you can deduce from the presence of a dependency/ library, whether or not there is a security problem. There is no evidence, that any of the CVEs against log4j 1 apply here.

@matthiasblaesing matthiasblaesing added dependencies Pull requests that update a dependency file Upgrade Library Library (Dependency) Upgrade HTML [ci] enable web job ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Mar 25, 2023
@matthiasblaesing matthiasblaesing added this to the NB18 milestone Mar 25, 2023
@matthiasblaesing matthiasblaesing added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Mar 25, 2023
@matthiasblaesing
Copy link
Contributor Author

This PR is currently for only for review. Before merge the binary will be moved to the OSUOL server.

@mbien
Copy link
Member

mbien commented Mar 25, 2023

patch looks good. An alternative approach for something like this could be to use SLF4J bridges, since the project has a bridge for almost everything. In this particular case:

log4j-over-slf4j + slf4j-jdk14

this would map log4j 1 to slf4j and use the weirdly named slf4j-jdk14 impl as "logging impl", which is actually a bridge to java.util.logging.

https://www.slf4j.org/legacy.html#log4j-over-slf4j +
https://www.slf4j.org/manual.html#projectDep (scrolll down to slf4j-jdk14)

edit:
link to upstream issue: validator/validator#1313

@matthiasblaesing matthiasblaesing force-pushed the html_validation_without_log4j branch from a756d7d to f39dbba Compare March 25, 2023 14:02
@matthiasblaesing
Copy link
Contributor Author

@mbien ah - nice idea. Would be good to consider if we can get the patching down or updated.

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Looks good!

@matthiasblaesing matthiasblaesing added do not merge Don't merge this PR, it is not ready or just demonstration purposes. and removed do not merge Don't merge this PR, it is not ready or just demonstration purposes. labels Apr 1, 2023
This commit modifies the patched version of the html.validator not to
rely on log4j anymore.

There is no security gain estimated. This commit exists only to silence
"security" teams, that have some strange interpretation of security.
They assume, that you can deduce from the presence of a dependency/
library, whether or not there is a security problem. There is no
evidence, that any of the CVEs against log4j 1 apply here.
@matthiasblaesing matthiasblaesing force-pushed the html_validation_without_log4j branch from f39dbba to 0a7374e Compare April 1, 2023 13:26
@matthiasblaesing matthiasblaesing removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Apr 1, 2023
@matthiasblaesing
Copy link
Contributor Author

Pushed new binary to the OSUOSL server and updated the reference. Will merge tomorrow once tests are green.

@matthiasblaesing matthiasblaesing merged commit ca2b812 into apache:master Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) dependencies Pull requests that update a dependency file HTML [ci] enable web job Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants