-
Notifications
You must be signed in to change notification settings - Fork 903
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
RSE-1052: fix IP disclosure when request doesnt have Host header #9110
Conversation
Testing the solution feasibility:
By passing the host header in:
|
rundeckapp/grails-app/init/rundeckapp/init/servlet/JettyServletContainerCustomizer.groovy
Outdated
Show resolved
Hide resolved
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.
Please, see my comments
rundeckapp/grails-app/init/rundeckapp/init/servlet/BanHttpMethodCustomizer.groovy
Outdated
Show resolved
Hide resolved
rundeckapp/grails-app/init/rundeckapp/init/servlet/BanHttpMethodCustomizer.groovy
Outdated
Show resolved
Hide resolved
@@ -50,6 +55,9 @@ class JettyServletContainerCustomizer implements WebServerFactoryCustomizer<Jett | |||
} | |||
}) | |||
factory.addServerCustomizers(new BanHttpMethodCustomizer()) | |||
if(featureService.featurePresent("setServerUrlOnNohostHeader", false)) { |
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.
Is it possible that the server.address
is not configured?
If so, I'm pretty sure the redirects (302s) will break.
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.
Looks much cleaner! Almost there!
Added a minor comment and a question.
@@ -3,6 +3,7 @@ package rundeckapp.init.servlet | |||
import org.eclipse.jetty.http.HttpMethod | |||
import org.eclipse.jetty.http.HttpStatus | |||
import org.eclipse.jetty.server.Connector | |||
import org.eclipse.jetty.server.HostHeaderCustomizer |
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.
Do you still need it?
@@ -50,6 +54,9 @@ class JettyServletContainerCustomizer implements WebServerFactoryCustomizer<Jett | |||
} | |||
}) | |||
factory.addServerCustomizers(new BanHttpMethodCustomizer()) | |||
if(featureService.featurePresent("setServerUrlOnNoHostHeader", false) && serverUrl) { |
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.
nit: for future considerations...
For minor perf improvements, it's recommended to order &&
checks from least expensive to most expensive.
Can you also please mark this PR as a "bugbash candidate". Thanks! |
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.
Looks great! 👍
Is this a bugfix, or an enhancement? Please describe.
When performing a request to Rundeck with no Host header, the internal IP is revealed on the response
Describe the solution you've implemented
This PR add a customizer that is enabled by the config
rundeck.feature.setServerUrlOnNoHostHeader.enabled
that will force the serverUrl and serverPort in the response if there is no Host header on requestDescribe alternatives you've considered
Additional context