Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference
Summary: OPACBaseURL sometimes set by ENV variable and not system preference
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 8952
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-17 06:09 UTC by David Cook
Modified: 2015-06-04 23:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.37 KB, patch)
2014-01-20 01:05 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.68 KB, patch)
2014-02-25 05:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.72 KB, patch)
2014-06-16 03:20 UTC, David Cook
Details | Diff | Splinter Review
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.87 KB, patch)
2014-06-16 04:21 UTC, David Cook
Details | Diff | Splinter Review
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.92 KB, patch)
2014-06-19 10:01 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference (3.99 KB, patch)
2014-06-20 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2014-01-17 06:09:45 UTC
Currently, OPACBaseURL is being set by the $ENV{'SERVER_NAME'} variable with the following code (from 2009) executed in Auth.pm's sub "get_template_and_user":

OPACBaseURL => ($in->{'query'}->https() ? "https://" : "http://") . $ENV{'SERVER_NAME'} . ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443" : "80") ? '' : ":$ENV{'SERVER_PORT'}"),

Basically, this checks whether or not HTTP or HTTPS was used, then uses an environmental variable to fill in the SERVER_NAME and the SERVER_PORT (if it's not 443 or 80).

Unfortunately, it appears that this SERVER_NAME is set by the URL used to access Koha, rather than the ServerName provided by Apache. 

In many cases, this is probably fine, but it means that the OPACBaseURL can change, if you have multiple aliases or if you use an IP address to connect to Koha.

Moreover, if you're using a proxy server, it seems to pick up the local domain of the proxy server and NOT the domain from the URL provided by a user.

The domain from your URL will appear instead in $ENV{'HTTP_X_FORWARDED_HOST'}.

I propose that we just change this variable declaration to:

OPACBaseURL => ($in->{'query'}->https() ? "https://" : "http://") . C4::Context->preference("OPACBaseURL")

The comments from Bug 2152 seem to indicate other times when the system preference should be used over the environmental variable as well.
Comment 1 David Cook 2014-01-20 01:05:22 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2014-01-20 04:23:58 UTC
HTTP_X_FORWARDED_HOST
That reminds me of bug 6800.
Comment 3 M. Tompsett 2014-02-25 05:26:45 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2014-02-25 07:29:48 UTC
How does this interfere with bug 8952 in the Passed QA list (for 5 months or so..) ?
Comment 5 M. Tompsett 2014-02-25 15:55:50 UTC
Good point, Marcel.

The third patch on bug 8952 has this:
# Set the using_https variable for templates
# FIXME Under Plack the CGI->https method always returns 'OFF'
my $https = $in->{query}->https();
my $using_https = (defined $https and $https ne 'OFF') ? 1 : 0;

I think perhaps this patch should use this variable ($using_https) instead of checking $in->{query}->https() itself, and add a depends to the bug.
Comment 6 David Cook 2014-06-16 03:20:00 UTC Comment hidden (obsolete)
Comment 7 David Cook 2014-06-16 03:21:16 UTC
Thought about switching this back to Signed Off, but made minor changes to take into account bug 8952, so leaving that up to M. Tompsett and/or the QA team.

(Also improved the commit message to be more accurate)
Comment 8 M. Tompsett 2014-06-16 03:37:04 UTC
So, should you obsolete one of the patches, and add a dependency? Once you do that, I'll test again. Though, you might want to copy the FIXME from the using_https area down there too, just so people don't forget how it affects multiple areas.
Comment 9 David Cook 2014-06-16 04:21:12 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2014-06-19 10:01:47 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-06-20 13:36:17 UTC
Created attachment 29101 [details] [review]
Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system preference

This patch changes how the OPACBaseURL parameter gets set in the subroutine
get_template_and_user in Auth.pm.

Currently, it's being set by the $ENV{'SERVER_NAME'} variable. In many
cases, this will probably match the URL that the user uses to access a
page. However, this causes problems with reverse proxies.

There are ways to compensate for proxy servers (such as inspecting
other variables set by the web server), but such a solution seems
a bit convoluted...especially since we already use the system preference
OPACBaseURL in many other parts of Koha.

We probably shouldn't be passing OPACBaseURL from Auth.pm at all, and
instead use the Koha TT plugin and using_https param to determine
protocol. However, that's outside the scope of this bug/patch.

This patch is just meant to fix an existing bug.

I did leave the $ENV{'SERVER_NAME'} as a full back if OPACBaseURL isn't
set, but that's it.

_TEST PLAN_

Before applying:

1) Clear your OPACBaseURL preference
2) Perform a search in the OPAC
3) Click on or hover over the orange RSS icon
4) Note that the URL used for the RSS links is either:
  a) The same URL you used to access Koha (no reverse proxy)
  b) The ServerName from your Koha apache conf which isn't the
  same URL you used to access Koha (reverse proxy)
5) Add an OPACBaseURL that isn't the same as the actual OPAC URL
6) Note that the OPACBaseURL system preference has no effect here

After applying the patch:

