Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#11773 closed defect (fixed)

[PATCH] Image links based on RFC2397 not supported

Reported by: Dirk Stöcker Owned by: Dirk Stöcker
Priority: normal Milestone: 1.0.5
Component: wiki system Version: 1.0.1
Severity: normal Keywords:
Cc: leho@… Branch:
Release Notes:

The data URL scheme can be used with the Image macro.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

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.

    old new  
    581581        parts = filespec.split(':')
    582582        url = raw_url = desc = None
    583583        attachment = None
    584         if (parts and parts[0] in ('http', 'https', 'ftp')): # absolute
     584        if (parts and parts[0] in ('http', 'https', 'ftp', 'data')): # absolute
     585            if parts[0] == "data":
     586              filespec = filespec.replace("%2C", ",")
    585587            raw_url = url = filespec
    586588            desc = url.rsplit('?')[0]
    587589        elif filespec.startswith('//'):       # server-relative

Simple patch with a workaround for the comma issue.

Attachments (0)

Change History (25)

comment:1 by Dirk Stöcker, 6 years ago

Following fixes both tickets.

  • macros.py

    old new  
    507502        return True
    508503
    509504    _split_filespec_re = re.compile(r''':(?!(?:[^"':]|[^"']:[^'"])+["'])''')
     505    _split_args_re = re.compile(r''',(?!(?:[^"',]|[^"'],[^'"])+["'])''')
    510506
    511507    def expand_macro(self, formatter, name, content):
    512508        # args will be null if the macro is called without parenthesis.
     
    514510            return ''
    515511        # parse arguments
    516512        # we expect the 1st argument to be a filename (filespec)
    517         args = content.split(',')
     513        args = [i.strip('''['"]''')
     514                 for i in self._split_args_re.split(content)]
    518515        if len(args) == 0:
    519516            raise Exception("No argument.")
    520517        # strip unicode white-spaces and ZWSPs are copied from attachments
     
    589586                 for i in self._split_filespec_re.split(filespec)]
    590587        url = raw_url = desc = None
    591588        attachment = None
    592         if (parts and parts[0] in ('http', 'https', 'ftp')): # absolute
     589        if (parts and parts[0] in ('http', 'https', 'ftp', 'data')): # absolute
    593590            raw_url = url = filespec
    594591            desc = url.rsplit('?')[0]
    595592        elif filespec.startswith('//'):       # server-relative

comment:2 by Dirk Stöcker, 6 years ago

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

in reply to:  2 comment:3 by Dirk Stöcker, 6 years ago

Replying to dstoecker:

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

Seems to work at least in my Trac instance.

comment:4 by lkraav <leho@…>, 6 years ago

Cc: leho@… added

comment:5 by Dirk Stöcker, 6 years ago

Milestone: 1.0.2
Version: 1.0.1

comment:6 by Dirk Stöcker, 6 years ago

Summary: Image links based on RFC2397 not supported[PATCH] Image links based on RFC2397 not supported

comment:7 by Jun Omae, 6 years ago

Milestone: 1.0.2next-stable-1.0.x

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

comment:8 by Dirk Stöcker, 6 years ago

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.

comment:9 by Jun Omae, 6 years ago

[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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:10 by Dirk Stöcker, 6 years ago

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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:11 by Dirk Stöcker, 6 years ago

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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:12 by Dirk Stöcker, 6 years ago

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

comment:13 by Ryan J Ollos, 6 years ago

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.

comment:14 by Ryan J Ollos, 6 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:15 by Dirk Stöcker, 6 years ago

Any reason why this does not get applied?

comment:16 by Ryan J Ollos, 6 years ago

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.

comment:17 by Dirk Stöcker, 6 years ago

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!

comment:18 by Ryan J Ollos, 6 years ago

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.

comment:19 by Ryan J Ollos, 6 years ago

Milestone: next-stable-1.0.x1.0.4

comment:20 by Ryan J Ollos, 6 years ago

Milestone: 1.0.41.0.5

comment:21 by Ryan J Ollos, 6 years ago

Description: modified (diff)
Release Notes: modified (diff)

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?

comment:22 by Ryan J Ollos, 6 years ago

Proposed changes in log:rjollos.git:t11773.

comment:23 by Jun Omae, 6 years ago

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

comment:24 by Ryan J Ollos, 6 years ago

Resolution: fixed
Status: assignedclosed

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

comment:25 by Ryan J Ollos, 6 years ago

Owner: changed from Ryan J Ollos to Dirk Stöcker

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Dirk Stöcker.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Dirk Stöcker 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.