Closed
Bug 143574
Opened 22 years ago
Closed 22 years ago
file error - cache failed to write [foo].tmpl: Permission denied
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: myk, Assigned: bbaetz)
Details
(Keywords: perf, Whiteboard: want for 2.16rc2)
Attachments
(1 file, 1 obsolete file)
2.40 KB,
patch
|
myk
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
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...
Component: Bugzilla-General → bugzilla.org
Comment 2•22 years ago
|
||
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).
Component: bugzilla.org → Bugzilla-General
Assignee | ||
Comment 3•22 years ago
|
||
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...
Assignee | ||
Comment 4•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Comment 5•22 years ago
|
||
Didn't we release note this? Is the release note going away now?
Assignee | ||
Comment 6•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 83177 [details] [diff] [review] v1 r= justdave
Attachment #83177 -
Flags: review+
Updated•22 years ago
|
Whiteboard: want for 2.16rc2
Reporter | ||
Comment 8•22 years ago
|
||
>we won't use cached tempaltes for buglists + other places where a format is
>involved.
Why not?
Assignee | ||
Comment 9•22 years ago
|
||
> 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.
Reporter | ||
Comment 10•22 years ago
|
||
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•22 years ago
|
||
> 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?
Assignee | ||
Comment 12•22 years ago
|
||
> 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.
Reporter | ||
Comment 13•22 years ago
|
||
>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.
Assignee | ||
Comment 14•22 years ago
|
||
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.
Reporter | ||
Comment 15•22 years ago
|
||
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."
Assignee | ||
Comment 16•22 years ago
|
||
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
Assignee | ||
Comment 17•22 years ago
|
||
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?
Assignee | ||
Comment 18•22 years ago
|
||
OK, try this. Can someone else confirm that detainting this is OK?
Attachment #83177 -
Attachment is obsolete: true
Reporter | ||
Comment 19•22 years ago
|
||
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?
Attachment #86046 -
Flags: review+
Comment 20•22 years ago
|
||
Comment on attachment 86046 [details] [diff] [review] v2 yup. r= justave looks good.
Attachment #86046 -
Flags: review+
Assignee | ||
Comment 21•22 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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
•