Opened 5 years ago

Closed 3 years ago

# "Invalid attachment' displayed on every page after bookmarking attachments page

Reported by: Owned by: rjollos rjollos high BookmarkPlugin normal jun66j5, hasienda 0.12

### Description

After uploading an attachment named to ticket 1253, I see "Invalid attachment" displayed when trying to navigate to any realm. The following was captured when navigating to the /report page:

The attachment name is hardware mode error.png. The ticket summary is BLS Program Execution Hardware Fault. Disabling the BookmarkPlugin eliminates the error.

Here is the traceback from the log file.

2012-02-10 15:12:40,675 Trac[main] WARNING: HTTPNotFound: 404 Invalid Attachment (Attachment 'ticket:: 1253' does not exist.)
2012-02-10 15:12:41,984 Trac[main] ERROR: Exception caught while post-processing request:
Traceback (most recent call last):
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/web/main.py", line 276, in dispatch
self._post_process_request(req)
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/web/main.py", line 365, in _post_process_request
f.post_process_request(req, *(None,)*extra_arg_count)
File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 173, in post_process_request
self.render_bookmarker(req)
File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 291, in render_bookmarker
params = self.__format_name(req, url)
File "build/bdist.linux-x86_64/egg/tracbookmark/__init__.py", line 238, in __format_name
name = get_resource_description(self.env, resource, 'summary')
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/resource.py", line 333, in get_resource_description
return manager.get_resource_description(resource, format, **kwargs)
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 610, in get_resource_description
return Attachment(self.env, resource).description
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 126, in __init__
self._fetch(self.resource.id, db)
File "/usr/local/python26_trac12/lib/python2.6/site-packages/Trac-0.12.3-py2.6.egg/trac/attachment.py", line 155, in _fetch
_('Invalid Attachment'))
ResourceNotFound: Attachment 'ticket:: 1253' does not exist.


### comment:1 Changed 5 years ago by rjollos

Attempting to reproduce the issue in my development environment.

### comment:2 Changed 5 years ago by rjollos

It looks like I had bookmarked /attachment/ticket/1253, which is the critical step that allows the error to be reproduced. I had intended to bookmark /ticket/1253, but this looks to be a real error, nonetheless.

### comment:3 Changed 5 years ago by rjollos

I've dug a little deeper into this. The problem can be reproduced when bookmarking a page in the attachment realm that is not an actual attachment. Here are two examples:

• /attachment/ticket/1/?action=new&attachfilebutton=Attach+file
• /attachment/ticket/1/

The first example is the href for the page that allows you to upload an attachment. The second example is the href for the page that loads after an attachment is added - i.e. the page that lists all the attachments for a ticket.

This error is likely to occur infrequently because looking at this practically, there is little reason to bookmark these hrefs. However, I did this by accident and it blocked me from accessing any Trac resource, which is a pretty severe failure. We can either prevent these resources from having a bookmark icon, or add special cases to handle these.

### comment:4 Changed 4 years ago by rjollos

• Summary changed from "Invalid attachment' displayed on every page after uploading an attachment to "Invalid attachment' displayed on every page after bookmarking attachments page

### comment:5 follow-up: ↓ 8 Changed 4 years ago by rjollos

• Cc pkline removed

Here is one problematic case that I propose to deal with by just blacklisting the path. /attachment/ticket/n (note there is no trailing forward slash) for any ticket n, results in:

This will be a difficult resource to bookmark since it apparently doesn't have an ID when the trailing backslash is not present. It also seems pretty useless to bookmark, so blacklisting it seems like a fine solution.

### comment:6 Changed 4 years ago by rjollos

(In [11672]) Refs #9785: Refactored code to use the helper functions get_resource_shortname and get_resource_summary.

