search

jenkins-infra / repository-permissions-updater

Artifactory permissions synchronization tool and data set
77 stars 1.03k forks source link

report-jtreg #3351

Closed judovana closed 1 year ago

judovana commented 1 year ago

Repository URL

https://github.com/judovana/jenkins-report-jtreg

New Repository Name

report-jtreg-plugin

Description

This plugin is built on top of chart.js api - https://github.com/jenkins-infra/repository-permissions-updater/issues/3172 It was following msot of the steps from https://github.com/jenkins-infra/repository-permissions-updater/issues/3221 and https://github.com/jenkins-infra/repository-permissions-updater/issues/3222; hopefully correctly.

It is comaptible with junit/xunit/jtreg/jck and tck results. Is optimised for suites with 1 000 000 and more results and on is known to be super fast on 10M testbases. If there are queries the optimised plugin view do not answer, it includes cli, able to blazingly quickly do various comparisons and dig more informations out of the results.

GitHub users to have commit permission

@judovana @pmikova @RadekCap @zzambers @andrlos

Jenkins project users to have release permission

judovana

Issue tracker

GitHub issues

jenkins-cert-app commented 1 year ago

Security audit, information and commands

The security team is auditing all the hosting requests, to ensure a better security by default.

This message informs you that a Jenkins Security Scan was triggered on your repository. It takes ~10 minutes to complete.

Commands The bot will parse all comments, and it will check if any line start with a command. Security team only:
  • /audit-ok => the audit is complete, the hosting can continue :tada:.
  • /audit-skip => the audit is not necessary, the hosting can continue :tada:.
  • /audit-findings => the audit reveals some issues that require corrections :pencil2:.
