-
Notifications
You must be signed in to change notification settings - Fork 26
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
Use standard java regex rather than commons-text string-substitutor #416
Conversation
Generate changelog in
|
@@ -6,7 +6,6 @@ dependencies { | |||
api 'com.google.guava:guava' | |||
api 'com.fasterxml.jackson.core:jackson-databind' | |||
api 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml' | |||
api 'org.apache.commons:commons-text' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically a break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth the possibility of someone having used this. Hopefully our tooling would have made them put a direct dep on it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I considered making this PR but keeping the commons-text dep as runtimeOnly
to avoid abi breaks, but I don't think it's worth the trouble because it retains the entirety of the downside without changing much from our current behavior.
public final class DecryptingVariableSubstitutor extends StringSubstitutor implements Substitutor { | ||
public final class DecryptingVariableSubstitutor implements Substitutor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used internally in our biggest monorepo, our auth product, atlas and a few other places :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should try and get them off that first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In atlas we don't rely on this class being a StringSubstitutor, it's only passed to JsonNodeStringReplacer
(from this library) as a Substitutor
(our own interface).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, all the other uses are like that too, so I guess we're good!
👍 |
76ae607
to
6cc6f13
Compare
Refusing to release a major rev - please do this manually at https://autorelease.general.dmz.palantir.tech/api/palantir/encrypted-config-value |
==COMMIT_MSG==
Use standard java regex rather than commons-text string-substitutor
==COMMIT_MSG==
Note that this project wasn't impacted by
CVE-2022-42889
due to providing its own substitution function as opposed to the default, however in our investigation it became clear that this library is simple enough that it should not use such complex and powerful dependencies.