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

Added dynamic macros into Jenkins config #20

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

patrikcerbak
Copy link
Collaborator

Added dynamic macros into Jenkins config

The main feature. Now, when setting links to the comparator tool in the Jenkins config, you can use macros which will correspond with the config items set in the section below.

So now, when you for example set a way to look for nvr in the Config items section of Jenkins config and you put nvr into the "Name of the item to find" field. You can use the macro %{nvr} in the comparator links section, which will translate into the nvr of the current job and build according to the report page.

Added a check for escaping the directory

I also added a basic check to stop potential nefarious users from trying to escape directory when using the --build-config-find/--job-config-find or the "Config items" setting in Jenkins. It checks with regex whether there is ../ in the path to the file and if so, it throws an exception.

Reworked comparator argument parsing

Till now, the --build-config-find arguments had to declared before using the dynamic argument itself (for example --nvr). Now it doesnt matter, you can use --nvr before declaring --build-config-find changelog.xml:nvr:/build/nvr.

Other

  • Fixed a bug where the dropdown menu in Jenkins config did not save the value.
  • Updated help message of comparator and Jenkins

@patrikcerbak patrikcerbak requested a review from a team as a code owner February 21, 2024 16:17
public static final int VAGUE_QUERY_LENGTH_THRESHOLD = 4;
public static final String ESCAPE_DIRECTORY_REGEX = "((\\.\\./)+.*(\\.\\./)*.*)|((\\.\\./)*.*(\\.\\./)+.*)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Isnt better to simply check, that the fully resolved filename, the final abs path (java have methods for that), is simply still in desired workspace/cwd/app dir?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wil merge this as it is. But the rework of shoudl be needed.
Again,. from my own experiments, checking input like this may be missleading.

eg correctdir/correctdir/../fileInFirstCorrectDir is pretty correct
Eg I'm checking the bash injection in the CGI wrapper by checking for "|" and thus literally killing this tool. Btrw, any ideas how to fix that "|" (https://github.com/jenkinsci/report-jtreg-plugin/blob/master/report-jtreg-service/src/main/java/io/jenkins/plugins/report/jtreg/main/web/ContextExecutingHandler.java#L114)

</p>
<p>
<i>So an example of the whole prompt with arguments can look like this:</i><br>
<code>
--compare<br>
--history 5<br>
--build-config-find changelog.xml:nvr:/build/nvr<br>
--nvr ".*"<br>
--nvr "%{nvr}"<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

it maybe worthy to keep also the --nvr ".*" example. Otherwise users may fell to assumption, that it is the final form, and no xtensions possible

@@ -199,7 +209,7 @@ private static Options.Configuration getConfiguration(String[] values, String cu
}

private static String getArgumentValue(String[] arguments, int i) {
if (i + 1 < arguments.length) {
if (i + 1 < arguments.length && !arguments[i + 1].matches("^--.*")) {
return arguments[i + 1];
} else {
throw new RuntimeException("Expected some value after " + arguments[i] + " argument.");
Copy link
Contributor

Choose a reason for hiding this comment

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

trust in you, I have, young padawan. However this is becaoming regex hell :) I once implemented mathematic formula parser based on split and repalce... not any more!

@judovana judovana merged commit 6c222f5 into jenkinsci:master Mar 1, 2024
13 checks passed
@patrikcerbak patrikcerbak deleted the dynamic-macros branch March 13, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants