Three changes: Allowed headers, generation of environment variables, remove some INFO logging

jm009 commented 6 years ago

Based on what i wrote here: https://github.com/jFastCGI/jfastcgi/issues/23

Whitelist of allowed client headers instead of blacklist.
Initialize SCRIPT_NAME and PATH_INFO environment variables independently from servlet mapping, by searching the php script file in the URL with a regular expression.
Remove some logging on info level from FastCGIHandler.java.

Sorry for having all these changes in one single commit. I have Nextcloud running with this patch :-)

Should I have asked, before replacing the blacklist for allowed client headers by a whitelist? Sorry for my doing before talking :-( I saw the whitelist approach in the Tomcat CGIServlet and to me it looks like a good solution. I prefer to be overcautious when it is about publicly accessible web stuff.

And something else: I had downloaded jfastcgi-2.1.jar from https://sourceforge.net/projects/jfastcgi/. It contains different java package names (JRebel?)

Thank you for jFastCGI!

By the way, my Nextcloud calendar (in fact about 10 calendars) now opens in the web browser in less than 20 seconds, instead of more than 30 seconds before. The test server is not very fast and limited to one CPU core. Mozilla 57 and Chromium probably also will load the page much faster then my Debian 9.2 Stretch Firefox 52.5. So there is much room for improvement. But anyway, I would like to bang my head against the wall, to have to wait so long for some hundred calendar entries to be displayed on my screen... I don't want to complain about Nextcloud. Nextcloud is great. demo.nextcloud.com runs much faster. So I definitely have to look for a better test server ;-) It's just the fact, that gigahertz processors and megabit Internet connections are not a guarantee, to allow fast data editing ;-)

More on how I run nextcloud: https://help.nextcloud.com/t/success-nextcloud-runs-with-jfastcgi-on-tomcat-9-0-1-beta/23935

jm009 commented 6 years ago

Con someone help me? continuous-integration/travis-ci/pr shows a NullPointerException...

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.jfastcgi.client.FastCGIHandlerTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.243 sec <<< FAILURE!
testProxyHeader(org.jfastcgi.client.FastCGIHandlerTest)  Time elapsed: 0.214 sec  <<< ERROR!
java.lang.NullPointerException
    at org.jfastcgi.client.FastCGIHandler.setEnvironment(FastCGIHandler.java:286)
    at org.jfastcgi.client.FastCGIHandler.handleRequest(FastCGIHandler.java:222)
    at org.jfastcgi.client.FastCGIHandler.service(FastCGIHandler.java:195)
    at org.jfastcgi.client.FastCGIHandlerTest.testProxyHeader(FastCGIHandlerTest.java:83)

Lines 285 and 286 are

String documentRoot = req.getRealPath("/");
documentRoot = documentRoot.substring(0, documentRoot.length() - 1);

That means req.getRealPath("/") returns null... I have no clue about this testing context and would be thankful, if someone who is more familiar with it could help out... What kind of servlet context is not available in the testing environment?

jrialland commented 6 years ago

Hi, thanks you for your tremendous work ! I have a lot of work right now but i'll take the time to review your patch this week hopefully - Concerning the broken test, it uses a mocked instance of RequestAdapter, so you have to define what is shall return by addding something like Mockito.when(request.getRealPath("/")).thenReturn("/opt/whatever"); around line 50 in the org.jfastcgi.client.FastCGIHandlerTest class.

jFastCGI / jfastcgi

Three changes: Allowed headers, generation of environment variables, remove some INFO logging #24