7) Refresh the page
8) Note that the URL you see now is actually the OPACBaseURL system
preference that you set
9) Clear your OPACBaseURL system preference
10) Refresh your search page
11) Note that the URL has reverted back to the URL that you saw before
(either the original Koha site URL or the Koha ServerName defined
in Apache and not the URL of the proxy)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 David Cook 2014-06-23 07:05:24 UTC
(In reply to Jonathan Druart from comment #11)
> Created attachment 29101 [details] [review] [review]
> Bug 11575 - OPACBaseURL sometimes set by ENV variable and not system
> preference
> 
> This patch changes how the OPACBaseURL parameter gets set in the subroutine
> get_template_and_user in Auth.pm.
> 
> Currently, it's being set by the $ENV{'SERVER_NAME'} variable. In many
> cases, this will probably match the URL that the user uses to access a
> page. However, this causes problems with reverse proxies.
> 
> There are ways to compensate for proxy servers (such as inspecting
> other variables set by the web server), but such a solution seems
> a bit convoluted...especially since we already use the system preference
> OPACBaseURL in many other parts of Koha.
> 
> We probably shouldn't be passing OPACBaseURL from Auth.pm at all, and
> instead use the Koha TT plugin and using_https param to determine
> protocol. However, that's outside the scope of this bug/patch.
> 
> This patch is just meant to fix an existing bug.
> 
> I did leave the $ENV{'SERVER_NAME'} as a full back if OPACBaseURL isn't
> set, but that's it.
> 
> _TEST PLAN_
> 
> Before applying:
> 
> 1) Clear your OPACBaseURL preference
> 2) Perform a search in the OPAC
> 3) Click on or hover over the orange RSS icon
> 4) Note that the URL used for the RSS links is either:
>   a) The same URL you used to access Koha (no reverse proxy)
>   b) The ServerName from your Koha apache conf which isn't the
>   same URL you used to access Koha (reverse proxy)
> 5) Add an OPACBaseURL that isn't the same as the actual OPAC URL
> 6) Note that the OPACBaseURL system preference has no effect here
> 
> After applying the patch:
> 
> 7) Refresh the page
> 8) Note that the URL you see now is actually the OPACBaseURL system
> preference that you set
> 9) Clear your OPACBaseURL system preference
> 10) Refresh your search page
> 11) Note that the URL has reverted back to the URL that you saw before
> (either the original Koha site URL or the Koha ServerName defined
> in Apache and not the URL of the proxy)
> 
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Should this be marked as Passed QA now?
Comment 13 Jonathan Druart 2014-07-08 13:31:12 UTC
(In reply to David Cook from comment #12)
> Should this be marked as Passed QA now?

Yes, sorry.
Comment 14 Tomás Cohen Arazi 2014-07-08 14:00:07 UTC
Are we introducing problems for Plack?
Comment 15 Robin Sheat 2014-07-08 23:14:03 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> Are we introducing problems for Plack?

Probably not. Really, OPACBaseURL should include the method so there's no need for any of that $using_https crap. But that's a story for another day.

Also, as this promotes the use of the syspref over the envvar, it's probably better for plack.
Comment 16 David Cook 2014-07-08 23:38:46 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> Are we introducing problems for Plack?

I don't think so. Rather, we're highlighting an existing problem that Marcel noted in bug 8952. (Marcel added the FIXME in bug 8952 and I'm just copying it over to some relevant areas.)

I imagine that most people are using proxies in front of Plack so this might be relevant for fixing this issue: https://github.com/plack/Plack/wiki/How-to-detect-reverse-proxy-and-SSL-frontend

Plack::Middleware::ReverseProxy seems to just reset environmental variables depending on the headers it gets from the reverse proxy (so long as they're configured).

http://cpansearch.perl.org/src/MIYAGAWA/Plack-Middleware-ReverseProxy-0.15/lib/Plack/Middleware/ReverseProxy.pm

I suppose another alternative might be just to set the environmental variable explicitly in the vhost (if you're using Apache) or whatever else you need to with another proxy.

This may or may not be relevant: 

http://httpd.apache.org/docs/2.2/mod/mod_ssl.html

We don't use Apache for our proxy server, so I'm not really sure, but this info might give other people (who are using Plack and a front end proxy) some ideas.

I haven't played with Plack yet but I've done a fair bit of reading.

By the way, here is the CGI->https() method from 5.16:

#### Method: https
# Return the value of HTTPS, or
# the value of an HTTPS variable, or
# the list of variables
####
'https' => <<'END_OF_FUNC',
sub https {
    my ($self,$parameter) = self_or_CGI(@_);
    if ( defined($parameter) ) {
        $parameter =~ tr/-a-z/_A-Z/;
        if ( $parameter =~ /^HTTPS(?:_|$)/ ) {
            return $ENV{$parameter};
        }
        return $ENV{"HTTPS_$parameter"};
    }
    return wantarray
        ? grep { /^HTTPS(?:_|$)/ } keys %ENV
        : $ENV{'HTTPS'};
}
END_OF_FUNC

It looks like it's just looking for a HTTPS environmental variable, which Plack::Middleware::ReverseProxy can provide. Otherwise, I suppose it depends on how you've configured Plack?
Comment 17 Tomás Cohen Arazi 2014-07-15 15:19:04 UTC
Patch pushed to master.

Thanks David!
Comment 18 M. Tompsett 2014-09-20 11:30:07 UTC
(In reply to Robin Sheat from comment #15)
> (In reply to Tomás Cohen Arazi from comment #14)
> > Are we introducing problems for Plack?
> 
> Probably not. Really, OPACBaseURL should include the method so there's no
> need for any of that $using_https crap. But that's a story for another day.
> 
> Also, as this promotes the use of the syspref over the envvar, it's probably
> better for plack.

What's even better for Plack is using the OPACBaseURL with the protocol included all the time, like I just tweaked the code for in bug 5010. ;) Well, to be honest, I don't know if it is better for plack, but that using_https stuff is reduced a bit now. :)