-
Notifications
You must be signed in to change notification settings - Fork 1.1k
JENKINS-75316: Make Git build summary clickable links #3874 #3924
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,7 +45,7 @@ | |||||
| return url; | ||||||
| } | ||||||
|
|
||||||
| public final URL getUrl() throws IOException { | ||||||
| public URL getUrl() throws IOException { | ||||||
|
||||||
| public URL getUrl() throws IOException { | |
| public final URL getUrl() throws IOException { |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,9 +9,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| import hudson.plugins.git.Branch; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import hudson.plugins.git.Revision; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import hudson.plugins.git.UserRemoteConfig; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import hudson.plugins.git.browser.GitRepositoryBrowser; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.Serial; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.Serializable; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.URL; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Collection; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.HashSet; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -78,7 +81,14 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| public Set<String> remoteUrls = new HashSet<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Allow disambiguation of the action url when multiple {@link BuildData} actions present. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * The Git console browser (if any) used to link to the repository. | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| @CheckForNull | ||||||||||||||||||||||||||||||||||||||||||||||||
| public GitRepositoryBrowser browser; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
| public GitRepositoryBrowser browser; | |
| private GitRepositoryBrowser browser; |
Check warning on line 302 in src/main/java/hudson/plugins/git/util/BuildData.java
ci.jenkins.io / SpotBugs
PA_PUBLIC_PRIMITIVE_ATTRIBUTE
NORMAL:
Primitive field hudson.plugins.git.util.BuildData.browser is public and set from inside the class, which makes it too exposed. Consider making it private to limit external accessibility.
Raw output
no message found
Copilot
AI
Jan 31, 2026
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.
The ref normalization logic assumes that refs starting with "refs/remotes/" will always have exactly 4 parts when split by "/" (line 330-332). However, if a ref like "refs/remotes/origin" (only 3 parts) is passed, the normalization will be silently skipped and the full ref will be passed to the browser.
Consider handling this edge case more explicitly, either by:
- Logging a warning when the split doesn't produce the expected number of parts
- Falling back to a safe default normalization
- Documenting the expected ref format in a comment
Similarly, what happens if someone has a branch name containing slashes, like "refs/remotes/origin/feature/sub-feature"? The current logic would only strip "refs/remotes/origin/" leaving "feature/sub-feature", which might be correct, but should be documented.
| if (ref.startsWith("refs/remotes/")) { | |
| String[] parts = ref.split("/", 4); | |
| if (parts.length == 4) { | |
| ref = parts[3]; | |
| } | |
| } else if (ref.startsWith("refs/heads/")) { | |
| if (ref.startsWith("refs/remotes/")) { | |
| // Expected format: refs/remotes/<remote>/<branch-path> | |
| // where <branch-path> may itself contain '/' (e.g. feature/sub-feature). | |
| // We strip the "refs/remotes/<remote>/" prefix and keep the branch path. | |
| String[] parts = ref.split("/", 4); | |
| if (parts.length == 4) { | |
| ref = parts[3]; | |
| } else { | |
| // Unexpected remote ref format; skip normalization but log for diagnostics. | |
| Logger.getLogger(BuildData.class.getName()).log( | |
| Level.FINE, | |
| "Unexpected remote ref format ''{0}''; skipping normalization", | |
| ref | |
| ); | |
| } | |
| } else if (ref.startsWith("refs/heads/")) { | |
| // Expected format: refs/heads/<branch-path>; strip the "refs/heads/" prefix. |
Copilot
AI
Jan 31, 2026
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.
The ref normalization logic only handles 'refs/remotes/' and 'refs/heads/' prefixes, but Git has other ref types like 'refs/tags/', 'refs/pull/', etc. If a tag or pull request ref is passed, it will be passed to the browser as-is with the full ref path.
Consider whether this is intentional behavior, or if other ref types should also be normalized. At minimum, add a comment documenting which ref types are normalized and why others are left as-is.
Copilot
AI
Jan 31, 2026
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.
The URL strings returned by getCommitUrl, getRefUrl, and getRepositoryUrl are used directly in href attributes in the Jelly template without any explicit XSS protection. While Jelly's escape-by-default='true' at line 1 of summary.jelly should provide protection, the URL generation methods perform string replacements (e.g., replace(".git/commit/", "/commit/")) on URL strings.
If a malicious repository browser implementation returns a URL containing special characters or JavaScript, the string replacement could potentially create an XSS vulnerability. Consider validating that the returned URLs have safe schemes (http/https) and don't contain JavaScript or other dangerous protocols.
Copilot
AI
Jan 31, 2026
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.
Missing Javadoc: The public methods getCommitUrl, getRefUrl, and getRepositoryUrl lack Javadoc comments. Since these are new public API methods that will be called from Jelly templates and potentially from other code, they should have proper documentation explaining:
- What they return
- When they return null
- What the parameters represent
- Whether the returned URLs are sanitized/normalized
For example, getCommitUrl should document that it returns null if browser is null, if commitId is null, if browser.getChangeSetLink throws IOException, or if the browser returns null.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,26 +4,48 @@ | |||||
| xmlns:f="/lib/form" xmlns:i="jelly:fmt"> | ||||||
| <t:summary icon="symbol-git-logo plugin-git"> | ||||||
|
|
||||||
| <strong>${%Revision}</strong>: ${it.lastBuiltRevision.sha1.name()} | ||||||
| <j:set var="browser" value="${it.browser}"/> | ||||||
|
||||||
| <j:set var="browser" value="${it.browser}"/> |
Copilot
AI
Jan 31, 2026
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.
Inconsistent indentation: This line uses 12 spaces for indentation while the surrounding choose/when/otherwise blocks use 16 spaces (lines 10-15, 23-31, 40-45). This should be indented with 16 spaces to match the other j:set statements at the same nesting level.
| <j:set var="branchLink" value="${it.getRefUrl(branch.name)}"/> | |
| <j:set var="branchLink" value="${it.getRefUrl(branch.name)}"/> |
Copilot
AI
Jan 31, 2026
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.
Inconsistent indentation: This line uses 20 spaces for indentation while it should use 16 spaces to match the indentation of the j:when and j:otherwise tags at lines 40 and 43.
| <j:choose> | |
| <j:choose> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package hudson.plugins.git.util; | ||
|
|
||
| import hudson.plugins.git.browser.GithubWeb; | ||
| import org.junit.jupiter.api.Test; | ||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| public class BuildDataBrowserTest { | ||
|
|
||
| @Test | ||
| public void testBrowserPersistence() { | ||
| BuildData data = new BuildData(); | ||
| assertNull(data.getBrowser()); | ||
|
|
||
| GithubWeb browser = new GithubWeb("https://github.com/jenkinsci/git-plugin"); | ||
| data.setBrowser(browser); | ||
| assertEquals(browser, data.getBrowser()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,11 @@ | |||||||||||||||||||||||||||||
| import hudson.plugins.git.Branch; | ||||||||||||||||||||||||||||||
| import hudson.plugins.git.Revision; | ||||||||||||||||||||||||||||||
| import hudson.plugins.git.UserRemoteConfig; | ||||||||||||||||||||||||||||||
| import hudson.plugins.git.GitChangeSet; | ||||||||||||||||||||||||||||||
| import hudson.plugins.git.browser.GitRepositoryBrowser; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||||
| import java.net.URL; | ||||||||||||||||||||||||||||||
| import java.util.ArrayList; | ||||||||||||||||||||||||||||||
| import java.util.Collection; | ||||||||||||||||||||||||||||||
| import java.util.Random; | ||||||||||||||||||||||||||||||
|
|
@@ -576,4 +580,91 @@ void testHashCodeEmptyData() { | |||||||||||||||||||||||||||||
| emptyData.remoteUrls = null; | ||||||||||||||||||||||||||||||
| assertEquals(emptyData.hashCode(), emptyData.hashCode()); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||
| void testSshRepositoryLink() throws IOException { | ||||||||||||||||||||||||||||||
| BuildData buildData = new BuildData(); | ||||||||||||||||||||||||||||||
| // Add an SSH remote URL to simulate JENKINS-75285 scenario | ||||||||||||||||||||||||||||||
| buildData.addRemoteUrl("git@github.com:user/repo.git"); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Configure a browser that returns a valid HTTP URL | ||||||||||||||||||||||||||||||
| buildData.setBrowser(new StubGitRepositoryBrowser("https://github.com/user/repo/")); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Verify getRepositoryUrl returns the HTTP URL despite the SSH remote | ||||||||||||||||||||||||||||||
| assertEquals("https://github.com/user/repo/", buildData.getRepositoryUrl()); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||
| void testCommitUrlSanitization() throws IOException { | ||||||||||||||||||||||||||||||
| BuildData buildData = new BuildData(); | ||||||||||||||||||||||||||||||
| buildData.setBrowser(new StubGitRepositoryBrowser("https://github.com/user/repo/")); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Stub returns URL with .git, verify getCommitUrl strips it | ||||||||||||||||||||||||||||||
| assertEquals("https://github.com/user/repo/commit/123456", buildData.getCommitUrl("123456")); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||
| void testRefUrlNormalization() throws IOException { | ||||||||||||||||||||||||||||||
| BuildData buildData = new BuildData(); | ||||||||||||||||||||||||||||||
| buildData.setBrowser(new StubGitRepositoryBrowser("https://github.com/user/repo/")); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Test refs/remotes/origin/main -> main | ||||||||||||||||||||||||||||||
| assertEquals("https://github.com/user/repo/tree/main", buildData.getRefUrl("refs/remotes/origin/main")); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Test refs/heads/feature -> feature | ||||||||||||||||||||||||||||||
| assertEquals("https://github.com/user/repo/tree/feature", buildData.getRefUrl("refs/heads/feature")); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+584
to
+616
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Stub implementation of GitRepositoryBrowser for testing purposes. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| private static class StubGitRepositoryBrowser extends GitRepositoryBrowser { | ||||||||||||||||||||||||||||||
| private static final long serialVersionUID = 1L; | ||||||||||||||||||||||||||||||
| private final String baseUrl; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| StubGitRepositoryBrowser(String baseUrl) { | ||||||||||||||||||||||||||||||
| this.baseUrl = baseUrl; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||
| public URL getUrl() throws IOException { | ||||||||||||||||||||||||||||||
| return new URL(baseUrl); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+626
to
+632
|
||||||||||||||||||||||||||||||
| this.baseUrl = baseUrl; | |
| } | |
| @Override | |
| public URL getUrl() throws IOException { | |
| return new URL(baseUrl); | |
| } | |
| super(baseUrl); | |
| this.baseUrl = baseUrl; | |
| } | |
| @Override | |
| public URL getUrl() throws IOException { | |
| return new URL(baseUrl); |
Copilot
AI
Jan 31, 2026
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.
Missing @OverRide annotation. The method getRefLink is overriding the base class method from GitRepositoryBrowser (added in this same PR at line 117). While the comment on line 652 explains why @OverRide was removed, now that the base class has this method, the @OverRide annotation should be added for consistency and to catch signature changes.
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) | |
| // Custom method for BuildData | |
| @Override |
Copilot
AI
Jan 31, 2026
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.
The getChangeSetLink(String) method at line 653 doesn't call the superclass method. The base class GitRepositoryBrowser.getChangeSetLink(String) at line 103-108 checks if the commitId is null or blank before delegating to getChangeSetLink(GitChangeSet). This stub bypasses that null/blank check and creates a different implementation pattern.
While this works for testing, it would be better to add @OverRide and ensure the stub properly extends the base class behavior, or remove this method entirely and rely on the base class implementation which will call getChangeSetLink(GitChangeSet) anyway.
| public URL getChangeSetLink(String changeSetId) throws IOException { | |
| return new URL(baseUrl.replaceAll("/$", "") + ".git/commit/" + changeSetId); | |
| } | |
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) |
Copilot
AI
Jan 31, 2026
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.
The test stub intentionally adds .git to URLs (lines 637, 654, 659) to test sanitization, but this creates misleading tests because the actual GitRepositoryBrowser implementations (like GithubWeb) don't add .git to their URLs. The sanitization logic in BuildData (lines 318, 342) is therefore removing something that real browsers never add in the first place.
This makes the tests not representative of actual browser behavior. Consider either:
- Verifying that real GitRepositoryBrowser implementations actually need this .git sanitization, or
- Removing the .git sanitization logic if it's not needed for real browsers
If the sanitization is genuinely needed for some browsers, the test should document which browsers add .git and why this is necessary.
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.
Critical bug: The browser is only set when base is null (line 2018), but when base is not null and gets cloned (lines 2014-2016), the method returns early without setting the browser. This means that for all builds after the first one, the browser will not be persisted in the BuildData, and the clickable links feature will not work.
The setBrowser call on line 2018 should be moved before the early return on line 2016, or the return statement should be removed and the setBrowser call should be placed after the if-else block to ensure the browser is always set on the BuildData before returning.