199502 - It's possible to take down Bugzilla by changing the languages param

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Dave Miller [:justdave]]

The languages param is a comma-separated list of the language packs you wish to
use on your Bugzilla site.  In order to list something there, you have to have a
directory by that name within the template directory.

Problem is, there is no validation check to ensure that the directory names you
enter in that param actually exist.  And if you fail to list "en" and you don't
have anything else there, it blindly sets it, and then you can't get back in to
fix it unless you have shell access.

This is a major problem if you have someone with admin rights that doesn't have
shell access.

Comment 1

[Gervase Markham [:gerv]]

Perhaps we should change how this works? Instead of explicitly listing "en",
there's an implicit "en" on the end of whatever's there.

So, by default, the field is empty, and you list additional language packs
there. If you want en to have precedence over some of them, you just list it
again, in the middle.

Hmm. Would that be more complicated for the admin to understand?

Gerv

Comment 2

[Dave Miller [:justdave]]

defparams has a mechanism for validating params.

We just need a validation routine which verifies that the directories you listed
actually exist.  Pretty basic I'd think.

Comment 3

[Bradley Baetz (:bbaetz)]

well, we'd need to check that they have all hte correct templates too, which is
harder.

Just append en, I think.

Comment 4

[Dave Miller [:justdave]]

what if they remove en or they don't want it used?  I thought that was the whole
point of the param.

Checking that the directories exist should be enough.  If they install a bad
template set that's the sysadmin's problem.  The problem trying to be prevented
here is to keep someone who does have access to tweakparams but doesn't have
shell access on the machine from screwing it up.

Comment 5

[Gervase Markham [:gerv]]

I'm going off that idea, actually. 

We should just check that en appears in the list somewhere, and assume that they
haven't been stupid enough to delete some of the important default templates
(header, footer etc.).

Gerv

Comment 6

[Dave Miller [:justdave]]

what if they don't want 'en'?  Suicide on a cvs install, but perfectly
reasonable on a stable release with a fully-localized template set available.

Comment 7

[Gervase Markham [:gerv]]

So perhaps we need to check for the existence of those templates which are
required to get a copy of editparams.cgi working. At the moment, that's the
header and footer, and their dependencies. Hmm - still a fairly long list.

> what if they don't want 'en'?

If they have a fully localised set available, there's no harm in having "en" as
well. It's not like deleting the templates will save disk space...

Gerv

Comment 8

[Dave Miller [:justdave]]

But having it available in that list allows a client browser to specify that
they want English and get it.

If the admin wants to customize their site, and the site is Spanish, and they
don't want to duplicate their work, they might disable the English templates and
only use the Spanish ones.  If they're forced to leave English in the list then
a user can still get to the English templates, even though they might be lacking
the customizations they did.

Comment 9

[Gervase Markham [:gerv]]

Yeah, OK... you're right :-) Let's just check that the directories exist.

Gerv

Comment 10

[Tobias Burnus]

Attachment: Test for existance of the template directories

Comment 11

[Tobias Burnus]

Mine.

Comment 12

[Gervase Markham [:gerv]]

Could you also change editparams.cgi to say, in the description for this param:

"Available languages are en, de, fr, ..."?

Gerv

Comment 13

[Tobias Burnus]

Attachment: v2: Increase usebility by showing the languages available.

Comment 14

[Tobias Burnus]

Comment on attachment 130008 [details] [diff] [review]
v2: Increase usebility by showing the languages available.

> "Available languages are en, de, fr, ..."?
| +	      'Available languages: ' . find_languages() ,

I think I should change this to the proposed
  "Available languages are ".find_languages."."

But I'm first waiting for other nits... (A "and" would be nice: 'de, en and
fr', but I think this is not that easy and thus probably not worth doing so for
editparams.cgi.)

Comment 15

[GavinS]

+        my $deft_path = File::Spec->catdir('template', $lang, 'custom');
+        my $cust_path = File::Spec->catdir('template', $lang, 'custom');


Should the first of these lines end with: | 'default'); | ?

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 130008 [details] [diff] [review]
v2: Increase usebility by showing the languages available.

>>+sub check_languages {
>+    my @languages = split /,/, trim($_);
>+    if(!scalar(@languages)) {
>+       return "You need to specify a language tag."

This isn't localisable - but I believe the other functions have this problem,
so never mind.

>+    }
>+    foreach my $language (@languages) {
>+       if(   ! -d 'template/'.trim($language).'/custom' 
>+          && ! -d 'template/'.trim($language).'/default') {

You shouldn't check for the existence of the "custom" directory; it's not
required.

>+    foreach my $lang (@langdirs) {
>+        next if($lang =~ /^CVS$/i);
>+        my $deft_path = File::Spec->catdir('template', $lang, 'custom');
>+        my $cust_path = File::Spec->catdir('template', $lang, 'custom');

Are these supposed to be identical? ;-)

Gerv

Comment 17

[Tobias Burnus]

Attachment: v2.1: Initalize $deft_path by 'default' not 'custom'

> >+	   if(	 ! -d 'template/'.trim($language).'/custom' 
> >+	      && ! -d 'template/'.trim($language).'/default') {
>
> You shouldn't check for the existence of the "custom" directory; it's not
> required.
I do: if(either custom or default exists) then ok, adding a not before the ()
expands to:
   if( (not custom) AND (not default) ) then error

> >+	    my $deft_path = File::Spec->catdir('template', $lang, 'custom');
> >+	    my $cust_path = File::Spec->catdir('template', $lang, 'custom');
>
> Are these supposed to be identical? ;-)
Of cause not, but I wanted to wait for some nits before updating that file.

Interdiff:
-	 my $deft_path = File::Spec->catdir('template', $lang, 'default');
+	 my $deft_path = File::Spec->catdir('template', $lang, 'custom');

Comment 18

[Dave Miller [:justdave]]

did you swap the wrong one?  I would think $cust_path is the one you would want
to have 'custom' on the end of. :)

Comment 19

[Tobias Burnus]

Comment on attachment 130218 [details] [diff] [review]
v2.1: Initalize $deft_path by 'default' not 'custom'

> did you swap the wrong one?
I think I initially did, but then I corrected it (and missed the interdiff ;).
Anyway, the patch is correct and can be reviewed.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 130218 [details] [diff] [review]
v2.1: Initalize $deft_path by 'default' not 'custom'

r=gerv. Looks good to me.

Gerv

Comment 21

[Myk Melez [:myk] [@mykmelez]]

a=myk.  Please clear your review request of bbaetz before checking in if you
don't need him to review anymore.

Comment 22

[Tobias Burnus]

FIXED
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.118; previous revision: 1.117

Comment 23

[Oscar Manuel Gómez Senovilla]

What about, even as extra option, to automatically determine de languages
parameter by listing the existing directories under the template directory?

Comment 24

[Gervase Markham [:gerv]]

This was the cause of bug 226217. 

Gerv