Closed
Bug 199502
Opened 22 years ago
Closed 21 years ago
It's possible to take down Bugzilla by changing the languages param
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: justdave, Assigned: burnus)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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
Reporter | ||
Comment 2•22 years ago
|
||
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•22 years ago
|
||
well, we'd need to check that they have all hte correct templates too, which is
harder.
Just append en, I think.
Reporter | ||
Comment 4•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 6•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 8•22 years ago
|
||
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•22 years ago
|
||
Yeah, OK... you're right :-) Let's just check that the directories exist.
Gerv
Assignee | ||
Comment 10•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #129167 -
Flags: review?(bbaetz)
Assignee | ||
Comment 11•21 years ago
|
||
Mine.
Assignee: justdave → burnus
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Updated•21 years ago
|
Attachment #129167 -
Flags: review?(justdave)
Comment 12•21 years ago
|
||
Could you also change editparams.cgi to say, in the description for this param:
"Available languages are en, de, fr, ..."?
Gerv
Assignee | ||
Comment 13•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #129167 -
Attachment is obsolete: true
Assignee | ||
Comment 14•21 years ago
|
||
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)
Comment 15•21 years ago
|
||
+ 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•21 years ago
|
||
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
Assignee | ||
Comment 17•21 years ago
|
||
> >+ 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');
Assignee | ||
Updated•21 years ago
|
Attachment #130218 -
Flags: review?(gerv)
Reporter | ||
Comment 18•21 years ago
|
||
did you swap the wrong one? I would think $cust_path is the one you would want
to have 'custom' on the end of. :)
Assignee | ||
Comment 19•21 years ago
|
||
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 20•21 years ago
|
||
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+
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Flags: approval?
Comment 21•21 years ago
|
||
a=myk. Please clear your review request of bbaetz before checking in if you
don't need him to review anymore.
Flags: approval? → approval+
Assignee | ||
Updated•21 years ago
|
Attachment #130218 -
Flags: review?(bbaetz)
Assignee | ||
Updated•21 years ago
|
Attachment #130008 -
Flags: review?(gerv)
Assignee | ||
Updated•21 years ago
|
Attachment #129167 -
Flags: review?(justdave)
Attachment #129167 -
Flags: review?(bbaetz)
Assignee | ||
Comment 22•21 years ago
|
||
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: 21 years ago
Resolution: --- → FIXED
Comment 23•21 years ago
|
||
What about, even as extra option, to automatically determine de languages
parameter by listing the existing directories under the template directory?
Comment 24•21 years ago
|
||
This was the cause of bug 226217.
Gerv
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•