Here is the patch I'm proposing. It removes the bookmark star from the ^/attachment/ticket/n$page, but leaves the bookmark list context menu. This way, the offending page can't be bookmarked. Before committing a change, I'd expand the blacklisted_paths variable to support a list of regexes. I'm just posting this for feedback at the moment. • ## trunk/tracbookmark/__init__.py  bookmark_path = re.compile(r'/bookmark') path_match = re.compile(r'/bookmark/(add|delete|delete_in_page)/(.*)') blacklisted_path = re.compile(r'^/attachment/ticket/[0-9]+$') image_map = { 'on'  : 'bookmark_on.png', 'off' : 'bookmark_off.png', } resource = self._get_resource_uri(req) bookmark = self.get_bookmark(req, resource) if bookmark: img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on'])) anchor = tag.a(img, id='bookmark_this', href=req.href.bookmark('delete', resource), title='Delete Bookmark') else: img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off'])) anchor = tag.a(img, id='bookmark_this', href=req.href.bookmark('add', resource), title='Bookmark this page') # Blacklisted paths can't be bookmarked if not self.blacklisted_path.match(req.path_info): if bookmark: img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['on'])) anchor = tag.a(img, id='bookmark_this', href=req.href.bookmark('delete', resource), title='Delete Bookmark') else: img = tag.img(src=req.href.chrome('bookmark/' + self.image_map['off'])) anchor = tag.a(img, id='bookmark_this', href=req.href.bookmark('add', resource), title='Bookmark this page') elm = tag.span(anchor, id='bookmark', title='') req.chrome.setdefault('ctxtnav', []).insert(0, elm) add_script(req, 'bookmark/js/tracbookmark.js') add_stylesheet(req, 'bookmark/css/tracbookmark.css') elm = tag.span(anchor, id='bookmark', title='') req.chrome.setdefault('ctxtnav', []).insert(0, elm) menu = tag.ul('', id='bookmark_menu', title='')

### comment:8 in reply to: ↑ 5 ; follow-up: ↓ 9 Changed 4 years ago by jun66j5

Here is one problematic case that I propose to deal with by just blacklisting the path. /attachment/ticket/n (note there is no trailing forward slash) for any ticket n, results in:

Yes. /attachment/wiki/WikiStart has the same problem. So the URL of attachments list page must have a trailing slash.

This will be a difficult resource to bookmark since it apparently doesn't have an ID when the trailing backslash is not present. It also seems pretty useless to bookmark, so blacklisting it seems like a fine solution.

+1. Good solution. Your patch in comment:7 basically looks good.

### comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 4 years ago by rjollos

Yes. /attachment/wiki/WikiStart has the same problem. So the URL of attachments list page must have a trailing slash.

Oh, thanks, I hadn't run across that one before. So do you think I should just blacklist ^/attachment/[^/]+/[^/]+$? That seems to work well for /attachment/wiki/WikiStart and /attachment/ticket/1 at least. ### comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 4 years ago by jun66j5 Replying to rjollos: Oh, thanks, I hadn't run across that one before. So do you think I should just blacklist ^/attachment/[^/]+/[^/]+$ ?

The regular expression doesn't match /attachment/wiki/WikiName/SubWiki.

I think it would be simple as the following.

        # Ignore the page which begins with /attachment/ and doesn't have a trailing slash
if not (req.path_info.startswith('/attachment/') and not req.path_info.endswith('/')):


### comment:11 in reply to: ↑ 10 Changed 4 years ago by rjollos

The regular expression doesn't match /attachment/wiki/WikiName/SubWiki.

Thanks for catching that. I'll implement the change that you propose.

### comment:12 Changed 4 years ago by rjollos

Related to t:#10280.

### comment:13 Changed 4 years ago by rjollos

• Owner changed from saigon to rjollos
• Priority changed from normal to high

### comment:14 Changed 4 years ago by rjollos

• Status changed from new to assigned

### comment:15 follow-up: ↓ 16 Changed 3 years ago by rjollos

This is a bit more complex than I thought initially. First, we need to deal with ambiguity of whether the page that is bookmarked has a trailing slash or not. That is dealt with by:

• Right-strip slashes in _get_resource_uri, so that /attachment/parent_realm/parent_id/attachment_id and /attachment/parent_realm/parent_id/attachment_id/ are both treated as the same bookmark.
• Append a trailing slash to the href when the page is /attachment/parent_realm/parent_id.

