143574 - file error - cache failed to write [foo].tmpl: Permission denied

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

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

I'm getting these errors:

file error - cache failed to write list-rdf.rdf.tmpl: Permission denied

They happen in a variety of situations including loading the following URL:

http://landfill.tequilarista.org/bugzilla-tip/buglist.cgi?format=rdf&bug_id=5,6,7

(Note that Mozilla will give you a parsing error because the HTML error message
is not valid XML.  View the source to look at the actual TT error.)

I also get this error when I make a change to a tip install template then re-run
checksetup.pl.

Comment 1

[Bradley Baetz (:bbaetz)]

This is because the following are all true:

1) a webservergroup isn't set
2) the user running checksetup isn't the same as the user the webserver runs as
3) a template file changed without rerunnning checksetup.

The causes the template compilation code to try to recompile, but since the
webserver doesn't have write access to that directory, it fails.

Note that this does not require root access; if teh template files are up to
date, then the webserver doesn't need write access. Its tsep 3 which is important.

-> bugzilla.org so that justdave reruns checksetup on that directory.

The alternate fix is to make the data/template dirs world writable for teh
non-webservergroup case. People were against me doing that, but given how much
else has to be world writable if you don't set up a webservergroup, world
writable templates are the least of our worries...

Comment 2

[Dave Miller [:justdave]]

bzzzt, try again.

Running checksetup.pl on that directory is the last thing I did before logging
out of landfill when I updated this afternoon.  I ran it twice in a row in fact,
because it gave me a bunch of "converting such and such" messages the first time
and I was just checking that the second time would just give me the usual stuff
(which it did).

Comment 3

[Bradley Baetz (:bbaetz)]

OK, got it. The filename we get from validate output format is tainted, so the
'require $filename' in the template toolkit fails the taint checks.

This causes an error, so the toolkit recompiles the file, and then stores it. It
doesn't need to reevaluate it again, since its already in memory, so it keeps
going without an error, as long as it could store it on disk.

The fix is to trick_taint the output file.

Needed for 2.16, and I'm going to make the dir world writable in the non
webserver case. Although if that had been the case already, we would never have
detected this...

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, here we go. As a side effect, this adds progess messages in checksetup,
makes data/template and world writable if you don't have a webservergroup set.

The real bug fix is to check that the type of the file was a valid type from
localconfig, and then trick_taint it. Why were we accepting different types in
the first place, btw?

The alternate fix would be to, instead of doing readdir, check for the
existance of $script-$format.$type.tmpl for every type in keys $::contenttypes.
Unfortunately, this is done in a separate GetOutputFormats routine for the
error message, so I can't do that.

I also filed bug 143604 on format=html not working

Comment 5

[Matthew Tuck [:CodeMachine]]

Didn't we release note this?  Is the release note going away now?

Comment 6

[Bradley Baetz (:bbaetz)]

No, this is a separate issue to what we relnoted. Its a perf issue too, because
we won't use cached tempaltes for buglists + other places where a format is
involved.

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 83177 [details] [diff] [review]
v1

r= justdave

Comment 8

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

>we won't use cached tempaltes for buglists + other places where a format is
>involved.

Why not?

Comment 9

[Bradley Baetz (:bbaetz)]

> Why not?

Because TT does eval { require $filename; } to load a compiled tempalte, and if
$filename is tainted, then the require fails, and so it decides it needs to
recompile the template from scratch.

Comment 10

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

Comment on attachment 83177 [details] [diff] [review]
v1

>-                  'contenttype' => $::contenttypes->{$2} || "text/plain" , 
>+                  'contenttype' => $::contenttypes->{$2} , 

There seems to be something missing from the comment you made
when you added the attachment.	Why are you doing this?

Comment 11

[Matthew Tuck [:CodeMachine]]

> No, this is a separate issue to what we relnoted.

Maybe but if you make non-webservergroup world writeable the issue we relnoted
will go away, won't it?

