[JENKINS-60738] Fix global configuration submission from UI by Dohbedoh · Pull Request #347 · jenkinsci/github-plugin

Dohbedoh · 2023-05-02T11:29:45Z

Fixes JENKINS-60738 caused by changes introduced in #221.

Databinding of hookUrl is not working properly, per my understanding because the DataboundSetter setHookUrl(String hookUrl) accepts a String but the field is a URL. While adding a public void setHookUrl(URL hookUrl) might solve some of the problem, then there is yet another issue that disabling the Specify another hook URL for GitHub configuration checkbox does not remove the hook URL at all... that seems wrong.

So the propose fix:

handle the binding and the optional property state in the configure(StaplerRequest req, JSONObject json)
reactivate and adjusted the global configuration UI tests

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

julieheard

LGTM, manually tested and is working 🙂

jglick · 2023-05-03T18:31:43Z

While this sounds fine as a hotfix, the proper fix would be to delete all the overrides and special cases handling and fix the form to use normal databinding by convention.

jglick · 2023-05-03T18:33:29Z

src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy

            f.entry(title: _("Override Hook URL")) {
                g.blockWrapper {
                    f.optionalBlock(title: _("Specify another hook URL for GitHub configuration"),
+                            name: "isOverrideHookUrl",
                            inline: true,
                            checked: instance.isOverrideHookUrl()) {
                        f.entry(field: "hookUrl") {
-                            f.textbox(checkMethod: "post")
+                            f.textbox(checkMethod: "post", name: "hookUrl")
                        }
                    }
                }


For example all of this should just be like

<f:entry field="hookUrl"> <f:textbox label="Alternate hook URL for GitHub configuration"/> </f:entry>

which may be blank.

Oh I see. Remove all the optional block that is not necessary.. Yeah sounds good to me I can test this.

Would be more work for sure. The usual problem with this sort of fix is retaining compatibility for both XML settings and JCasC. Sometimes hacking up the JSON processing in the GUI logic is the only option since refactoring to a simpler structure would break something.

the thing that bother me is the type URL of the hookUrl... Because of that type, the value cannot actually be blank :( :

java.net.MalformedURLException: no protocol: at java.base/java.net.URL.<init>(URL.java:645) at java.base/java.net.URL.<init>(URL.java:541) at java.base/java.net.URL.<init>(URL.java:488) at org.kohsuke.stapler.Stapler$3.convert(Stapler.java:1141) Caused: org.apache.commons.beanutils.ConversionException: no protocol:

So it is not that simple to "reset" the hook URL because of the binding.. I thought about changing the type from URL to String but the existing public getter might be a problem: https://github.com/jenkinsci/github-plugin/blob/master/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java#L131-L141

Changing its signature could be a breaking change. Though it does not seem to be used outside the scope of that plugin repo: https://github.com/search?ref=simplesearch&type=code&q=user%3Ajenkinsci++getHookUrl.

BTW use permalinks for better display:

github-plugin/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java

Lines 131 to 141 in 47b623a

/**

* @return hook url used as endpoint to search and write auto-managed hooks in GH

* @throws GHPluginConfigException if default jenkins url is malformed

*/

public URL getHookUrl() throws GHPluginConfigException {

if (hookUrl != null) {

return hookUrl;

} else {

return constructDefaultUrl();

}

}

src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java

jtnord · 2023-06-13T12:12:34Z

@KostyaSha are you ok with this as the active maintainer of this plugin?

jrtc27 · 2024-03-05T00:23:56Z

What's the status of this? I've just run into this with our own instance.

yahooguntu · 2024-03-06T23:09:04Z

I have the same issue; as a workaround I've been adding <hookUrl> directly to github-plugin-configuration.xml, but unfortunately it's cleared out when I save/apply System settings in the web UI.

jrtc27 · 2024-03-06T23:21:26Z

I have the same issue; as a workaround I've been adding <hookUrl> directly to github-plugin-configuration.xml, but unfortunately it's cleared out when I save/apply System settings in the web UI.

If you edit the config file and restart so it has the updated value in memory then the UI should show the correct setting and not clobber it on save.

franknarf8 · 2024-04-08T13:00:22Z

Can this be merged soon? I've just noticed this open PR after implementing the same fix and opening another PR : #375

Feel free to close the other PR

@KostyaSha @jglick

jglick · 2024-04-30T11:46:50Z

(I am not a maintainer)

Dohbedoh · 2024-10-30T07:22:05Z

@jenkinsci/github-plugin-developers anybody able to merge this ?

jglick · 2024-11-01T15:45:40Z

@KostyaSha perhaps

Dohbedoh · 2025-01-08T07:38:04Z

@KostyaSha @lanwen @oleg-nenashev anybody able to help merge this ?

sebastianreloaded · 2025-06-19T17:10:49Z

I have the same issue; as a workaround I've been adding <hookUrl> directly to github-plugin-configuration.xml, but unfortunately it's cleared out when I save/apply System settings in the web UI.

If you edit the config file and restart so it has the updated value in memory then the UI should show the correct setting and not clobber it on save.

But even when adding the hookUrl to the xml and restarting, the endpoint still only works for "/github-webhook/". everything else gives a "HTTP ERROR 403 No valid crumb was included in the request"

jgarciacloudbees · 2025-11-03T11:02:52Z

Hi @jenkinsci/github-plugin-developers, is there any chance to merge this PR?

jtnord · 2025-11-03T11:25:51Z

Hi @jenkinsci/github-plugin-developers, is there any chance to merge this PR?

the group contains 4 3 users (and a bot) who are not so active, I would suggest you start the process to adopt this plugin.

KostyaSha · 2025-11-03T22:27:00Z

the group contains 4 3 users (and a bot) who are not so active, I would suggest you start the process to adopt this plugin.

thanks to dependabot who spamed from all jenkins repos

olamy · 2026-02-11T04:34:15Z

@KostyaSha thanks for merging. Could you please make a release now? Thanks

olamy · 2026-02-11T10:33:20Z

Hi @jenkinsci/github-plugin-developers, is there any chance to merge this PR?

the group contains 4 3 users (and a bot) who are not so active, I would suggest you start the process to adopt this plugin.

looks like a good idea.

@KostyaSha let us know if you are too busy to maintain the plugin actively, I'm pretty sure some people will be happy to help to maintain such critical plugin.

[JENKINS-60738] Fix global configuration submission from UI

cfaa0c4

Dohbedoh force-pushed the JENKINS-60738 branch from 56e2723 to cfaa0c4 Compare May 2, 2023 11:31

[JENKINS-60738] Fix json binding for arrays

3a5d46f

Dohbedoh mentioned this pull request May 3, 2023

[JENKINS-71183] Fix Build actions anonymization enable UI check jenkinsci/support-core-plugin#462

Merged

6 tasks

julieheard approved these changes May 3, 2023

View reviewed changes

jglick reviewed May 3, 2023

View reviewed changes

Dohbedoh added 3 commits June 20, 2023 16:19

Merge branch 'master' into JENKINS-60738

a6ec398

Merge branch 'master' into JENKINS-60738

b14789f

Merge branch 'master' into JENKINS-60738

929dbe3

Merge branch 'master' into JENKINS-60738

e8952dd

Merge branch 'master' into JENKINS-60738

37f455b

Merge branch 'master' into JENKINS-60738

029dc4a

Dohbedoh requested a review from a team as a code owner November 1, 2024 01:54

Dohbedoh requested a review from KostyaSha November 15, 2024 00:23

KostyaSha merged commit daa1fa7 into jenkinsci:master Nov 3, 2025

	/**
	* @return hook url used as endpoint to search and write auto-managed hooks in GH
	* @throws GHPluginConfigException if default jenkins url is malformed
	*/
	public URL getHookUrl() throws GHPluginConfigException {
	if (hookUrl != null) {
	return hookUrl;
	} else {
	return constructDefaultUrl();
	}
	}

Conversation

Dohbedoh commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julieheard left a comment

Choose a reason for hiding this comment

Uh oh!

jglick commented May 3, 2023

Uh oh!

jglick May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Dohbedoh May 4, 2023

Choose a reason for hiding this comment

Uh oh!

jglick May 4, 2023

Choose a reason for hiding this comment

Uh oh!

Dohbedoh May 5, 2023

Choose a reason for hiding this comment

Uh oh!

jglick May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtnord commented Jun 13, 2023

Uh oh!

jrtc27 commented Mar 5, 2024

Uh oh!

yahooguntu commented Mar 6, 2024

Uh oh!

jrtc27 commented Mar 6, 2024

Uh oh!

franknarf8 commented Apr 8, 2024

Uh oh!

jglick commented Apr 30, 2024

Uh oh!

Dohbedoh commented Oct 30, 2024

Uh oh!

jglick commented Nov 1, 2024

Uh oh!

Dohbedoh commented Jan 8, 2025

Uh oh!

sebastianreloaded commented Jun 19, 2025

Uh oh!

jgarciacloudbees commented Nov 3, 2025

Uh oh!

jtnord commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Nov 3, 2025

Uh oh!

olamy commented Feb 11, 2026

Uh oh!

olamy commented Feb 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Dohbedoh commented May 2, 2023 •

edited

Loading

jtnord commented Nov 3, 2025 •

edited

Loading