[PATCH] Image links based on RFC2397 not supported

[Dirk Stöcker]

Directly included images in the ​rfc:2397 format isn't supported. They don't work in [[Image()]] macro nor in #!html processor.

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACgAAAAoAQMAAAC2MCouAAAAA1BMVEXLQ0MOAUiXAAAAC0lE
QVQIHWMYYQAAAPAAASEIRrcAAAAASUVORK5CYII=

See also #8168 which comes up again, as there is a comma in the string which can't be encoded as %2c, otherwise fix would be easy.

Fix for data: is easy, simply handle it like http:, but the comma is a problem again. Proper argument splitting is required.

macros.

@@ -581 +581 @@
         parts = filespec.split(':')
         url = raw_url = desc = None
         attachment = None
-        if (parts and parts[0] in ('http', 'https', 'ftp')): # absolute
+        if (parts and parts[0] in ('http', 'https', 'ftp', 'data')): # absolute
+            if parts[0] == "data":
+              filespec = filespec.replace("%2C", ",")
             raw_url = url = filespec
             desc = url.rsplit('?')[0]
         elif filespec.startswith('//'):       # server-relative

Simple patch with a workaround for the comma issue.

[Dirk Stöcker]

Following fixes both tickets.

macros.py

@@ -507 +502 @@
         return True
 
     _split_filespec_re = re.compile(r''':(?!(?:[^"':]|[^"']:[^'"])+["'])''')
+    _split_args_re = re.compile(r''',(?!(?:[^"',]|[^"'],[^'"])+["'])''')
 
     def expand_macro(self, formatter, name, content):
         # args will be null if the macro is called without parenthesis.
@@ -514 +510 @@
             return ''
         # parse arguments
         # we expect the 1st argument to be a filename (filespec)
-        args = content.split(',')
+        args = [i.strip('''['"]''')
+                 for i in self._split_args_re.split(content)]
         if len(args) == 0:
             raise Exception("No argument.")
         # strip unicode white-spaces and ZWSPs are copied from attachments
@@ -589 +586 @@
                  for i in self._split_filespec_re.split(filespec)]
         url = raw_url = desc = None
         attachment = None
-        if (parts and parts[0] in ('http', 'https', 'ftp')): # absolute
+        if (parts and parts[0] in ('http', 'https', 'ftp', 'data')): # absolute
             raw_url = url = filespec
             desc = url.rsplit('?')[0]
         elif filespec.startswith('//'):       # server-relative

[Dirk Stöcker]

Should be checked, that it does not break #10562 again.

[Dirk Stöcker]

Replying to dstoecker:

Should be checked, that it does not break #10562 again.

Seems to work at least in my Trac instance.

[Jun Omae]

I think we should only use data: scheme if [wiki] safe_schemes has data: at least.

[Dirk Stöcker]

The "data:" is no external link - it is similar to a local image file with the exception, that it is directly delivered in the HTML code instead of with a second request.

Why should data: be added to safe_schemas? It is never external.

[Jun Omae]

[wiki] safe_schemes option is introduced by #9557. According to ​attack vectors in comment:6:ticket:9557, data: URI can lead XSS. So the option doesn't has data entry, I think.

[Dirk Stöcker]

So actually adding it to safe_schemes would be an error. But this does not affect this here, as it refers only to images. I can imagine, that data usage can cause a lot of issues in many places, so I would not propose it to allow it everywhere (without checking first). But also beside images I don't see much gain anyway. CSS and JavaScript are plaintext and can be embedded directly. Other formats like video and sound are usually to large to embed. Only (small) images are an proper use-case. And actually that's also the only places were I have seen it till now and where we use it.

[Dirk Stöcker]

I assume, that Firefox does not do any XSS stuff when the SVG is referenced as image - otherwise that would be a major bug in firefox. The page you have above needs embedding the SVG directly instead of <img> links.

[Dirk Stöcker]

Could this be applied closing the two bugs now that 1.0.2 is out?

[Ryan J Ollos]

I haven't looked closely at the ticket to see exactly what is required, but usually we add unit tests. I can't commit to reviewing it at the moment, but probably can sometime during milestone:1.0.3.

[Dirk Stöcker]

Any reason why this does not get applied?

[Ryan J Ollos]

I haven't found time to review the patch, and I try to avoid committing changes that I don't fully understand. I'll try to find time to review in milestone:1.0.4. Unit tests are probably needed.

[Dirk Stöcker]

Please ask, when you don't understand the patch!

The first two changes are similar to r11732 and fixes #8168. The third fix is for this ticket, but it requires the previous one to work due the the comma which is always in data URL's.

It's now 3 months for such a simple change and the work already done. 2 months this fix is live in our Trac instance. It's a major discouraging when patches are ignored. Results will simply be, that I wont provide patches at all.

Main discussions about the idea of this fix have already been in #10562. I'm actually not willing to do the same again and again!

[Ryan J Ollos]

I didn't say that I didn't understand it. I don't have time to study it right now. I understand it can be discouraging, but there are lots of important things to fix. I won't commit until I have adequate time to study the patch.

I'd be disappointed to lose your contributions, but other things are more important at the moment, such as getting the next release out.

[Ryan J Ollos]

Splitting the args may be implemnetd as described in comment:13:ticket:8168, which will result in a refactoring of the changes from #10562. IMAGE_MACRO_TEST_CASES will be extended to cover the data scheme: tags/trac-1.0.4/trac/wiki/tests/macros.py@:56#L52​. The Image macro documentation will be extended.

Jun, do you still have any concerns about using the data scheme with the Image macro?

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11773.

[Jun Omae]

That changes look good to me. I have no concerns now about using the data uri with img element.

[Ryan J Ollos]

Committed to 1.0-stable in [13787], merged to trunk in [13788].