Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10668 closed defect (fixed)

Cut and paste of attachment name leads to zero-width character in string, preventing image macro from displaying attachment

Reported by: Ryan J Ollos <ryano@…> Owned by: Jun Omae
Priority: normal Milestone: 1.0
Component: wiki system Version: 0.12.3
Severity: normal Keywords: attachment link
Cc: Branch:
Release Notes:

The Image wiki macro is robust w.r.t. Zero-width whitespace unicode characters (as can be found after copy/paste of attachment names from the Trac pages)

API Changes:
Internal Changes:

Description (last modified by Christian Boos)

The following scenario might qualify as user error, but I could imagine some other users making the error, so I thought it might be worthwhile to report it.

The scenario is, I frequently upload image attachments with long filenames. Being lazy and not wanting to type out some long filename, I highlight and copy the filename from the attachment section of the ticket, and paste the filename into the comment box, inside a call to the Image macro. Frequently, I find that the Image macro can't link to the attachment, even though everything looks fine in the wiki markup.

The issue seems to be that a zero width UTF character (E2 80 8B) can get inserted into the comment box during the paste operation, but they are not visible in the comment dialog. Clicking on the attachment though, we get URLs like like /attachment/ticket/1228/MilestoneProgressBar.png%E2%80%8B, which is what pointed me towards the issue.

I'll show some screen captures and then attempt to demonstrate the issue. I tested this out before posting here in demo-0.12:ticket/5002.





Related branches

Attachments (5)

