Item10438: lighttpd incompatibility - found in viewfile, but could be there for other things too
Priority: Normal
Current State: Closed
Released In: 1.1.4
Target Release: patch
pathinfo and redirect infos are quite different among http servers. lighttpd does it yet differently ... which triggers this error.
Situation is that actually
all scripts are accessed as part of a redirect. Viewfile now thinks this must be a 4XX error page.
Infact the REQUEST_STATUS is a good one namely 200. At this point the
filename
parameter is not read which results in an
attachment-not-found oops.
This patch seems to fix it. Can somebody confirm?
--- lib/Foswiki/UI/Viewfile.pm (revision 10878)
+++ lib/Foswiki/UI/Viewfile.pm (working copy)
@@ -44,7 +44,7 @@
my $fileName;
my $pathInfo;
- if ( defined( $ENV{REDIRECT_STATUS} ) && defined( $ENV{REQUEST_URI} ) ) {
+ if ( defined( $ENV{REDIRECT_STATUS} ) && $ENV{REDIRECT_STATUS} != 200 && defined( $ENV{REQUEST_URI} ) ) {
# this is a redirect - can be used to make 404,401 etc URL's
# more foswiki tailored and is also used in TWikiCompatibility
$pathInfo = $ENV{REQUEST_URI};
# ignore parameters, as apache would.
$pathInfo =~ s/^(.*)(\?|#).*/$1/;
$pathInfo =~ s|$Foswiki::cfg{PubUrlPath}||; #remove pubUrlPath
--
MichaelDaum - 03 Mar 2011
Anybody running lighttpd that can confirm?
Has the patch been tested on Apache?
You did not set target release. You want this to block 1.1.3?
--
KennethLavrsen - 06 Mar 2011
Added some more knowledgeable people on the waiting for list.
--
MichaelDaum - 08 Mar 2011
Patch looks reasonable to me.
--
CrawfordCurrie - 13 Mar 2011
this does not fix viewfile enough
for eg, on apache
-
http://foswiki.org/bin/viewfile/System/ProjectLogos/foswiki-logo.gif
works, but it doesn't on lighttpd - it complains that ? attachment does not exist.
Michael - I'm using the core/toold/lighttpd.pl to test, as i'd like to avoid the time to learn all of lighttpd atm (i'd rather learn nginx, but don't really have time for that either)
can you please give me a config file for lighttpd to reproduce your issue? or better yet, patch the lighttpd.pl in trunk?
--
SvenDowideit - 17 Mar 2011
Hi Sven, attached my config. The
? attachment does not exist
is exactly what the above patch was fixing for me.
--
MichaelDaum - 17 Mar 2011
ta - it doesn't fix it for me, but I was testing the non-redirect version. I'll play with it this weekend.
--
SvenDowideit - 18 Mar 2011
I ended up not having time to deal with this yet, so there's no reason to block the 1.1.3 release. bummer, but thats life.
--
SvenDowideit - 24 Mar 2011
Could anybody shed some light on what exactly the affected code area is all about? It seems as if it is only used to implement error redirect pages or something ... so checking the http status is actually a must, isnt it?
After long thinking I came to the conclusion that this
if
branch checking for a redirect is quite useless.
Instead of fixing a feature that nobody seems to be able to explain, which is buggy and probably not documented anyway the more sensible solution would be to remove this
if
branch all together.
... which under any circumstances is a release blocker.
--
MichaelDaum - 24 Mar 2011
I don't understand how this is a release blocker unless there is a test case, either in the
TestCases, or a live example that shows that it is broken. Everything works fine as far as I can tell on Apache. In any event, making a change in this area without a test case to prove correct operation this late in the release cycle is very risky. There are a number of very critical fixes, including some effecting security, being held up for something without a demonstrated failure on Apache or UnitTests.
Please consider creating a test case so that the before/after impact of the change can be verified.
I've added the comments from the code to the above example. So this change needs to be tested thorough on an apache system running with redirect to viewfile, for both found and not-found files. It
appears that it would only apply to 4xx codes, but it needs to be tested.
--
GeorgeClark - 24 Mar 2011
Actually reading our
InstallationGuide, and release notes, I don't see where Lighttpd is a supported environment. Doesn't this need a feature proposal to add lighttpd as a supported environment complete with appropriate documentation.
--
GeorgeClark - 24 Mar 2011
I have not seen any description of an error mode that will prevent a patch release
You can set this to urgent or very urgent or deadly. 1.1.3 is so much better than 1.1.2 that it is about time to get it out. And the already fixed bugs are more urgent to get out than fixing this corner case that 99% of us cannot even reproduce.
If this was so bloody urgent it would have been fixed urgently when it was raised. No matter what state this item has 1.1.3 is going out when the major languages are back at near 100%
--
KennethLavrsen - 24 Mar 2011
George, from what I see these lines of code are simply bit rot with no real value at all. No surprise there isn't even a unit test for the code
as is.
--
MichaelDaum - 25 Mar 2011
All this means is that yes, we can't rush it, cos we need to understand it before we change it, and then test all the cases it was intended for - clearly not a rushjob.
--
SvenDowideit - 25 Mar 2011