Comment 12

[Bradley Baetz (:bbaetz)]

> Why are you doing this?

because if we want a format, it needs to exist in localconfig - that why we have
the stuff there at all, isn't it? I've added a |next unless| thing above, to
match. text/plain is just a really bad guess. This means that a simple
text/plain template (and we don't have any of those, unless you cuold mail
templates, and we don't have any formats for those, yet) has to be called
-simple.html.tmpl, but thats what we're doing anyway.

>Maybe but if you make non-webservergroup world writeable the issue we relnoted
>will go away, won't it?

It will work, and so I wouldn't have noticed this problem, but we still won't
use teh compiled template, because the var will still be tainted, so we'll be
recompiling all the time.

Comment 13

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

>because if we want a format, it needs to exist in localconfig - that why we
>have the stuff there at all, isn't it?

Localconfig only contains filename extension -> content type mappings.  It
doesn't contain any information about formats themselves.

>text/plain is just a really bad guess.

Formats can be added to an installation by creating a new template file and
updating localconfig with the filename extension -> content type mapping (if it
doesn't already exist).  The purpose of "text/plain" is to give administrators a
clue about the problem when they create a new template file but neglect to
update localconfig with the mapping for it.  Otherwise they would get an error
saying the format does not exist, which provides no clue that the problem is
with the content type and not the format file.

Ideally, GetOutputFormats should return all formats whether a content type
exists or not and let its caller determine what to do in this situation
(probably return an error letting the user know a content type could not be
determined).  I filed that as bug 148133.  In the meantime, though, don't make
administrators' lives harder.

Comment 14

[Bradley Baetz (:bbaetz)]

But text/plain is the wrong answer - I don't think we even have text/plain
templates in the tree, apart from teh mail stuff.

At least this way they get an error message. Its a one line change to
localconfig, which is hardly a major issue for someone who is writing their own
formatted template.

Comment 15

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

Adding the line to localconfig is easy, but so is forgetting to add it,
especially when many new formats don't require it (because their mappings
already exist in localconfig).

In that situation it makes much less sense to show administrators a *misleading*
error message about their format not existing than to show them the raw output
of their format, which immediately demonstrates to them that the output is being
correctly generated but that there is a problem with its content type.

The point is not that "text/plain" is a reasonable default when one is not
defined, the point is that using that content type makes it much easier for
administrators to figure out they need to add a line in localconfig than if they
were to receive a message saying "that format does not exist" right after they
added the file (leading them to believe there is a problem with the file
itself).  Showing them their content as text/plain is a much better "error message."

Comment 16

[Bradley Baetz (:bbaetz)]

OK, I'm going to have to think about this WRT the taint checking (Which is the
reason I'm doing this in the first place). I don't think a user cna add .. in
there (because we're reading the directory contents and matching, not crating a
file name and checking). I want to be sure, though

Comment 17

[Bradley Baetz (:bbaetz)]

OK, I _think_ I'm convinced that trick_tainting the results from the directory
listing is OK. If someone could add a new file with a strange format to do
something odd, then they could just as well modify an existing one. I think.
Does someone want to confirm that for me?

Comment 18

[Bradley Baetz (:bbaetz)]

Attachment: v2

OK, try this. Can someone else confirm that detainting this is OK?

Comment 19

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

Comment on attachment 86046 [details] [diff] [review]
v2

Confirmed, since the fixPerms call gives the same permissions to the directory
as it does to the files (i.e. if someone can write to the template directory,
they can write over an existing template file).

r=myk

Dave, does your review still apply?

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 86046 [details] [diff] [review]
v2

yup.

r= justave

looks good.

Comment 21

[Bradley Baetz (:bbaetz)]

Fixed, branch and trunk:

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.3; previous revision: 1.169.2.2
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.6; previous revision: 1.149.2.5
done

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.173; previous revision: 1.172
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.155; previous revision: 1.154
done