Anyone:
  • /request-security-scan => the findings from the [Jenkins Security Scan](https://www.jenkins.io/doc/developer/security/scan/) were corrected, this command will re-scan your repository :mag:.
  • /audit-review => the findings from the audit were corrected, this command will ping the security team to review the findings :eyes:. It's only applicable when the previous audit required changes.
Only one command can be requested per comment.

(automatically generated message, version: 1.17.22)

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(code review, README content, etc) and process the request as quickly as possible. Thank you for your patience.

Hosting team members can host this request with /hosting host

jenkins-cert-app commented 1 year ago

The Jenkins Security Scan discovered 2 finding(s) :mag:. For each of them, either apply the recommended correction, suppress the warning or provide a justification.

Once you're done, either re-run the scan with /request-security-scan or request the Security team to review your justifications with /audit-review.

Remoting: Unsafe Callable

You can find detailed information about this finding here.

ReportParserCallable.java#82 ``` Potentially unsafe Callable implementation ReportParserCallable#checkRoles ```

Jenkins: Generally unsafe method calls

You can find detailed information about this finding here.

ArgumentsParsing.java#12 ``` Potentially unsafe invocation of System#exit ```
judovana commented 1 year ago

/request-security-scan

jenkins-cert-app commented 1 year ago

The Jenkins Security Scan discovered 1 finding(s) :mag:. For each of them, either apply the recommended correction, suppress the warning or provide a justification.

Once you're done, either re-run the scan with /request-security-scan or request the Security team to review your justifications with /audit-review.

Remoting: Unsafe Callable

You can find detailed information about this finding here.

ReportParserCallable.java#82 ``` Potentially unsafe Callable implementation ReportParserCallable#checkRoles ```
judovana commented 1 year ago

/request-security-scan

jenkins-cert-app commented 1 year ago

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! :tada:

:bulb: The Security team recommends that you are setting up the scan in your repository by following our guide.

judovana commented 1 year ago

Gosh... the adjuncts are not done....wip

alecharp commented 1 year ago

Hello, while reviewing your plugin, I have some comments on it:

judovana commented 1 year ago

Hello, while reviewing your plugin, I have some comments on it:

Hi! thanx for the hints!

* why do you need Jenkins `2.407` (https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/pom.xml#L19). It would be probably better to rely on an LTS version

There was indeed LTS which was not compatible with this plugin. In that time I switched to STS. Its year or two old issue, and We are currently back on LTS, so It will be ok to move. to LTS.

* it is probably better to have a specific version for `woodstox-core` rather than `LATEST` https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/pom.xml#LL131C22-L131C28

This is similar issue. I got breakages caused by woodstox updates, so I was rather in mode to fix them immediately. But it is indeed in past. I do not have strong opinion for LATEST or simply latest stable bumped by depnda bot.

* I don't really understand why you have the profiles in https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/pom.xml#L51-L102. Could you explain the use case for the `jar` profile?

The codebase have two main methods. If you built it as a jar, they serve as CLI able to access the local (this)plugin metadata and resolve complex queries. They can run also as service, and then you can setup the plugin to use those services directly from jenkins.

Although in jenkins settings this is just service's url to set up, it is not trivial to set the services running. The cli/services are using 100% of plugin codebase and are connectable from jenkins so should remain in close neighbourhood. However they are cli/service, not easy to deploy, and may be exploitable if deplyed wrongly. So they shoudl be at least a bit separated at least on field of maven modules to plugin, lib and services. I never found cycles to properly cut this dilema.

* https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/.github/workflows/release-drafter.yml#L7-L8 and https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/.github/workflows/jenkins-security-scan.yml#L7-L8 which default branch will you be using?

Tbh I was not aware of this record. Its master now if I see correclty. Will it be master after move to jenkins, or it will become main?

* I understand this is a legacy thing but most of the classes have a problematic license header: https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/src/main/java/io/jenkins/plugins/report/jtreg/JckStyles.java#L4 for example.

I do not have strong prefference on this. I can do any of following:

I would vote for b) - removal of the headers, and keeping just License file as it is only fixing "Copyright (c) 2016 Stanislav Baiduzhyi" by "Copyright (c) 2016-2023 report-jtreg contributors"

Thanx! J.

judovana commented 1 year ago 
* why do you need Jenkins `2.407` (https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/pom.xml#L19). It would be probably better to rely on an LTS version

I have downgraded it, But I htink Wrongly. What is proepr number here?

* it is probably better to have a specific version for `woodstox-core` rather than `LATEST` https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/pom.xml#LL131C22-L131C28

fixed.

* https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/.github/workflows/release-drafter.yml#L7-L8 and https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/.github/workflows/jenkins-security-scan.yml#L7-L8 which default branch will you be using?

Fixed

* I understand this is a legacy thing but most of the classes have a problematic license header: https://github.com/judovana/jenkins-report-jtreg/blob/a4cea683ed2c1f7b86485a1ed5f9c7f3c2108e76/src/main/java/io/jenkins/plugins/report/jtreg/JckStyles.java#L4 for example.

Fixed so it be at least not wrong.

One adjunct done, second to go. TY!

judovana commented 1 year ago

Both adjuncts done . blacklist/whitelist in progress

judovana commented 1 year ago

whitelist and balcklist replaced by allowlist and deny list. I think the plugin is now ready for review. TY!

judovana commented 1 year ago

/request-security-scan

judovana commented 1 year ago

/hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(code review, README content, etc) and process the request as quickly as possible. Thank you for your patience.

Hosting team members can host this request with /hosting host

jenkins-cert-app commented 1 year ago

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! :tada:

:bulb: The Security team recommends that you are setting up the scan in your repository by following our guide.

NotMyFault commented 1 year ago

Otherwise LGTM, I'd say.

Edit: It's not. https://github.com/judovana/jenkins-report-jtreg/tree/master/src/main/resources/io/jenkins/plugins/report/jtreg/main/web violates the content security policy. I recommend refactoring the files and outsourcing all <script> tags to dedicated .js files, loading them directly. Inline block contents are banned.

Please revise affected areas according to https://www.jenkins.io/doc/developer/security/csp/#statically-identifying-affected-code-and-plugins. This contains banned occurrences of onclick etc in jelly files.

judovana commented 1 year ago

renamed, ty.

Edit: It's not. https://github.com/judovana/jenkins-report-jtreg/tree/master/src/main/resources/io/jenkins/plugins/report/jtreg/main/web violates the content security policy. I recommend refactoring the files and outsourcing all <script> tags to dedicated .js files, loading them directly. Inline block contents are banned.

Those two files are not accessible from plugin. Only way how to reach them is to build the cli via -Pjar and then wrap that cli as CGI service. Do yo still insits on separating the JS?

NotMyFault commented 1 year ago

/hosting re-check

NotMyFault commented 1 year ago

/request-security-scan

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(code review, README content, etc) and process the request as quickly as possible. Thank you for your patience.

Hosting team members can host this request with /hosting host

jenkins-cert-app commented 1 year ago

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! :tada:

:bulb: The Security team recommends that you are setting up the scan in your repository by following our guide.

NotMyFault commented 1 year ago

/hosting host

NotMyFault commented 1 year ago

renamed, ty.

Edit: It's not. https://github.com/judovana/jenkins-report-jtreg/tree/master/src/main/resources/io/jenkins/plugins/report/jtreg/main/web violates the content security policy. I recommend refactoring the files and outsourcing all <script> tags to dedicated .js files, loading them directly. Inline block contents are banned.

Those two files are not accessible from plugin. Only way how to reach them is to build the cli via -Pjar and then wrap that cli as CGI service. Do yo still insits on separating the JS?

All good

jenkins-infra-bot commented 1 year ago

Hosting request complete, the code has been forked into the jenkinsci project on GitHub as https://github.com/jenkinsci/report-jtreg-plugin

GitHub issues has been selected for issue tracking and was enabled for the forked repo.

A pull request has been created against the repository permissions updater to setup release permissions. Additional users can be added by modifying the created file.

Please delete your original repository (if there are no other forks), under 'Danger Zone', so that the jenkinsci organization repository is the definitive source for the code. If there are other forks, please contact GitHub support to make the jenkinsci repo the root of the fork network (mention that Jenkins approval was given in support request 569994). Also, please make sure you properly follow the documentation on documenting your plugin so that your plugin is correctly documented.

You will also need to do the following in order to push changes and release your plugin:

In order for your plugin to be built by the Jenkins CI Infrastructure and check pull requests, please add a Jenkinsfile to the root of your repository with the following content: https://github.com/jenkinsci/archetypes/blob/master/common-files/Jenkinsfile

Welcome aboard!

judovana commented 1 year ago

ty!