Closed Bug 199502 Opened 17 years ago Closed 16 years ago

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

Categories

(Bugzilla :: Administration, task, critical)

2.17.3
task
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: justdave, Assigned: burnus)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
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.
well, we'd need to check that they have all hte correct templates too, which is
harder.

Just append en, I think.

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.
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
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.
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
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.
Yeah, OK... you're right :-) Let's just check that the directories exist.

Gerv
Attachment #129167 - Flags: review?(bbaetz)
Mine.
Assignee: justdave → burnus
Target Milestone: --- → Bugzilla 2.18
Attachment #129167 - Flags: review?(justdave)
Could you also change editparams.cgi to say, in the description for this param:

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

Gerv
Attachment #129167 - Attachment is obsolete: true
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.)
Attachment #130008 - Flags: review?(gerv)
+        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 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
> >+	   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');
Attachment #130218 - Flags: review?(gerv)
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 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.
Attachment #130218 - Flags: review?(bbaetz)
Comment on attachment 130218 [details] [diff] [review]
v2.1: Initalize $deft_path by 'default' not 'custom'

r=gerv. Looks good to me.

Gerv
Attachment #130218 - Flags: review?(gerv) → review+
Status: NEW → ASSIGNED
Flags: approval?
a=myk.  Please clear your review request of bbaetz before checking in if you
don't need him to review anymore.
Flags: approval? → approval+
Attachment #130218 - Flags: review?(bbaetz)
Attachment #130008 - Flags: review?(gerv)
Attachment #129167 - Flags: review?(justdave)
Attachment #129167 - Flags: review?(bbaetz)
FIXED
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.118; previous revision: 1.117
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
What about, even as extra option, to automatically determine de languages
parameter by listing the existing directories under the template directory?
This was the cause of bug 226217. 

Gerv
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.