So now, it doesn't matter whether /attachment/parent_realm/parent_id/attachment_id or /attachment/parent_realm/parent_id/attachment_id/ is bookmarked, the bookmark will point to /attachment/parent_realm/parent_id/.

Second, when getting the resource name in _format_name, we need to handle the case that that path could be to either an attachment list page or an attachment resource. To handle this, we assume first that the path points to an attachment resource. If that resource is not found, assume it points to an attachment list page.

parent = Resource(path[2], '/'.join(path[3:-1]))
resource = Resource(realm, path[-1], False, parent)
if resource_exists(self.env, resource):
else:
parent = Resource(path[2], '/'.join(path[3:]))
resource = Resource(realm, parent=parent)


I'll check-in this fix after I do a bit more testing.

### comment:16 in reply to: ↑ 15 Changed 3 years ago by rjollos

... To handle this, we assume first that the path points to an attachment resource. If that resource is not found, assume it points to an attachment list page.

This didn't quite cover all of the cases since we want to add the missing class to attachments that don't exist. Here is what I finally came up with:

# Assume it's a file and check existence
parent = Resource(path[2], '/'.join(path[3:-1]))
resource = Resource(realm, path[-1], False, parent)
try:
Attachment(self.env, resource)
except ResourceNotFound:
# Assume it's an attachment list page
# and check existence
parent = Resource(path[2], '/'.join(path[3:]))
resource = Resource(realm, parent=parent)
if resource_exists(self.env, resource):
# Appending slash required for Trac < 1.0, t:#10280
if not query_string:
href += '/'
else:
# Assume it's a missing attachment
class_ = 'missing ' + realm


The need for such complicated code seems to be a product of how we store the resource information in the database, and we may want to consider changing the schema, as hasienda has already suggested for the similar VotePlugin in #10942.

### comment:17 Changed 3 years ago by rjollos

I've iterated further, and arrived at:

# Assume a file and check existence
parent = Resource(path[1], '/'.join(path[2:-1]))
resource = Resource(realm, path[-1], parent=parent)
if not resource_exists(self.env, resource):
# Assume an attachment list page and check existence
parent = Resource(path[1], '/'.join(path[2:]))
if resource_exists(self.env, parent):
resource = Resource(realm, parent=parent)
if 'action=new' in query_string:
% {'id': parent.id})
elif not query_string:
# Trailing slash needed for Trac < 1.0, t:#10280
href += '/'
else:
# Assume it's a missing attachment
class_ = 'missing ' + realm


The code in comment:16 didn't quite work because resource_exists checks whether the attachment path exists in the file system, so if a page such as /attachment/wiki/TracEnvironment is bookmarked, but attachments have never been added to the resource, then resource_exists for the code in comment:16 would return false.

The logic of the above code is:

1. Assume the path points to an attachment and check for its existence.
2. If resource_exists returns false for (2), assume the path points to the attachment page for a resource and check for its existence.
3. If resource_exists returns false for (2), assume the attachment is missing.

### comment:18 Changed 3 years ago by rjollos

#11043 is a follow-on ticket in which edit and delete pages will be blacklisted from having a bookmark icon, at least by default.

### comment:19 follow-up: ↓ 20 Changed 3 years ago by rjollos

• Resolution set to fixed
• Status changed from assigned to closed

(In [13029]) Fixes #9785:

• Allow attachments, attachment list pages and attachment add pages to be bookmarked. Each has a unique description.
• Prefix the version to wiki page description with an @ followed by the version number, if the version is contained in the URL
• Prefix milestones with "Milestone " so that they can be readily differentiated from wiki pages.

Tested with Trac 0.11.7 and 1.0.2dev

### comment:20 in reply to: ↑ 19 Changed 3 years ago by rjollos

Tested with Trac 0.11.7 and 1.0.2dev

This was not quite true. I tested with Trac 0.11-stable (i.e. 0.11.8dev), and while reviewing a patch from hasienda recently I noticed his comment that resource_exists isn't in Trac <= 0.11.7. I'll have to backport the function to make BookmarkPlugin compatible back to Trac 0.11.0.