If a user attempts to access an attachment for which they does not have permission to view the resulting error screen will display the Log In and Forgotten password links in the header and footer This can be see on the BMO site (version 3.4.4+) To replicate find an attachment you are denied access to and attempt to display it.

Unless I miss something, I'm unable to reproduce this issue with 3.5.2 and 3.4.4. Do you have a URL?

I attempted to view an attachment from irc, since I had a attachment already up I had just replaced the attach_id. What I failed to notice was the url itself had redirected originally to include the bug id at the beginning the url. Like this: https://bug<bugid>.bugzilla.mozilla.org/attachment.cgi?id=<attachmentid> I did not replace the bugid. So if the bug_id is left incorrect, but the attach id is change it will try and open the attach but throw the access denied error as it should and references the bug actually associated with the attachment. Which is where the screen shot came from on BMO. So in summary, this shouldn't occur under normal conditions but if someone manually enters the attachment id and has no permissions to view it they will get the incorrectly displayed page, though still correctly denying access. If the bug_id is also changed the page will display normally.

Ah ok, I didn't try this testcase. Reopening for further testing when I have some time.

I can reproduce the issue. I'm surprised attachment.cgi didn't redirect to correct_urlbase first before throwing the error.

In bug 38862, I assumed that if you were not on the bug's alternate host, you were on the main host, from where we could safely check your credentials. This is a good assumption most of the time, except when each bug has its own host for attachments and you hack the URL to view one attachment from another bug's alternate host, as described in comment 2. In that case, we need to redirect you to the main host to check your credentials first. I don't think that's exploitable as the login form points to the main host, but I want to handle this edge-case correctly anyway for 3.4.x and tip.

As said in my previous comment, we need to redirect the user to the main host to check his credentials when the alternate host mismatches the expected host.

Comment on attachment 419417 [details] [diff] [review] patch, v1 This logic is getting really confusing. Could we possibly reverse it all so that the viewing only happens in the $bug_id case? (That is, reverse all the if, elsif, else?) In any case, this looks OK.

(In reply to comment #7) > that the viewing only happens in the $bug_id case? (That is, reverse all the > if, elsif, else?) An alternative I had in mind was: if (!$cgi->url_is_attachment_base($bug_id)) { if ($cgi->url_is_attachment_base) { $cgi->redirect_to_urlbase; } .... } else { .... } Does it look more understandable to you? Personally, I found it more confusing than my current patch.

Yeah, I find that more confusing, too. I was thinking of making the very top condition "if ($cgi->url_is_attachment_base($bug_id)" and putting what's currently in the "else" into there, and then reversing the logic in general.

Ah, I see. Hum, I'm not sure the benefit is real, though. :)

Yeah, I think it would help with readability, though. Usually negative conditions are hard to read and positive conditions are easy to read.

Here is the patch with the logic reversed. That's the one I'm going to commit.

The patch doesn't apply cleanly on the 3.4 branch as bug 523495 didn't land on this branch, and so $cgi->url_is_attachment_base is not available there. As this bug is not too severe, I'm only committing this patch for 3.6. Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.167; previous revision: 1.166 done

Comment on attachment 419566 [details] [diff] [review] alternate patch (reversed logic), v1.1 That looks way better, thank you. :-)