On a site where I'm using project with ifac (recently upgraded) my user did not have the "upload files" nor "view uploaded files" permissions. And yet when users who did have that permission would send me emails I would see the link to the patch or whatever the Attachment was. My feeling is that somewhere in the mix of modules should be a check for whether or not the user that is receiving the mail has the permissions to view uploaded files and, if they don't, then the "Attachment: " line would be removed.
This is a relatively uncommon configuration - if I can view and subscribe to the issue queue then my role probably has the permission to view attached files.
However, it seemed like it was worth noting since it allows unprivileged users to get some information about files and possibly actually download the files. The Project/Component that I'm assigning this to may not be right, but it's hard for me to know given my level of knowledge of this area of code.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | mail_attachment_perm.patch | 14.24 KB | hunmonk |
| #4 | mail_attachment_perm.patch | 13.71 KB | hunmonk |
| #3 | mail_attachment_perm_0.patch | 11.55 KB | hunmonk |
| #2 | mail_attachment_perm.patch | 10.88 KB | hunmonk |
Comments
Comment #1
dwwThanks for the report. This is definitely a bug -- in fact, if IFAC had actually been publically released by now, this would be a security issue because of the access bypass, so I'm bumping the priority. And, you almost had the project right. ;) IFAC is entirely the domain of project_issue. Because project_issue is the thing generating and sending the emails, this is where the bug lies. It shouldn't blindly assume the user has permission to view/upload files, it should gracefully degrade in this regard. It's possible there's a similar bug in comment_upload, but I'm not sure. For now, let's call this issue about the fact that when project_issue generates those emails, it's not careful enough checking the permissions. Once this is fixed, we can then figure out if there are any other parts of the IFAC chain that need help in the same regard.
Comment #2
hunmonk commentedattached is untested. i implemented caching where i was able, so that each kind of mail is only built once.
i hate this code, and i don't want to deploy it on drupal.org (all those user_loads() are going to hurt performance), but i don't see another decent solution if we really want to enforce this.
Comment #3
hunmonk commentedthis approach i can live with, because it doesn't have much of a performance cost in the case where anon or auth user has 'view uploaded files' perm. the code checks for that condition first, and if auth users would have the perm, it skips all the individual user checking.
the code supports two different versions of output, one with and one without attachments displayed -- and it caches the output after it builds each the first time, for greater efficiency.
this is not critical -- it does not affect the drupal.org deployment, and the the code is only on a dev branch.
Comment #4
hunmonk commentedkeeping up with HEAD.
also, the fix implemented over at http://drupal.org/node/188969 was a bit incomplete, so i've corrected in here also. basically, it's cleaner and less error-prone to clone the current issue and use that for the history.
this has been tested pretty heavily, but could use a bit more testing and a code review before commit.
Comment #5
aclight commentedThe patch works well (with 1 exception, below) and addresses the original problem. The code looks good to me.
The problem is that it seems that this patch breaks the Message-Id header value, in that the commend id is not added to the message id. This breaks the threading of the messages within Gmail. So, for example, the thread will show up in this order within gmail:
original issue
comment #2
comment #1
whereas if I revert the patch I get:
original issue
comment #1
comment #2.
I'll look into where this is getting broken.
Comment #6
hunmonk commentedok, we need to find the last and next-to-last cid while we're looping through building the history. attached does this, and then uses them to set the header appropriately. i didn't test this small change.
Comment #7
aclight commentedI tested patch in #6 and it works as advertised. Threading works properly, and users without view uploads permission don't get the file attachment links in issue emails.
Some of the headers (specifically References) may not be perfect according to http://www.ietf.org/rfc/rfc2822.txt?number=2822, but I don't see any problems with keeping them as they are right now (and, indeed, how they have been for quite some time). We can create a separate issue to fix the headers to be up to standards if necessary.
This is RTBC.
Comment #8
hunmonk commentedfixed in HEAD, installed on drupal.org
Comment #9
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.