[JENKINS-70630] - Fix HTTP2 missing header #7701

ca-monteiro · 2023-03-09T14:39:38Z

The 'Host' HTTP header is not included on HTTP2, compute the equivalent of the host header using the currentRequest.getServerName() and currentRequest.getServerPort() which is not dependent on the HTTP version used.

Testing done

The error is reproducible if you try to configure (or have previously configured) a Resource Root URL.

Built Jenkins from master, ran it and tried to configure a Resource Root URL - this reproduced the NPE. Applied the fix, rebuilt the WAR, ran it again and verified that the error is not present and that there is no NPE in System Log.

Proposed changelog entries

Fix null pointer exception on the "Manage Jenkins" page when HTTP/2 is enabled.

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
- Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

daniel-beck · 2023-03-09T14:47:12Z

core/src/main/java/jenkins/security/ResourceDomainConfiguration.java

@@ -134,7 +134,7 @@ private FormValidation checkUrl(String resourceRootUrlString, boolean allowOnlin

        StaplerRequest currentRequest = Stapler.getCurrentRequest();
        if (currentRequest != null) {
-            String currentRequestHost = currentRequest.getHeader("Host");
+            String currentRequestHost = String.format("%s:%s", currentRequest.getServerName(), currentRequest.getServerPort());


Did you test this? Seems like the next condition would rarely be true, given the lack of a port for the majority of host specifications.

I see, I was miss led by the original code, the getHeader("Host") could return host:port but the resourceRootUrlHost will always just be the host. I will made the change on the last commit

Yeah you also kinda discovered a bug -- resource root URL validation doesn't look like it deals well with nonstandard ports.

Are you up to attempting to address this while you're here? 😀

I.e., make this work for standard ports, nonstandard ports, and explicitly specified standard ports (even when mismatching).

(Haven't tried this recently, so I might be way off in guessing what the problems are.)

@daniel-beck would it be OK if this were limited to resolving the reported null pointer exception?

We've had two different bug reports of the issue and I'd rather see us bring that to a conclusion rather than broadening the pull request to cover more topics.

Given that the requirement is that the hostname of the resource root host is different from the jenkins host, does the port check really matter? This check is to prevent the user from locking himself out of Jenkins by using the same hostname, I don't think the port would matter in this case.
In my last commit, is now only checking that the current server hostname is different from what the user is trying to set as the resource root url.

@ca-monteiro Good point. So if this works it should be fine. (It even seems to fix a bug, since previously the port would be part of the Host header IIUC).

@MarkEWaite Hence me asking nicely and not marking the PR as "change requested" 😉

@daniel-beck, yes correct, the Host header sometimes could have the port appended to the string. The resourceRootUrlHost string would never have the port as it comes from the URL().getHost().

thomaslorentsen · 2023-03-13T01:27:51Z

core/src/main/java/jenkins/security/ResourceDomainConfiguration.java

@@ -134,7 +134,7 @@ private FormValidation checkUrl(String resourceRootUrlString, boolean allowOnlin

        StaplerRequest currentRequest = Stapler.getCurrentRequest();
        if (currentRequest != null) {
-            String currentRequestHost = currentRequest.getHeader("Host");
+            String currentRequestHost = String.format("%s", currentRequest.getServerName());


This could just be written as String currentRequestHost = currentRequest.getServerName(); to avoid the additional call to String.format. It will always return a String.

jenkinsci#7701 (comment)

MarkEWaite

I believe this is ready to merge once CI passes.

@daniel-beck are you OK with this as well?

basil

The Testing Done section is mandatory, not optional. Running java -jar jenkins.war --httpsKeyStore=/home/basil/src/jenkinsci/winstone/src/ssl/wildcard.jks --httpsKeyStorePassword=changeit --http2Port=443 and going to the Configure System page, I cannot reproduce the NullPointerException you describe. How do you know that this PR fixes the reported issue?

andreyakostovsap · 2023-04-05T23:23:45Z

Hi @basil, the error is reproducible if you try to configure (or have previously configured) a Resource Root URL.

I built Jenkins from master, ran it with your command and tried to configure a Resource Root URL - this reproduced the NPE. Then I applied @ca-monteiro's fix, rebuilt the WAR, ran it again and verified that the error is not present and that there is no NPE in System Log.

basil

Thanks! Given the successful testing, this looks good to me.

MarkEWaite · 2023-04-07T17:24:07Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

JENKINS-70630 - fix HTTP2 missing header

f177dd2

daniel-beck reviewed Mar 9, 2023

View reviewed changes

MarkEWaite self-assigned this Mar 9, 2023

carlos-mon-inm and others added 2 commits March 9, 2023 15:55

JENKINS-70630 - remove port

e9a541b

Merge branch 'master' into JENKINS-70630

71cc226

NotMyFault added the bug For changelog: Minor bug. Will be listed after features label Mar 12, 2023

NotMyFault changed the title ~~JENKINS-70630 - fix HTTP2 missing header~~ [JENKINS-70630] - Fix HTTP2 missing header Mar 12, 2023

NotMyFault requested review from daniel-beck and MarkEWaite March 12, 2023 09:47

thomaslorentsen reviewed Mar 13, 2023

View reviewed changes

MarkEWaite added 2 commits March 17, 2023 14:18

Merge branch 'master' into JENKINS-70630

5d552a3

Simplify assignment of currentRequestHost

bd09763

jenkinsci#7701 (comment)

MarkEWaite approved these changes Mar 17, 2023

View reviewed changes

Make it compile

f8819c4

basil reviewed Mar 22, 2023

View reviewed changes

basil approved these changes Apr 6, 2023

View reviewed changes

MarkEWaite approved these changes Apr 7, 2023

View reviewed changes

MarkEWaite merged commit 29e6edc into jenkinsci:master Apr 8, 2023
15 checks passed

NotMyFault added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-70630] - Fix HTTP2 missing header #7701

[JENKINS-70630] - Fix HTTP2 missing header #7701

ca-monteiro commented Mar 9, 2023 •

edited by MarkEWaite

daniel-beck Mar 9, 2023

ca-monteiro Mar 9, 2023

daniel-beck Mar 9, 2023 •

edited

MarkEWaite Mar 9, 2023

ca-monteiro Mar 10, 2023

daniel-beck Mar 10, 2023 •

edited

ca-monteiro Mar 10, 2023

thomaslorentsen Mar 13, 2023

MarkEWaite left a comment

basil left a comment

andreyakostovsap commented Apr 5, 2023 •

edited

basil left a comment

MarkEWaite commented Apr 7, 2023

[JENKINS-70630] - Fix HTTP2 missing header #7701

[JENKINS-70630] - Fix HTTP2 missing header #7701

Conversation

ca-monteiro commented Mar 9, 2023 • edited by MarkEWaite

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

daniel-beck Mar 9, 2023

Choose a reason for hiding this comment

ca-monteiro Mar 9, 2023

Choose a reason for hiding this comment

daniel-beck Mar 9, 2023 • edited

Choose a reason for hiding this comment

MarkEWaite Mar 9, 2023

Choose a reason for hiding this comment

ca-monteiro Mar 10, 2023

Choose a reason for hiding this comment

daniel-beck Mar 10, 2023 • edited

Choose a reason for hiding this comment

ca-monteiro Mar 10, 2023

Choose a reason for hiding this comment

thomaslorentsen Mar 13, 2023

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

andreyakostovsap commented Apr 5, 2023 • edited

basil left a comment

Choose a reason for hiding this comment

MarkEWaite commented Apr 7, 2023

ca-monteiro commented Mar 9, 2023 •

edited by MarkEWaite

daniel-beck Mar 9, 2023 •

edited

daniel-beck Mar 10, 2023 •

edited

andreyakostovsap commented Apr 5, 2023 •

edited