Skip to content
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

Fix Response#sendRedirect() if no request context exists. #479

Conversation

elkman
Copy link

@elkman elkman commented Feb 24, 2022

If no ROOT context is defined, the context may be null in special cases, e.g. RewriteValve may use Response#sendRedirect() without any application context associated.
In this case, the Tomcat behaviors for the context attributes useRelativeRedirects and sendRedirectBody are assumed, but without considering org.apache.catalina.STRICT_SERVLET_COMPLIANCE.

If no ROOT context is defined, the context may be null in special cases, e.g. RewriteValve may use Response#sendRedirect() without any application context associated.
In this case, the Tomcat behaviors for the context attributes useRelativeRedirects and sendRedirectBody are assumed, but without considering org.apache.catalina.STRICT_SERVLET_COMPLIANCE.
@elkman
Copy link
Author

elkman commented Feb 24, 2022

Hi,

I stumbled across this issue while writing a "RewriteValve" rule that redirects everything based on an HTTP header condition.

This is happening in a development environment where no ROOT context is configured and no reverse proxy ensures that only valid path prefixes are passed to the Tomcat instances. So it is not a real production world problem, but the NPE made me skeptical.

I don't know if there is an easy way to access the actual configured default values from the context.xml and taking into account the STRICT_SERVLET_COMPLIANCE setting. This would be the preferred solution, but the case is also relatively unlikely in production setups and my primary goal is to avoid the NPE.

Cheers,
@elkman

@markt-asf
Copy link
Contributor

This looks like invalid configuration at this point.

Generally, an NPE is preferable to a change that masks a configuration issue.

@elkman
Copy link
Author

elkman commented Feb 28, 2022

That is a good point. Do you think the misconfiguration is that a ROOT context should always be configured, or that rewrite rules should only be defined on existing contexts?

@markt-asf
Copy link
Contributor

I can see valid use cases that wouldn't meet at least one of those criteria so I don't think it is quite that simple. Having a concrete use case to discuss might make things a little clearer. Can you provide an example that triggered the NPE (request URI, rewrite rule and any other relevant info)?

@elkman
Copy link
Author

elkman commented Feb 28, 2022

A valid UseCase for me would be: Migration of an application to a new context

https://host/myOldApp -> https://host/myNewApp

In this case you would forward both URL prefixes from the load balancer/proxy to the Tomcat and define a rewrite rule that redirects all requests to the old context to the new one.

RewriteRule ^/myOldApp/(.*)$ /myNewApp/legacy/$1 [R=301,L]

But since there exists no context for myOldApp, this would result in an NPE.

(And yes, of course the Tomcat terminates TLS in this case and not the load balancer, otherwise you would do the redirect there of course.)

I would prefer to handle such misconfiguration by the RewriteValve in a understandable way without debugging into Tomcat codebase. But I don't see any practical way for reliable detection by the RewriteValve. Neither when loading the rule configuration, nor at runtime (except to catch the NPE).
Instead of using default values, a specific exception could be generated. Then the error handling could be realized in the calling component, but this would violate the API.

@markt-asf
Copy link
Contributor

Thanks for that example.
The right way to fix this will depend on the answer to the question "If a rewrite valve is configured at the Host level, is it a requirement that a context is present that matches the original request?" If yes, this is a documentation issue. If no, this is a Tomcat bug.
I'm currently leaning towards a "no" based on your use case. I can't really see a justification for requiring either a ROOT context or an myOldApp context.

@rmaucher
Copy link
Contributor

rmaucher commented Mar 1, 2022

It expands the capability of rewrite a bit, so "no" would be a good option.
Actually I was considering using rewrite for the context path optional feature (until you said this had to be done cleanly ;) ), and this is also the same use case.

@markt-asf
Copy link
Contributor

Based on this discussion, the original fix is correct. I'll merge this shortly.

@markt-asf markt-asf merged commit c3eb91e into apache:main Mar 1, 2022
markt-asf added a commit that referenced this pull request Mar 1, 2022
@elkman
Copy link
Author

elkman commented Mar 2, 2022

Thanks for the discussion, time and all your great work on the Tomcat project.

@elkman elkman deleted the feature/fix_rewriteValve_NPE_if_no_context_matches branch March 2, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants