Filter build-info and modules properties#716
Merged
Or-Geva merged 7 commits intojfrog:masterfrom Mar 6, 2023
Merged
Conversation
yahavi
requested changes
Mar 5, 2023
Member
yahavi
left a comment
There was a problem hiding this comment.
Please don't merge this code before my final approval. I want to go through it again.
build-info-extractor/src/main/java/org/jfrog/build/extractor/BuildInfoExtractorUtils.java
Outdated
Show resolved
Hide resolved
build-info-extractor/src/main/java/org/jfrog/build/extractor/BuildInfoExtractorUtils.java
Outdated
Show resolved
Hide resolved
...fo-extractor/src/main/java/org/jfrog/build/extractor/clientConfiguration/PatternMatcher.java
Outdated
Show resolved
Hide resolved
...fo-extractor/src/main/java/org/jfrog/build/extractor/clientConfiguration/PatternMatcher.java
Show resolved
Hide resolved
...fo-extractor/src/main/java/org/jfrog/build/extractor/packageManager/PackageManagerUtils.java
Show resolved
Hide resolved
...xtractor/src/test/java/org/jfrog/build/extractor/packageManager/PackageManagerUtilsTest.java
Outdated
Show resolved
Hide resolved
...xtractor/src/test/java/org/jfrog/build/extractor/packageManager/PackageManagerUtilsTest.java
Outdated
Show resolved
Hide resolved
...fo-extractor/src/main/java/org/jfrog/build/extractor/packageManager/PackageManagerUtils.java
Outdated
Show resolved
Hide resolved
| private static final String apiKeySecretPrefix = "AKCp8"; | ||
| private static final int apiKeySecretMinimalLength = 73; | ||
| private static final String referenceTokenSecretPrefix = "cmVmdGtuOjAxOj"; | ||
| private static final int referenceTokenSecretMinimalLength = 64; |
Member
There was a problem hiding this comment.
I almost sure that reference tokens are 64 characters fixed
| } | ||
|
|
||
| public static Properties getEnvProperties(Properties startProps, Log log) { | ||
| IncludeExcludePatterns patterns = new IncludeExcludePatterns( |
Member
There was a problem hiding this comment.
Removing the filtering code from this method sounds very risky to me. Let's do the "get" and the "filter" logic in one place to enforce filtering in all flows.
Contributor
Author
There was a problem hiding this comment.
I understand the concern, but this is not the place to do filtering. According to the name of this function, it returns all environment variables. By moving the filter logic to a new location, we can filter not only the environment variables, but also modules. Putting the filter logic here would prevent us from filtering things unrelated to environment variables, so we would have to split the logic across the code, and that will cause a mess in the code.
Contributor
Author
There was a problem hiding this comment.
Due to security risks, we will return the filter logic to here despite the above comment .
yahavi
reviewed
Mar 5, 2023
.../src/main/groovy/org/jfrog/gradle/plugin/artifactory/extractor/GradleBuildInfoExtractor.java
Show resolved
Hide resolved
yahavi
requested changes
Mar 6, 2023
.../src/main/groovy/org/jfrog/gradle/plugin/artifactory/extractor/GradleBuildInfoExtractor.java
Outdated
Show resolved
Hide resolved
...xtractor/src/test/java/org/jfrog/build/extractor/packageManager/PackageManagerUtilsTest.java
Outdated
Show resolved
Hide resolved
yahavi
reviewed
Mar 6, 2023
...fo-extractor/src/main/java/org/jfrog/build/extractor/packageManager/PackageManagerUtils.java
Outdated
Show resolved
Hide resolved
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prior to this PR, build-info properties were filtered only for environment variables and system properties.
The PR moves all filter logic into one place and performs exclude/include logic for all properties, including:
buildInfo.envprefix)