Modernize plugin: Jenkins 2.541.3/Java 17 baseline, tunnel isolation, secret masking#13
Conversation
… secret masking - Raise baseline: parent 5.31, jenkins.version 2.541.3, bom-2.541.x, Java 17; drop java.level and findbugs.skip - Fix per-build tunnel isolation: per-execution App keyed by identifier (no more shared static tunnel races) - Mask the TestingBot secret in build console logs (ConsoleLogFilter) - Guard the build-summary RunListener against NPE/IOOBE on non-TestingBot / mixed suites - Honor tunnel options; expose SELENIUM_HOST/PORT and TESTINGBOT_TUNNEL_IDENTIFIER env vars - Add @post + permission checks to credential form endpoints; drop @exported on getSecret() - Replace deprecated APIs: ACL.SYSTEM2, lookupCredentialsInItem, commons-cli DefaultParser/Option.builder, Guava - Extract shared TunnelManager; remove dead code (vestigial builder, TunnelConfig) - Add unit tests, GitHub Actions CI/CD (JEP-229) and Dependabot
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe plugin updates its Jenkins and Maven baselines, adds CI/CD automation, centralizes TestingBot tunnel management, isolates concurrent pipeline tunnels, hardens credential and reporting paths, masks secrets in logs, and removes obsolete builder configuration. ChangesTestingBot Jenkins plugin modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant TestingBotTunnelStepExecution
participant TunnelManager
participant RUNNING_TUNNELS
participant TestingBotTunnel
Pipeline->>TestingBotTunnelStepExecution: execute tunnel step
TestingBotTunnelStepExecution->>TunnelManager: start credentials, options, and unique identifier
TunnelManager->>TestingBotTunnel: boot and poll for READY
TunnelManager-->>RUNNING_TUNNELS: store running App
TestingBotTunnelStepExecution-->>Pipeline: provide tunnel and Selenium environment
Pipeline->>TestingBotTunnelStepExecution: body completes
TestingBotTunnelStepExecution->>RUNNING_TUNNELS: remove matching App
TestingBotTunnelStepExecution->>TunnelManager: stop App
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cd.yaml:
- Line 14: The workflow references in .github/workflows/cd.yaml lines 14-14 and
.github/workflows/ci.yaml lines 18-20 use mutable tags; replace the reusable
Maven workflow, actions/checkout, and actions/setup-java references with their
corresponding full commit SHAs, preserving the existing workflow configuration.
- Around line 8-10: Update the reusable workflow reference in the CD workflow to
use an immutable commit SHA instead of the mutable `@v1` tag. Preserve the
referenced maven-cd.yml workflow and existing permissions while pinning it to
the approved commit ref.
In @.github/workflows/ci.yaml:
- Line 18: Update the actions/checkout@v4 step in the CI workflow to set
persist-credentials to false, ensuring the checkout action does not retain the
GitHub token in local Git configuration.
In `@README.md`:
- Around line 74-81: Update the README environment-variable documentation to
state that testingbot blocks expose only TESTINGBOT_KEY/TB_KEY and
TESTINGBOT_SECRET/TB_SECRET, while SELENIUM_HOST, SELENIUM_PORT, and
TESTINGBOT_TUNNEL_IDENTIFIER are available exclusively inside testingbotTunnel
blocks; adjust the surrounding tunnel-isolation text to preserve the accurate
behavior description.
In `@src/main/java/testingbot/pipeline/TestingBotTunnelStep.java`:
- Around line 105-118: Require Item.CONFIGURE instead of Item.EXTENDED_READ in
the credential dropdown permission checks for doFillCredentialsIdItems in
src/main/java/testingbot/pipeline/TestingBotTunnelStep.java lines 105-118 and
the corresponding endpoint in
src/main/java/testingbot/pipeline/TestingBotStep.java lines 121-131; preserve
the existing administrator check and credential lookup behavior.
- Around line 137-142: Update the tunnel startup flow in call so exceptions
after App creation or TunnelManager.start are handled before rethrowing: stop
the App when startup fails, and remove and stop the registered tunnel when the
post-start body invocation fails. Ensure both failure paths clean up the tunnel
before propagating the original exception, while preserving normal registration
and return behavior.
In `@src/main/java/testingbot/TestingBotBuildSummary.java`:
- Around line 63-68: In the completion handling around AbstractTestResultAction
and TestResult, validate that getResult() returns a TestResult before casting.
If the result is null or another type, return without processing; only cast and
continue when the value is a TestResult.
In `@src/main/java/testingbot/TestingBotBuildWrapper.java`:
- Around line 77-83: Update the tunnel startup flow in the build-environment
creation method so a failure from TunnelManager.start is not suppressed: clean
up the partially initialized App, then propagate the startup exception instead
of returning TestingBotBuildEnvironment or exporting tunnel state. Preserve the
successful path that returns the environment only after the tunnel starts
successfully.
In `@src/main/java/testingbot/TestingBotCredentials.java`:
- Around line 278-279: Add an upfront permission check in doCheckId before
iterating or probing credential stores, requiring the relevant item/credential
VIEW permission for the supplied ModelObject context. Return the validator’s
existing unauthorized response immediately when permission is absent, preserving
duplicate-ID and store-name warnings for authorized callers.
In `@src/main/java/testingbot/TestingBotReportFactory.java`:
- Around line 27-45: Update SESSION_PATTERN used by collectSessionIDs to require
a valid terminator immediately after the alphanumeric session ID, preventing
extraction of partial matches from values such as “abc-123”. Add or update tests
covering malformed suffixes and confirm only complete, valid session IDs are
collected by findSessionIDs.
In `@src/main/java/testingbot/TestingBotSecretConsoleLogFilter.java`:
- Around line 22-34: Update TestingBotStep and TestingBotTunnelStep to attach a
body-scoped TaskListenerDecorator using the existing secret-masking filter when
invoking their Pipeline bodies. Ensure TESTINGBOT_SECRET and TB_SECRET are
masked in Pipeline log output while preserving the current body environment
setup.
In `@src/main/java/testingbot/TunnelManager.java`:
- Around line 51-89: The tunnelOptions() allow-list includes options such as
se-port, pac, logfile, and noproxy that are not applied by the tunnel startup
flow. Either wire every declared option through to its corresponding tunnel
configuration, or remove/reject unsupported options explicitly so parsing cannot
silently ignore them; apply the same correction to the additional options
referenced by the comment.
- Around line 169-181: Update the readiness loop in TunnelManager around
api.pollTunnel and the ready flag to enforce a finite deadline using the
existing polling interval, and throw an appropriate failure when the deadline
expires. Replace the current exception logging-and-break behavior with
propagation of the polling failure so tunnel startup cannot report success after
an error, while preserving successful completion when the state becomes READY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d39d2241-0327-4d59-abf3-e378c69ca6b6
📒 Files selected for processing (27)
.github/dependabot.yml.github/workflows/cd.yaml.github/workflows/ci.yamlCHANGELOGJenkinsfileREADME.mdpom.xmlsrc/main/java/testingbot/TestReporter.javasrc/main/java/testingbot/TestingBotBuildAction.javasrc/main/java/testingbot/TestingBotBuildSummary.javasrc/main/java/testingbot/TestingBotBuildWrapper.javasrc/main/java/testingbot/TestingBotBuilder.javasrc/main/java/testingbot/TestingBotCredentials.javasrc/main/java/testingbot/TestingBotReportFactory.javasrc/main/java/testingbot/TestingBotSecretConsoleLogFilter.javasrc/main/java/testingbot/TunnelManager.javasrc/main/java/testingbot/pipeline/ExpanderImpl.javasrc/main/java/testingbot/pipeline/TestingBotStep.javasrc/main/java/testingbot/pipeline/TestingBotTestPublisher.javasrc/main/java/testingbot/pipeline/TestingBotTunnelStep.javasrc/main/java/testingbot/tunnel/TunnelConfig.javasrc/main/resources/index.jellysrc/main/resources/testingbot/TestingBotBuilder/config.jellysrc/main/resources/testingbot/TestingBotBuilder/global.jellysrc/main/resources/testingbot/TestingBotBuilder/help-apiKey.htmlsrc/main/resources/testingbot/TestingBotBuilder/help-apiSecret.htmlsrc/test/java/testingbot/TunnelParsingTest.java
💤 Files with no reviewable changes (6)
- src/main/resources/testingbot/TestingBotBuilder/global.jelly
- src/main/java/testingbot/tunnel/TunnelConfig.java
- src/main/resources/testingbot/TestingBotBuilder/config.jelly
- src/main/resources/testingbot/TestingBotBuilder/help-apiKey.html
- src/main/resources/testingbot/TestingBotBuilder/help-apiSecret.html
- src/main/java/testingbot/TestingBotBuilder.java
|
|
||
| jobs: | ||
| maven-cd: | ||
| uses: jenkins-infra/github-reusable-workflows/.github/workflows/maven-cd.yml@v1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== .github/workflows/cd.yaml ==\n'
cat -n .github/workflows/cd.yaml
printf '\n== .github/workflows/ci.yaml ==\n'
cat -n .github/workflows/ci.yamlRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 1339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list around workflows ==\n'
git ls-files .github/workflows
printf '\n== Grep for uses: lines in workflow files ==\n'
rg -n 'uses:' .github/workflows/*.yamlRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('.github/workflows/cd.yaml'), Path('.github/workflows/ci.yaml')]:
print(f"\n== {path} ==")
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if 'uses:' in line or 'checkout@' in line or 'setup-java@' in line:
print(f"{i:4d}: {line}")
PYRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 412
Pin these workflow references to commit SHAs.
.github/workflows/cd.yaml#L14 and .github/workflows/ci.yaml#L18-L20 still use mutable tags (@v1, @v4); switch the reusable workflow, actions/checkout, and actions/setup-java to full commit SHAs.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/cd.yaml#L14-L14(this comment).github/workflows/ci.yaml#L18-L20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cd.yaml at line 14, The workflow references in
.github/workflows/cd.yaml lines 14-14 and .github/workflows/ci.yaml lines 18-20
use mutable tags; replace the reusable Maven workflow, actions/checkout, and
actions/setup-java references with their corresponding full commit SHAs,
preserving the existing workflow configuration.
Source: Linters/SAST tools
| @POST | ||
| public ListBoxModel doFillCredentialsIdItems(final @AncestorInPath Item context) { | ||
| if (context == null ? !Jenkins.get().hasPermission(Jenkins.ADMINISTER) | ||
| : !context.hasPermission(Item.EXTENDED_READ)) { | ||
| return new StandardListBoxModel(); | ||
| } | ||
| return new StandardListBoxModel() | ||
| .withAll(CredentialsProvider.lookupCredentials( | ||
| .withAll(CredentialsProvider.lookupCredentialsInItem( | ||
| TestingBotCredentials.class, | ||
| context, | ||
| ACL.SYSTEM, | ||
| ACL.SYSTEM2, | ||
| new ArrayList<DomainRequirement>() | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require job configuration permission for credential dropdowns.
Both endpoints permit credential metadata enumeration with only Item.EXTENDED_READ; use Item.CONFIGURE, consistent with the build-wrapper endpoint.
src/main/java/testingbot/pipeline/TestingBotTunnelStep.java#L105-L118: replace the item permission check withItem.CONFIGURE.src/main/java/testingbot/pipeline/TestingBotStep.java#L121-L131: replace the item permission check withItem.CONFIGURE.
📍 Affects 2 files
src/main/java/testingbot/pipeline/TestingBotTunnelStep.java#L105-L118(this comment)src/main/java/testingbot/pipeline/TestingBotStep.java#L121-L131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/pipeline/TestingBotTunnelStep.java` around lines 105
- 118, Require Item.CONFIGURE instead of Item.EXTENDED_READ in the credential
dropdown permission checks for doFillCredentialsIdItems in
src/main/java/testingbot/pipeline/TestingBotTunnelStep.java lines 105-118 and
the corresponding endpoint in
src/main/java/testingbot/pipeline/TestingBotStep.java lines 121-131; preserve
the existing administrator check and credential lookup behavior.
| @POST | ||
| public final FormValidation doCheckId(@QueryParameter String value, @AncestorInPath ModelObject context) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant section
ast-grep outline src/main/java/testingbot/TestingBotCredentials.java --view expanded || true
sed -n '240,330p' src/main/java/testingbot/TestingBotCredentials.java
# Find related validation methods and permission checks in this file
rg -n "doCheckId|checkPermission|hasPermission|Permission|FormValidation|credentials" src/main/java/testingbot/TestingBotCredentials.java
# Find usages of the validation endpoint and any nearby access-control patterns
rg -n "doCheckId\(|`@AncestorInPath`|ModelObject context|Jenkins.get|ACL|Item.READ|Credential" src/main/javaRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 25463
🌐 Web query:
Jenkins credentials form validation permission checks duplicate ID lookup CredentialsProvider.VIEW doCheckId security guidance
💡 Result:
In Jenkins, implementing secure form validation for credentials—including ID lookups—requires careful attention to permission checks to prevent unauthorized information disclosure [1][2][3]. The following guidance outlines the security best practices: Permission Checks in Validation Methods When writing a doCheckCredentialsId (or similar) method, you must verify the user's permissions before processing the request [2][3]. 1. Validation Logic: If the user lacks sufficient permission, the validation method should return FormValidation.ok() [2][3]. Returning a specific error message (e.g., "Access Denied" or "Cannot find ID") when a user lacks permission can inadvertently leak information about the existence of credentials [2][4]. 2. Appropriate Permissions: Use Item.EXTENDED_READ or CredentialsProvider.USE_ITEM to verify access to credentials within an item context [2]. If performing system-level validation, ensure the user has appropriate administrative rights (e.g., Jenkins.ADMINISTER), and avoid exposing metadata to unauthorized web requests [5]. 3. Recommended Pattern: Use CredentialsProvider.listCredentialsInItem or CredentialsProvider.lookupCredentialsInItem in conjunction with CredentialsMatchers.withId(value) to safely check for credentials [2]. Credential ID Duplicate Lookup and Security While duplicate credential IDs were historically a technical issue, Jenkins has moved toward enforcing stricter ID handling [6][7][8]. - ID Integrity: Jenkins has implemented server-side validation using Jenkins.checkGoodName to prevent the creation of credentials with illegal characters, improving security and consistency across creation endpoints [6][7]. - Duplicate IDs: Relying on duplicate IDs is discouraged as it leads to inconsistent UI behavior and lookup errors [8]. Recent improvements focus on preventing ID updates for IdCredentials to maintain store integrity [8][9]. Security Guidance - Secure Form Validation: Follow the official Jenkins guide for securing form validation, which emphasizes utilizing modern checkUrl and checkDependsOn attributes to avoid legacy, unsafe evaluation methods [1][3]. - Information Leakage: Never return descriptive errors that confirm the existence of a credential ID if the requester lacks permission to view it [2][4]. - Implementation Example: public FormValidation doCheckCredentialsId(@AncestorInPath Item item, @QueryParameter String value) { // Permission check if (item!= null &&!item.hasPermission(Item.EXTENDED_READ) &&!item.hasPermission(CredentialsProvider.USE_ITEM)) { return FormValidation.ok; // Silent failure to prevent info leakage } if (StringUtils.isBlank(value)) return FormValidation.ok; // Safe ID lookup if (CredentialsProvider.listCredentialsInItem(item, CredentialsMatchers.withId(value)).isEmpty) { return FormValidation.error("Cannot find credentials"); } return FormValidation.ok; } [2]
Citations:
- 1: https://www.jenkins.io/doc/developer/security/form-validation/
- 2: https://github.com/jenkinsci/credentials-plugin/blob/master/docs/consumer.adoc
- 3: https://www.jenkins.io/doc/developer/forms/form-validation/
- 4: https://nvd.nist.gov/vuln/detail/cve-2022-20616
- 5: https://javadoc.jenkins-ci.org/plugin/credentials/com/cloudbees/plugins/credentials/common/AbstractIdCredentialsListBoxModel.html
- 6: [JENKINS-75943] Credentials IDs can be created with illegal characters jenkinsci/credentials-plugin#977
- 7: Reject illegal characters in new credential IDs jenkinsci/credentials-plugin#1058
- 8: https://issues.jenkins.io/browse/JENKINS-72611
- 9: [JENKINS-72611] Forbid updating credentials ID for
IdCredentialsjenkinsci/credentials-plugin#506
Add an upfront permission gate to this validator. The per-store CredentialsProvider.VIEW check still lets this endpoint return duplicate-ID and store-name warnings to callers who can reach it; short-circuit before probing stores if that disclosure should stay limited to users with the relevant item/credential permission.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotCredentials.java` around lines 278 - 279,
Add an upfront permission check in doCheckId before iterating or probing
credential stores, requiring the relevant item/credential VIEW permission for
the supplied ModelObject context. Return the validator’s existing unauthorized
response immediately when permission is absent, preserving duplicate-ID and
store-name warnings for authorized callers.
| private static final Pattern SESSION_PATTERN = Pattern.compile("TestingBotSessionID=([A-Za-z0-9]+)"); | ||
|
|
||
| public static List<String> findSessionIDs(CaseResult testResult) { | ||
| List<String> sessions = new ArrayList<>(); | ||
| Pattern p = Pattern.compile("TestingBotSessionID=(.*)"); | ||
| if (testResult.getStdout() != null) { | ||
| Matcher matchOut = p.matcher(testResult.getStdout()); | ||
| while (matchOut.find()) { | ||
| String sessionId = matchOut.group(1); | ||
| sessions.add(sessionId); | ||
| } | ||
| } | ||
|
|
||
| if (testResult.getStderr() != null) { | ||
| Matcher matchError = p.matcher(testResult.getStderr()); | ||
| while (matchError.find()) { | ||
| String sessionId = matchError.group(1); | ||
| sessions.add(sessionId); | ||
| } | ||
| } | ||
|
|
||
| if (testResult.getClassName() != null) { | ||
| Matcher matchName = p.matcher(testResult.getClassName()); | ||
| while (matchName.find()) { | ||
| String sessionId = matchName.group(1); | ||
| sessions.add(sessionId); | ||
| } | ||
| collectSessionIDs(testResult.getStdout(), sessions); | ||
| collectSessionIDs(testResult.getStderr(), sessions); | ||
| collectSessionIDs(testResult.getClassName(), sessions); | ||
| collectSessionIDs(testResult.getFullName(), sessions); | ||
| return sessions; | ||
| } | ||
|
|
||
| static void collectSessionIDs(String text, List<String> sessions) { | ||
| if (text == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (testResult.getFullName() != null) { | ||
| Matcher matchFullName = p.matcher(testResult.getFullName()); | ||
| while (matchFullName.find()) { | ||
| String sessionId = matchFullName.group(1); | ||
| sessions.add(sessionId); | ||
| } | ||
| Matcher matcher = SESSION_PATTERN.matcher(text); | ||
| while (matcher.find()) { | ||
| sessions.add(matcher.group(1)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject partial session-ID matches.
The pattern extracts abc from TestingBotSessionID=abc-123, potentially updating the wrong session. Require a valid terminator and test malformed suffixes.
Proposed fix
-private static final Pattern SESSION_PATTERN = Pattern.compile("TestingBotSessionID=([A-Za-z0-9]+)");
+private static final Pattern SESSION_PATTERN =
+ Pattern.compile("TestingBotSessionID=([A-Za-z0-9]+)(?=\\s|$)");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final Pattern SESSION_PATTERN = Pattern.compile("TestingBotSessionID=([A-Za-z0-9]+)"); | |
| public static List<String> findSessionIDs(CaseResult testResult) { | |
| List<String> sessions = new ArrayList<>(); | |
| Pattern p = Pattern.compile("TestingBotSessionID=(.*)"); | |
| if (testResult.getStdout() != null) { | |
| Matcher matchOut = p.matcher(testResult.getStdout()); | |
| while (matchOut.find()) { | |
| String sessionId = matchOut.group(1); | |
| sessions.add(sessionId); | |
| } | |
| } | |
| if (testResult.getStderr() != null) { | |
| Matcher matchError = p.matcher(testResult.getStderr()); | |
| while (matchError.find()) { | |
| String sessionId = matchError.group(1); | |
| sessions.add(sessionId); | |
| } | |
| } | |
| if (testResult.getClassName() != null) { | |
| Matcher matchName = p.matcher(testResult.getClassName()); | |
| while (matchName.find()) { | |
| String sessionId = matchName.group(1); | |
| sessions.add(sessionId); | |
| } | |
| collectSessionIDs(testResult.getStdout(), sessions); | |
| collectSessionIDs(testResult.getStderr(), sessions); | |
| collectSessionIDs(testResult.getClassName(), sessions); | |
| collectSessionIDs(testResult.getFullName(), sessions); | |
| return sessions; | |
| } | |
| static void collectSessionIDs(String text, List<String> sessions) { | |
| if (text == null) { | |
| return; | |
| } | |
| if (testResult.getFullName() != null) { | |
| Matcher matchFullName = p.matcher(testResult.getFullName()); | |
| while (matchFullName.find()) { | |
| String sessionId = matchFullName.group(1); | |
| sessions.add(sessionId); | |
| } | |
| Matcher matcher = SESSION_PATTERN.matcher(text); | |
| while (matcher.find()) { | |
| sessions.add(matcher.group(1)); | |
| } | |
| private static final Pattern SESSION_PATTERN = | |
| Pattern.compile("TestingBotSessionID=([A-Za-z0-9]+)(?=\\s|$)"); | |
| public static List<String> findSessionIDs(CaseResult testResult) { | |
| List<String> sessions = new ArrayList<>(); | |
| collectSessionIDs(testResult.getStdout(), sessions); | |
| collectSessionIDs(testResult.getStderr(), sessions); | |
| collectSessionIDs(testResult.getClassName(), sessions); | |
| collectSessionIDs(testResult.getFullName(), sessions); | |
| return sessions; | |
| } | |
| static void collectSessionIDs(String text, List<String> sessions) { | |
| if (text == null) { | |
| return; | |
| } | |
| Matcher matcher = SESSION_PATTERN.matcher(text); | |
| while (matcher.find()) { | |
| sessions.add(matcher.group(1)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotReportFactory.java` around lines 27 - 45,
Update SESSION_PATTERN used by collectSessionIDs to require a valid terminator
immediately after the alphanumeric session ID, preventing extraction of partial
matches from values such as “abc-123”. Add or update tests covering malformed
suffixes and confirm only complete, valid session IDs are collected by
findSessionIDs.
…docs - Pin GitHub Actions and the reusable CD workflow to commit SHAs (mutable-tag hardening) - Set persist-credentials: false on actions/checkout in CI - Require Item.CONFIGURE (not EXTENDED_READ) for the pipeline credential dropdowns, matching the freestyle wrapper - Stop the tunnel App if startup fails, and stop/remove the registered tunnel if body invocation fails, to avoid leaks - Clarify README: SELENIUM_HOST/PORT and TESTINGBOT_TUNNEL_IDENTIFIER are exposed only inside testingbotTunnel blocks
…tion, pipeline masking - TestingBotBuildSummary: guard getResult() with instanceof before casting to TestResult - TestingBotBuildWrapper.setUp: stop the App and fail the build if the tunnel cannot start (no silent broken tunnel) - TunnelManager: enforce a finite readiness deadline and propagate poll failures (never report success after an error) - TunnelManager: trim the option allow-list to only options the embedded tunnel can apply; others are rejected+logged, not silently ignored - Mask TESTINGBOT_SECRET/TB_SECRET in Pipeline step output via a body-scoped ConsoleLogFilter in both testingbot and testingbotTunnel Skipped: upfront permission check in TestingBotCredentials.doCheckId - checkForDuplicates already gates every store access with CredentialsProvider.VIEW and the endpoint is @post, so no unauthorized probing occurs.
The TestingBotTunnel uber-jar bundles an older commons-cli that shadows the plugin's at runtime, so DefaultParser/Option.builder threw NoSuchFieldError when actually invoked (compile passed; hpi:run never exercised it). Parse the six supported options by hand and remove the commons-cli dependency entirely - also removes the deprecated-API concern. Add pure unit tests (no Jenkins harness, so unaffected by the fat-jar Jetty clash): - TunnelManagerOptionsTest: each supported option is applied to App; unsupported options are reported - SecretMaskingTest: body-scoped secret masking logic JenkinsRule/JCasC coverage remains blocked by the fat-jar Jetty conflict until TestingBotTunnel is published as a thin jar.
…ckson API Now that 4.8 is published to Maven Central as a thin jar with a complete POM, consume the self-contained 'shaded' classifier and exclude all transitives. The tunnel embeds its own server stack (Jetty 11, Jackson, Apache HTTP, commons-cli) that conflicts with Jenkins-managed versions (Jetty 12, Jackson 2.22); bundling the shaded jar keeps those internal to the plugin classloader and avoids leaking an unreconcilable transitive tree (enforcer RequireUpperBoundDeps) into the build. Tunnel 4.8 changed Api.pollTunnel() to return com.fasterxml.jackson.databind.JsonNode instead of net.sf.json.JSONObject; update TunnelManager.start() accordingly.
|
@coderabbitai please re-check |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
132-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the release-trigger documentation with the workflow.
The README says merging to the default branch directly triggers the
cdworkflow, but.github/workflows/cd.yamldeclares onlyworkflow_dispatchandcheck_run: completed. Document the check-run trigger explicitly, or add a default-branchpushtrigger if direct merge triggering is intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 132, Update the release documentation to match the triggers declared in the cd workflow: explicitly describe the check_run completed trigger and workflow_dispatch, or add a default-branch push trigger if merging is intended to launch releases directly. Ensure the README no longer claims an unsupported direct merge trigger.src/main/java/testingbot/TestingBotBuildWrapper.java (1)
117-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize the null-context credential lookup.
When
contextis null, this endpoint performslookupCredentialsInItem(..., ACL.SYSTEM2, ...)without checking a caller permission, exposing globally scoped credential metadata. MatchTestingBotStep.DescriptorImpl: requireJenkins.ADMINISTERfor a null context andItem.CONFIGUREotherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/testingbot/TestingBotBuildWrapper.java` around lines 117 - 126, Update doFillCredentialsIdItems so null contexts require Jenkins.ADMINISTER before performing the credential lookup, while non-null contexts continue to require Item.CONFIGURE. Preserve the existing empty StandardListBoxModel response for unauthorized callers and apply the same authorization behavior as TestingBotStep.DescriptorImpl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/testingbot/TunnelManager.java`:
- Around line 84-117: The option parsing cases in TunnelManager must report
malformed input when required values are absent or the next token is another
flag. Update the handling for --proxy, --tunnel-identifier, --proxy-userpwd,
--metrics-port, and --auth to reject or record each missing-value case instead
of silently continuing, while preserving valid-value parsing; add coverage for
every option’s missing-value scenario.
---
Outside diff comments:
In `@README.md`:
- Line 132: Update the release documentation to match the triggers declared in
the cd workflow: explicitly describe the check_run completed trigger and
workflow_dispatch, or add a default-branch push trigger if merging is intended
to launch releases directly. Ensure the README no longer claims an unsupported
direct merge trigger.
In `@src/main/java/testingbot/TestingBotBuildWrapper.java`:
- Around line 117-126: Update doFillCredentialsIdItems so null contexts require
Jenkins.ADMINISTER before performing the credential lookup, while non-null
contexts continue to require Item.CONFIGURE. Preserve the existing empty
StandardListBoxModel response for unauthorized callers and apply the same
authorization behavior as TestingBotStep.DescriptorImpl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d3924a3-4aa7-4413-ab5f-9061902c3102
📒 Files selected for processing (12)
.github/workflows/cd.yaml.github/workflows/ci.yamlREADME.mdpom.xmlsrc/main/java/testingbot/TestingBotBuildSummary.javasrc/main/java/testingbot/TestingBotBuildWrapper.javasrc/main/java/testingbot/TunnelManager.javasrc/main/java/testingbot/pipeline/SecretMaskingConsoleLogFilter.javasrc/main/java/testingbot/pipeline/TestingBotStep.javasrc/main/java/testingbot/pipeline/TestingBotTunnelStep.javasrc/test/java/testingbot/TunnelManagerOptionsTest.javasrc/test/java/testingbot/pipeline/SecretMaskingTest.java
| case "-i": | ||
| case "--tunnel-identifier": | ||
| if (hasValue(tokens, i)) { | ||
| app.setTunnelIdentifier(tokens[++i]); | ||
| } | ||
| break; | ||
| case "-Y": | ||
| case "--proxy": | ||
| if (hasValue(tokens, i)) { | ||
| app.setProxy(tokens[++i]); | ||
| } | ||
| break; | ||
| case "-z": | ||
| case "--proxy-userpwd": | ||
| if (hasValue(tokens, i)) { | ||
| app.setProxyAuth(tokens[++i]); | ||
| } | ||
| break; | ||
| case "--metrics-port": | ||
| if (hasValue(tokens, i)) { | ||
| String value = tokens[++i]; | ||
| try { | ||
| app.setMetricsPort(Integer.parseInt(value)); | ||
| } catch (NumberFormatException nfe) { | ||
| unsupported.add(token + " " + value); | ||
| } | ||
| } | ||
| break; | ||
| case "-a": | ||
| case "--auth": | ||
| if (hasValue(tokens, i)) { | ||
| app.setBasicAuth(new String[]{tokens[++i]}); | ||
| } | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject options that are missing required values.
--proxy, --tunnel-identifier, --proxy-userpwd, --metrics-port, and --auth become silent no-ops when their next token is absent or another flag. Fail parsing (or at least report them as malformed) so a misconfigured tunnel cannot start with unintended defaults; add coverage for each missing-value case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TunnelManager.java` around lines 84 - 117, The
option parsing cases in TunnelManager must report malformed input when required
values are absent or the next token is another flag. Update the handling for
--proxy, --tunnel-identifier, --proxy-userpwd, --metrics-port, and --auth to
reject or record each missing-value case instead of silently continuing, while
preserving valid-value parsing; add coverage for every option’s missing-value
scenario.
… skipping TunnelManager.applyOptions silently ignored a value-taking option (--tunnel-identifier, --proxy, --proxy-userpwd, --metrics-port, --auth) when its value was absent or the next token was another flag. Report each such case in the build log (alongside unknown options and invalid --metrics-port values) so misconfigured tunnel options are visible rather than dropped. Adds unit coverage for every option's missing-value scenario.
1 participant
Modernization of the Jenkins TestingBot plugin. Raises the build baseline and fixes several correctness/security issues. Scope: Quick Wins + Medium + the critical tunnel-isolation fix; Step-API/Jakarta rewrites and a full harness test suite are intentionally deferred.
Highlights
jenkins.version2.541.3,bom-2.541.x, Java 17; removedjava.levelandfindbugs.skip.static Apptunnel with a per-executionAppkeyed by tunnel identifier, so parallel pipeline tunnels no longer corrupt or stop each other.ConsoleLogFilter);@POST+ permission checks on all credential form endpoints; dropped@ExportedfromgetSecret().RunListener(was NPE/IOOBE on non-TestingBot / mixed suites); null-checked the publisherData; fixed credential-migration bugs; anchored the session-id regex.ACL.SYSTEM2,lookupCredentialsInItem, commons-cliDefaultParser/Option.builder, Guava removed.TunnelManager; deleted the vestigialTestingBotBuilderandTunnelConfig; added@Symbol.mvn verifyon JDK 17 + 21) and CD (JEP-229), Dependabot, unit tests.Verification
mvn verifypasses on JDK 17 with enforcer + SpotBugs (0 bugs); 7 unit tests green;mvn hpi:runloads the plugin in Jenkins 2.541.3 with 0 SEVERE errors.Known limitation / follow-up
The
com.testingbot:TestingBotTunnelartifact is an uber-jar bundling an old Jetty, which clashes with the test harness' Jetty 12 and breaksJenkinsRule-based tests (the parent-injected test is excluded for this reason). Republishing that artifact as a thin jar would unblock full harness/JCasC testing.Summary by CodeRabbit