Bug 57337 - Content negotiation fails with conditional Redirect
Summary: Content negotiation fails with conditional Redirect
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_negotiation (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2014-12-10 13:05 UTC by Kees Hoekzema
Modified: 2018-07-07 17:23 UTC (History)
1 user (show)



Attachments
Propsed patch (first attempt, against 2.4.33) (11.17 KB, patch)
2018-07-07 16:34 UTC, Alex Smith
Details | Diff
Suggested improvement for reporting of `redirect of negotiation variant' configuration errors (1.54 KB, patch)
2018-07-07 16:59 UTC, Alex Smith
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kees Hoekzema 2014-12-10 13:05:46 UTC
When there is a conditional redirect in a vhost, Multiviews/negotiation does not work anymore.

Steps to reproduce this bug; use this config:

-=-=-=-=-=-
DocumentRoot /var/www
<Directory /var/www/>
	Options MultiViews
	MultiviewsMatch Any
</Directory>
<Virtualhost *:80>
	ServerName foo
	<If "req('X-FOO-BAR') != '1'">
		Redirect permanent / http://apache.org/
	</If>
</Virtualhost>
<Virtualhost *:80>
	ServerName bar
</Virtualhost>
-=-=-=-=-=-
Create a file in /var/www. Content doesn't matter, i used test.html

Request the file:
wget http://bar/test -> HTTP/1.1 200 OK
wget http://foo/test -> HTTP/1.1 404 Not Found (Expected: HTTP/1.1 301 Moved Permanently)
wget http://foo/test --header "X-FOO-BAR: 1" -> HTTP/1.1 404 Not Found (Expected HTTP/1.1 200 OK)

In the logs:
==> /var/log/apache2/other_vhosts_access.log <==
bar:80 172.17.42.1 - - [10/Dec/2014:12:48:01 +0000] "GET /test HTTP/1.1" 200 377 "-" "Wget/1.15 (linux-gnu)"
foo:80 172.17.42.1 - - [10/Dec/2014:12:48:02 +0000] "GET /test HTTP/1.1" 404 486 "-" "Wget/1.15 (linux-gnu)"
foo:80 172.17.42.1 - - [10/Dec/2014:12:54:05 +0000] "GET /test HTTP/1.1" 404 486 "-" "Wget/1.15 (linux-gnu)"

==> /var/log/apache2/error.log <==
[Wed Dec 10 12:48:02.859625 2014] [negotiation:error] [pid 48:tid 140525715056384] [client 172.17.42.1:41853] AH00687: Negotiation: discovered file(s) matching request: /var/www/test (None could be negotiated).
[Wed Dec 10 12:54:05.512771 2014] [negotiation:error] [pid 49:tid 140075132299008] [client 172.17.42.1:39714] AH00687: Negotiation: discovered file(s) matching request: /var/www/test (None could be negotiated).

A Redirect without the <If> works. Something else besides a Redirect in the <If> works (using Header for example works and sets a header). However the Redirect in combination with the <If> fails.
Comment 1 Kees Hoekzema 2015-09-28 11:06:41 UTC
retested in 2.5-HEAD, still an issue.
Comment 2 Alex Smith 2018-07-07 16:34:48 UTC
Created attachment 36018 [details]
Propsed patch (first attempt, against 2.4.33)

I also ran into this issue, when attempting to implement automatic upgrading of HTTP requests to the equivalent HTTPS requests only for browsers likely to support SNI. Analysis and details of the attached proposed patch below:


Analysis
--------

The root cause is that the merging of conditional sections (<If> and friends) that are enabled for the request only occurs in the `per-directory' configuration tree, rooted within the applicable vhost. Therefore any conditional server / vhost wide configuration only gets picked up at the point that a mapping to the filesystem is established, which is rather late.

What is happening in the reported scenario is as follows:

Configuration parsing:

* The <If> is treated like a top-level <Directory> section.
* mod_alias stores server / vhost wide aliases and redirects in the `server' configuration tree; there are none in the reported example, due to the handling of the <If> section.
* mod_alias stores the aliases and redirects within each directory-like section, which includes the <If>, in the `per-directory' configuration for that section.

Request processing:

* The `per-directory' configuration for the request starts as that for the top-level for the vhost.
* Conditional (<If) section merging into that `per-directory' configuration is performed.
* In the `translate name' hook, mod_alias looks for applicable aliases and redirects in its `server' configuration. There aren't any.
* In the `type checker' hook, mod_negotiation finds the possible variants and performs a sub-request for each one.
* During the sub-request for the first possible variant, the full `per-directory' configuration is constructed by merging everything relevant for that file.
* The mod_alias `fixups' hook then gets invoked, as part of the sub-request, and looks for applicable aliases and redirects in its part of the merged `per-directory' configuration for the sub-request, finding the conditional redirect that was merged from the <If> and applying it.
* mod_negotiation bails as described due to the sub-request not returning OK, i.e. because it returns the redirect status.

That mod_negotiation bails when the sub-request indicates a redirection is reasonable, although a clearer error would be useful in this case; consider a configuration with a redirect configured for only one of the possible variants.

The long-term fix would be to extend the conditional section processing to the `server' configuration processing, creating `server' <If> sections (instead of `per-directory' <If> sections) when they appear in global / vhost scope, within <Location> sections or as immediate children of other `server' <If> sections. When any are found a request-local copy of the server configuration would have to be taken and those that are enabled merged in. This would also allow e.g. conditional <Location> sections, but would clearly be a large piece of work and would require numerous module interface changes.


Proposed Patch
--------------
The attached patch is a shorter-term fix for just this issue. It makes mod_alias look in the merged `per-directory' configuration in the `translate name' hook if it didn't find a match in the `server' configuration, even though it is only the top-level directory configuration at that stage. Hence, when the <If> is enabled and it has been merged in, the conditional redirect is picked up early enough.

The patch has been taken against 2.4.33 and needs `patch -p1' in the root directory. It applies cleanly to trunk as of now, but I've only had a chance to test it on 2.4.33.
Comment 3 Alex Smith 2018-07-07 16:59:22 UTC
Created attachment 36019 [details]
Suggested improvement for reporting of `redirect of negotiation variant' configuration errors

Additional patch with a tangential improvement for consideration, in case it is useful.

As mentioned in the analysis in #2, when mod_negotiation is performing sub-requests for the possible variants it assumes anything other than a `success' return indicates an error and bails with a 404 Not Found response. If such a sub-request actually fails then that failure will likely be logged as well and it will be clear what the issue is. However the case that a sub-request gives a redirection response is arguably a configuration error that needs to be logged as such. Currently in this case there is little indication of the cause of the negotiation failure, since both the negotiation and redirection configurations are likely valid on their own and hence no other errors or warnings are logged; it is only their combination that is in error.

This patch simply puts in handling for this special case of negotiation failure, logging a more helpful error and returning 500 Internal Server Error.

Apply with `patch -p1' in the root directory. Taken against 2.4.33 but applies cleanly to current trunk. Only tested against 2.4.33 though. Would need a proper error number assigning before inclusion.