Attachment.png (7.7 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
AddAndThenEditAComment.png (9.3 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
AttachmentNotLinked.png (6.2 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
InvalidAttachment.png (5.4 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
MilestoneProgressBar.png (8.4 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.

Download all attachments as: .zip

Change History (29)

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: Attachment.png added

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: AddAndThenEditAComment.png added

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: AttachmentNotLinked.png added

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: InvalidAttachment.png added

comment:1 by Ryan J Ollos <ryano@…>, 8 years ago

Description: modified (diff)

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: MilestoneProgressBar.png added

comment:2 by Ryan J Ollos <ryano@…>, 8 years ago

Example:

  1. Highlight MilestoneProgressBar.png in the attachment section (#no1).
  2. Copy.
  3. Paste the filename as the argument for a call to the image macro.

comment:3 by Christian Boos, 8 years ago

That's a bit unfortunate, this character is there to avoid having the download icon and link split on different lines.

What would happen with a Soft_hyphen or even a Zero-width_joiner?

comment:4 by Ryan J Ollos <ryano@…>, 8 years ago

Summary: Cut and paste of attachment name leads to zero-width in string, preventing image macro from displaying attachmentCut and paste of attachment name leads to zero-width character in string, preventing image macro from displaying attachment

I can do some testing tonight or tomorrow night and get back to you.

in reply to:  3 ; comment:5 by Remy Blank, 8 years ago

Replying to cboos:

That's a bit unfortunate, this character is there to avoid having the download icon and link split on different lines.

Even worse, it's there to avoid having an empty <a></a> pair for the download icon. IIRC some browsers don't render the padding correctly when there's nothing between the opening and closing tag, therefore hiding the background image. With the ZWS, it renders correctly in all browsers.

What would happen with a Soft_hyphen or even a Zero-width_joiner?

I haven't tried, but I expect that copy / pasting will also copy them.

comment:6 by Ryan J Ollos <ryano@…>, 8 years ago

Something that I found odd is that the relative href is /attachment/ticket/10668/MilestoneProgressBar.png%E2%80%8B, but the error message is Attachment 'ticket:10668: MilestoneProgressBar.png​ does not exist. The space is in front of the filename in the error message, but the unicode character is appended to the end of the href.

in reply to:  6 comment:7 by Ryan J Ollos <ryano@…>, 8 years ago

Replying to Ryan J Ollos <ryano@…>:

The space is in front of the filename in the error message, but the unicode character is appended to the end of the href.

It looks like the space in front of the filename has nothing to do with the issue I'm reporting here. I had never noticed this before, but a space is added between the realm and resource in the error message, e.g. here is one I saw today, Attachment 'ticket:1227: trac.ini.20120419' does not exist. It seems like that space is probably unintended.

in reply to:  5 comment:8 by Jun Omae, 8 years ago

Replying to cboos:

That's a bit unfortunate, this character is there to avoid having the download icon and link split on different lines.

Using content css property (e.g. content: "\00200b") also avoids the splitting. However IE6/7 don't support the property. And the text inserted by the property cannot be copied to clipboard, but excepted Opera and IE.

Replying to rblank:

Even worse, it's there to avoid having an empty <a></a> pair for the download icon. IIRC some browsers don't render the padding correctly when there's nothing between the opening and closing tag, therefore hiding the background image. With the ZWS, it renders correctly in all browsers.

.trac-rawlink { *zoom: 1 } css hack avoids hiding the background image for an empty <a></a> pair on IE6/7.

I worked in d3e199d/jomae.git.

  • Works fine on Firefox 12, Chrome 19 and Safari 5.1.
  • U+200B is copied on Opera 11 and IE8. The icon and link aren't split.
  • The icon and link are split on IE6/7. U+200B isn't copied.

Thoughts?

Last edited 8 years ago by Jun Omae (previous) (diff)

comment:9 by Christian Boos, 8 years ago

Testing d3e199d/jomae.git, for me the raw attachment icon is sometimes split from the link (FF 12, IE9). But worse, the original problem is not fixed for Opera or IE9 for example. So the proposed fix is probably not the final word on the topic, though it's a progress for some browsers (Chrome and FF).

comment:10 by Christian Boos, 8 years ago

Regardless of whether we should support having the icon wrap on the next line or not (\u200b should in fact facilitate this splitting, no?), a different approach would be to simply be robust against the possible presence of \u200b in the link when parsing it… (approach I've used for #10713).

comment:11 by Christian Boos, 8 years ago

Description: modified (diff)
Keywords: attachment link added
Milestone: 0.13

While waiting for the Attach branch feature…

in reply to:  10 comment:12 by Jun Omae, 8 years ago

Replying to cboos:

…, a different approach would be to simply be robust against the possible presence of \u200b in the link when parsing it… (approach I've used for #10713).

+1, repos:jomae.git:t10668/strip-zwsp.

…Well, It would be good to strip the leading and trailing white spaces, not only ZWSP characters, from the first argument of [[Image]] macro. A file name and URL as the first argument normally have no leading spaces and no trailing spaces.

  • trac/wiki/macros.py

    diff --git a/trac/wiki/macros.py b/trac/wiki/macros.py
    index 0927265..818849a 100644
    a b class ImageMacro(WikiMacroBase):  
    496496        args = content.split(',')
    497497        if len(args) == 0:
    498498            raise Exception("No argument.")
    499         filespec = args.pop(0)
     499        filespec = args.pop(0).strip()
    500500
    501501        # style information
    502502        size_re = re.compile('[0-9]+(%|px)?$')

comment:13 by Christian Boos, 8 years ago

Yes, I approve both changes.

comment:14 by Remy Blank, 8 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:15 by Jun Omae, 8 years ago

Milestone: 1.0-triage1.0
Resolution: fixed
Status: newclosed

In [11064], strips unicode white-spaces from the first argument in the macro.

comment:16 by Jun Omae, 8 years ago

Owner: set to Jun Omae

comment:17 by Christian Boos, 8 years ago

Resolution: fixed
Status: closedreopened

Oops, the added test fails for me with Python 2.7.2 (Windows). It passes with 2.5 and 2.6 though.

  • (a) expected vs. (b) actual

    a b  
    11<p>
    2 <img src="/browser/%C2%AB%20test%C2%A0%C2%BB.png?format=raw" alt="source:\xab test\xa0\xbb.png" title="source:\xab test\xa0\xbb.png" />
     2<img src="/browser/%C2%AB%20test%C2%A0%C2%BB.png%E3%80%80%E2%80%8B?format=raw" alt="source:\xab test\xa0\xbb.png\u3000" title="source:\xab test\xa0\xbb.png\u3000" />
    33</p>

One-liner:

  • (a) expected vs. (b) actual

    a b  
    1 <img src="/browser/%C2%AB%20test%C2%A0%C2%BB.png?format=raw" alt="source:\xab test\xa0\xbb.png" title="source:\xab test\xa0\xbb.png" />
     1<img src="/browser/%C2%AB%20test%C2%A0%C2%BB.png%E3%80%80%E2%80%8B?format=raw" alt="source:\xab test\xa0\xbb.png\u3000" title="source:\xab test\xa0\xbb.png\u3000" />

comment:18 by Jun Omae, 8 years ago

Oh, I don't have tested with 2.7….

U+200B is whitespace on Python 2.4 - 2.6, however, it isn't on Python 2.7. refs http://bugs.python.org/issue13391 and http://en.wikipedia.org/wiki/Whitespace_character#Unicode.

I reworked at repos:jomae.git:t10668/strip-whitespace, strips [\s\u200b] from leading and trailing. Tested on Python 2.5 - 2.7.

comment:19 by Christian Boos, 8 years ago

Thanks for investigating! I found the problem too late yesterday to dig myself, so I'm glad you narrowed it down. However, I think that for clarity we could afford having a trac.util.text.stripws helper function doing this (and maybe precompile the regexp).

comment:20 by Jun Omae, 8 years ago

Absolutely. I added stripws in trac.util.text, repos:jomae.git:t10668/strip-whitespace.2.

comment:21 by Remy Blank, 8 years ago

Tested with 2.5 - 2.7, works fine. Please apply!

comment:22 by Jun Omae, 8 years ago

Resolution: fixed
Status: reopenedclosed

Thanks! Applied in [11074].

comment:23 by Christian Boos, 8 years ago

Release Notes: modified (diff)

comment:24 by Ryan J Ollos <ryan.j.ollos@…>, 8 years ago

#10865 is a related ticket for addressing problems with ZWSP characters in TracLinks.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.