779088 - Allow extensions to whitelist PATH_INFO

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Marc Schumann [:Wurblzap]]

This is a regression of bug 243764.

Removing PATH_INFO caused an extension using PATH_INFO to stop working properly. I fixed this locally by way of bug 243764, comment 28, whitelisting page.cgi.

Extensions should be allowed to whitelist certain scripts or pages themselves, so that such a local hack is not necessary. It might be useful to design this whitelisting infrastructure so that the REST API can use it as well.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to allow whitelisting of scripts when removing path_info (v1)

Adds new PATH_INFO_WHITELIST constant that can be updated by extensions via a hook.

dkl

Comment 2

[Frédéric Buclin]

Comment on attachment 647712 [details] [diff] [review]
Patch to allow whitelisting of scripts when removing path_info (v1)

>=== modified file 'Bugzilla/CGI.pm'

>+    if ($self->path_info 
>+        && !grep($_ eq $script, PATH_INFO_WHITELIST)) 

We don't need a constant for now as this constant is empty by default. Also, it would be the first time that a constant in Bugzilla::Constants is controlled by an extension. IMO, you should rather write something like:

    if ($self->path_info) {
        my @whitelist;
        Bugzilla::Hook::process('path_info_whitelist', { whitelist => \@whitelist });
        if (!grep {$_ eq $script} @whitelist) {
            $cgi->redirect(...);
        }
    }

If one day we have a script in our codebase which needs to be whitelisted, then I still prefer the constant to be left unaltered by extensions, and the code above would become:

        my @whitelist = PATH_INFO_WHITELIST;

and then the hook can add stuff to @whitelist instead of altering PATH_INFO_WHITELIST itself.

Also, this hook needs POD in Bugzilla/Hook.pm, which is the main reason of my r-.



>=== modified file 'extensions/Example/Extension.pm'

>+sub path_info_whitelist {
>+    my ($self, $args) = @_;
>+    my $whitelist = $args->{whitelist};
>+    #push(@$whitelist, "index.cgi");
>+}

You should rather use page.cgi as example, as it's more likely the script which will be whitelisted as there are hooks for this script already. Also, no need to comment this line. You can enable it, it's harmless.


Otherwise looks good.

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to allow whitelisting of scripts when removing path_info (v2)

Thanks LpSolit. New patch.

Comment 4

[Frédéric Buclin]

Comment on attachment 650774 [details] [diff] [review]
Patch to allow whitelisting of scripts when removing path_info (v2)

>=== modified file 'Bugzilla/Hook.pm'

>+=head2 path_info_whitelist

In case you didn't notice, all hooks are sorted alphabetically in this file, else the generated POD would be a total mess, with hook names out of order. :) Please move this hook right after page_before_template.


>+Path-Info is of no use for Bugzilla and interacts badly with IIS.
>+Moreover, it causes unexpected behaviors, such as totally breaking
>+the rendering of pages.

This is a pretty scary description and makes it sound like an evil thing. This hook exists for a reason, and we should instead describe why it's useful. Something like:

"By default, Bugzilla removes the Path-Info information from URLs before passing data to CGI scripts. If this information is needed for your customizations, you can enumerate the pages you want to whitelist here."


>+An array of script names that will not be redirected to remove Path-Info.

I'm not sure everybody will understand what you are talking about with "redirected" (yes, we do a $cgi->redirect, but that's technical). Maybe simply:

"An array of script names for which Path-Info will not be removed."

or something like that.



>=== modified file 'extensions/Example/Extension.pm'

>+sub path_info_whitelist {

Here too, subroutines are supposed to be sorted alphabetically. You will note that bug_check_can_change_field() a few lines above it is at the wrong place too, but it was already added by... you! :) Please move them at their right place, else it will quickly be a real mess to find something in this file.


Codewise, your patch works fine, though.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to allow whitelisting of scripts when removing path_info (v3)

Thanks. I completely forgot about the alphabetical order in the methods. I have addressed your requests as well as move other functions in Example/Extension.pm to be in the proper order.

Comment 6

[Frédéric Buclin]

Comment on attachment 650884 [details] [diff] [review]
Patch to allow whitelisting of scripts when removing path_info (v3)

r=LpSolit

Comment 7

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/CGI.pm
modified Bugzilla/Hook.pm
modified extensions/Example/Extension.pm
Committed revision 8342.

Comment 8

[Frédéric Buclin]

Added to relnotes for 4.4.