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

upd: migrate log4j to log4j2 to fix vulnerability on log4j 1 #352

Merged
merged 3 commits into from
Jan 17, 2020

Conversation

jcharlet
Copy link
Contributor

update dependencies and configuration for logging to log4j2
to fix new vulnerability on log4j 1 which is preventing builds to succeed on CI.

@jcharlet jcharlet force-pushed the upgrade_log4j branch 3 times, most recently from 8d79204 to c7e8788 Compare January 13, 2020 15:28
@jcharlet
Copy link
Contributor Author

Hi all, if someone could review that PR, that would fix the vulnerability issue we have on all other builds, and help me move on. Thanks!

@adamretter
Copy link
Contributor

Hi @jcharlet there seems to be a huge amount of changes in this due to reformatting or something. Unfortunately for many but not all files in this PR that makes it impossible to pick out the intentional changes.

If its not a huge amount of work, is it possible to get a PR where only the necessary code changes are made? or the reformatting changes are isolated in their own commit separately to the desired logging changes.

@jcharlet
Copy link
Contributor Author

Hi @jcharlet there seems to be a huge amount of changes in this due to reformatting or something. Unfortunately for many but not all files in this PR that makes it impossible to pick out the intentional changes.

If its not a huge amount of work, is it possible to get a PR where only the necessary code changes are made? or the reformatting changes are isolated in their own commit separately to the desired logging changes.

Hi @adamretter , sure thanks, will do

@jcharlet
Copy link
Contributor Author

argh @adamretter there was no reformatting issue, it's because I removed log4j.properties files and created log4j2.properties files. And as it considers them as different files, it doesn't try to show the diff between them.

I updated the commits, so that I initially commit properties files with renaming them.
But changes are so big, github shows files one after another.

A bit easier to compare in intellij though..
Screenshot from 2020-01-16 11-31-13

Could that be satisfying? I agree it's a large change, all properties files were updated, and syntax changed from log4j 1 to log4j2..

@jcharlet
Copy link
Contributor Author

@adamretter I'll save you some time, we have a new developer Saurab who's available to review the PR :), will see that with him!

@jcharlet jcharlet requested review from sparkhi and removed request for adamretter and ian-hoyle January 16, 2020 10:46
@sparkhi
Copy link
Collaborator

sparkhi commented Jan 16, 2020

The email id in the source header should be corrected. It is not strictly part of this issue, so it should be addressed on its own with corrections in all places where it appears in the codebase, also look to see if it forces a copyright year update

@sparkhi sparkhi closed this Jan 16, 2020
@sparkhi sparkhi reopened this Jan 16, 2020
@jcharlet jcharlet merged commit 580aeee into digital-preservation:master Jan 17, 2020
@jcharlet jcharlet deleted the upgrade_log4j branch January 17, 2020 08:13
@jcharlet jcharlet mentioned this pull request Jan 29, 2020
@jcharlet jcharlet added the security Pull requests that address a security vulnerability label Feb 12, 2020
@jcharlet jcharlet added this to the 6.5 milestone Feb 12, 2020
@jcharlet jcharlet self-assigned this Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants