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 commons-io dependency #95

Merged
merged 1 commit into from
Aug 26, 2020
Merged

remove commons-io dependency #95

merged 1 commit into from
Aug 26, 2020

Conversation

Jaxsun
Copy link
Contributor

@Jaxsun Jaxsun commented Aug 20, 2020

Changes

Removes the dependency on Apache commons-io.

IQ Server reports a vulnerability in commons-io relating to https://issues.apache.org/jira/browse/IO-559
Initially I was going to send a PR with a version bump similar to #26
However in checking whether the jwks-rsa library used the affected class from commons-io I noticed that the library doesn't appear to use commons-io at all. The build and test passes when removing the dependency so I figured it would be worth contributing that removal to avoid unnecessary dependencies and churn based on vulns or other issues with the dependency.

If I am wrong and this dependency is needed I can happily contribute a version bump instead.

References

https://issues.apache.org/jira/browse/IO-559
#26

Testing

I am unsure what testing other than the existing build and test is needed by maintainers to validate this change.

  • This change adds test coverage (no new functionality to test)
  • This change has been tested on the latest version of Java or why not (no new functionality to test)

Checklist

@Jaxsun Jaxsun requested a review from a team August 20, 2020 21:11
@lbalmaceda lbalmaceda added the needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue label Aug 20, 2020
@lbalmaceda lbalmaceda self-assigned this Aug 20, 2020
@Jaxsun
Copy link
Contributor Author

Jaxsun commented Aug 25, 2020

Hi @lbalmaceda, just checking in to see if you've had a chance to take a look at this yet or not.
Thanks for the review!

@lbalmaceda
Copy link
Contributor

lbalmaceda commented Aug 25, 2020

👋 Hi @Jaxsun. I won't be able to take a look at it until the next week. It does look like a valid removal, as the CI is not complaining about anything. But I'd like to double-check locally before merging this.

For our own reference, we're tracking this internally under SDK-1918.

@Jaxsun
Copy link
Contributor Author

Jaxsun commented Aug 25, 2020

Thanks for the update!

@lbalmaceda lbalmaceda added CH: Removed and removed needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue labels Aug 26, 2020
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Checked locally, it looks good.

@lbalmaceda lbalmaceda merged commit bf27250 into auth0:master Aug 26, 2020
@lbalmaceda lbalmaceda added this to the v0-Next milestone Aug 26, 2020
@lbalmaceda
Copy link
Contributor

lbalmaceda commented Aug 26, 2020

I can cut a release by the end of the week.
Edit: 0.13.0

@lbalmaceda lbalmaceda modified the milestones: v0-Next, 0.13.0 Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants