#1828 closed defect (wontfix)

Problem with linked images' display in rst docs from trac

Reported by: mk.fg Owned by: zooko
Priority: minor Milestone: undecided
Component: website Version: n/a
Keywords: website docs Cc: mk.fraggod@…, zooko
Launchpad Bug:

Description (last modified by zooko)

For example, it can be seen on docs/specifications/file-encoding.rst. Following code appears there:

<img alt="file-encoding1.svg" src="file-encoding1.svg" />

URL in "src" attribute now leads to HTML page of trac (with embedded image), but should lead to raw image itself for it to be rendered.

Change History (23)

comment:1 Changed at 2012-10-25T14:06:16Z by mk.fg

  • Cc mk.fraggod@… added
  • Keywords website docs added

comment:2 Changed at 2012-10-25T15:55:42Z by davidsarah

Hmm. I think the syntax will work for local files, and we don't want to break that. Perhaps the trac can be configured to serve those files raw, but I don't know whether it deliberately avoids doing that as an XSS defence.

comment:3 Changed at 2012-10-25T18:14:44Z by mk.fg

I vaguely remember that the images were displayed on docs in the past, so I'd guess maybe some configuration bits from darcs plugin were not applied to git.

Ideally, I imagine it might have a whitelist of mime-types (like "image/*") or file patterns (like "*.{jpg,png,svg}") to serve raw, but lacking that I can imagine a following workaround (nginx config syntax):

location ~ ^/trac/tahoe-lafs/browser/git/docs/(\S+\.(svg|png|jpg|gif))$ {
  uwsgi_param REQUEST_URI /trac/tahoe-lafs/export/master/git/docs/$1;
  uwsgi_param PATH_INFO /trac/tahoe-lafs/export/master/git/docs/$1;

  uwsgi_pass unix:/run/uwsgi/trac.sock;
}

location / {
  ... block for the rest of the urls
}

(nginx seem to be advertised in the response headers)

I think it'd only introduce a little bit of unexpected behavior for users who might browse git tree and expect intermediate page for images, not the raw entities, but I think it shouldn't be that much of a problem either (browsers seem to handle images w/o html wrapping).

comment:4 Changed at 2012-10-25T18:22:57Z by zooko

  • Cc zooko added
  • Owner set to zooko
  • Status changed from new to assigned

comment:5 Changed at 2012-10-25T18:24:10Z by zooko

mk-fg: I'm willing to try some tweak to the nginx/uwsgi configuration, but what should it be, exactly? To make all .svg's and .jpg's be served raw by nginx? Or maybe configure trac to render them when it serves them?

comment:6 Changed at 2012-10-25T18:52:14Z by mk.fg

Apologies for snippet being unhelpful, should've explained it with words.

The above tweak doesn't make nginx serve files as such, they are still served by trac, just as per the following link:

https://tahoe-lafs.org/trac/tahoe-lafs/export/master/git/docs/specifications/file-encoding1.svg

What it does is for any uri like "/trac/tahoe-lafs/browser/git/docs/specifications/file-encoding1.svg" (matching regexp pattern in the location block) to pass uri like "/trac/tahoe-lafs/export/master/git/docs/specifications/file-encoding1.svg" to uwsgi and trac, so it will serve the raw file back to nginx.

I.e. it doesn't work around trac (which might be a bad idea, and probably not so easy to do for a bare git repo), so all the trac checks and tricks are in place, and it will only apply to "docs/" path in git.

I'm afraid I'm not aware of a way to make trac serve raw images by default and looking over git configuration section in a trac I maintain, I can't see any option to make it do that (as I think it did with darcs), but if such option exists, imho it might be a bit more consistent solution, though maybe a bit less convenient since it might be undesirable to display raw image attachments in wiki and tickets.

Does it make any sense?

Last edited at 2012-10-25T18:56:35Z by mk.fg (previous) (diff)

comment:7 Changed at 2012-10-25T19:02:56Z by mk.fg

Forgot to mention the downside, which probably should be considered.

Links like this one with images embedded have info about git revision these images came from and related links for revision/repo navigation, which will be inaccessible if such links will always be served raw.

Should still be possible to get to any revision by other means and pick the image there, but if this interface is commonly used (more so than docs with embedded images), proposed change might be undesirable.

comment:8 follow-up: Changed at 2012-10-25T20:52:36Z by davidsarah

Note that SVGs allow scripts, so serving them raw would in fact introduce an XSS vulnerability.

comment:9 in reply to: ↑ 8 ; follow-up: Changed at 2012-10-25T20:53:59Z by davidsarah

Replying to davidsarah:

Note that SVGs allow scripts, so serving them raw would in fact introduce an XSS vulnerability.

... which we may already be vulnerable to.

comment:10 in reply to: ↑ 9 ; follow-up: Changed at 2012-10-26T00:25:44Z by mk.fg

Replying to davidsarah:

Note that SVGs allow scripts, so serving them raw would in fact introduce an XSS vulnerability.

... which we may already be vulnerable to.

Whoa, you're right, didn't know about that at all.

But I still think benefits far outweight the risks in this case ;)

And it shouldn't be that dangerous coming from tahoe git. Trac shouldn't be that high-value target to worry about anything more sophisticated than wide-spectrum spam botnets, I think.

comment:11 in reply to: ↑ 10 Changed at 2012-10-26T00:30:13Z by mk.fg

Replying to mk.fg:

Replying to davidsarah:

Note that SVGs allow scripts, so serving them raw would in fact introduce an XSS vulnerability.

... which we may already be vulnerable to.

Whoa, you're right, didn't know about that at all.

But I still think benefits far outweight the risks in this case ;)

Though maybe easy win-win solution would be to just convert these images to png and commit them like that, disallowing svg in trac from then on.

comment:12 Changed at 2013-06-16T00:55:23Z by zooko

  • Description modified (diff)

#2004 was a duplicate of this.

comment:13 Changed at 2013-06-16T00:59:27Z by zooko

I still don't know what kind of behavior I want to see. Hm, here's an idea: what if https://tahoe-lafs.org nginx inspects both the URL *and the Referer*, and serves up the image plain when it is ... No, that still won't do what I want.

What I want is for the image to be embedded raw into the surrounding doc when it is being included via an <img> tag, but instead to be presented inside the trac revision control machinery when it is visited directly in the URL. I don't think there's a way to make that work.

So, pragmatically, I think one good answer is to stop using the trac source code renderer as the way to present our docs to the outside world! How about instead we write a script that runs rst2html on the docs, and we upload the resulting html files somewhere and link to that instead of to docs/about.rst?

comment:14 Changed at 2013-06-17T23:48:45Z by nejucomo

+1 for zooko's proposed solution of a separate upload, because it seems simplest to implement.

To me, it seems the "cleanest" approach is to add logic to the trac renderer which knows to point any relative links in the rst to the raw url (.../export/... rather than .../browser/...). This seems like a general feature the trac renderer would benefit from.

As for the XSS vulnerability: We already have that because of the .../export/... feature which spits out raw files. The renderer is just a different vector, which is more complicated to attack. This means we rely on all committers to omit malicious files anywhere in the repository, where "malicious file" means it would abuse a web-site viewer's account or resources.

I'm going to spend a little while tomorrow investigating trac config and features to see if it has my proposed "clean" solution, and if not, I'd advocate Zooko's proposal of a simple static directory where we upload generated results of doc rendering. This could possibly be triggered by some git hook associated with a pre-existing repository on that box.

One downside of this approach is that people may still browse and link to the trac-rendered, broken documentation.

comment:15 Changed at 2013-06-18T00:11:46Z by daira

nginx can do the redirect to export/HEAD (the "HEAD" is needed because we don't know the changeset hash) more easily than modifying trac. I tried to do that today but got the syntax wrong and had to back it out for now.

comment:16 Changed at 2013-06-18T00:13:17Z by daira

Note that /export is not vulnerable to XSS because it's setting the Content-Disposition to force a download. This would also work for img links.

comment:17 follow-up: Changed at 2013-11-30T10:10:23Z by joepie91

Perhaps this is worth looking into, at least as a duct tape solution: http://trac-hacks.org/wiki/TracImageSvgMacro

comment:18 in reply to: ↑ 17 Changed at 2013-12-01T12:26:11Z by daira

Replying to joepie91:

Perhaps this is worth looking into, at least as a duct tape solution: http://trac-hacks.org/wiki/TracImageSvgMacro

I don't see how this helps. The problem is that we want the image to display correctly both in a local rst2html build of the docs, and on the trac (and also in Sphinx docs when we have those). A Trac-specific macro won't work for the former.

comment:19 Changed at 2013-12-01T18:59:43Z by zooko

I think the solution to this is going to end up being fixing #2102 (generate docs with sphinx, upload those docs to a server that will serve them), and stop relying on trac's auto-rendering feature to render our docs.

comment:20 follow-up: Changed at 2013-12-02T02:59:26Z by daira

I'm going to try doing the nginx rewrite again.

comment:21 in reply to: ↑ 20 Changed at 2014-02-13T18:10:37Z by daira

Replying to daira:

I'm going to try doing the nginx rewrite again.

I added this redirect which does what I intended:

rewrite ^/trac/tahoe-lafs/browser/trunk/(.*)[.]svg /trac/tahoe-lafs/export/HEAD/trunk/$1.svg redirect;

but it still doesn't work, I think because we're now using Trac 1.0, which renders the rst images to HTML like this:

<object data="file-encoding1.svg" type="image/svg+xml">
file-encoding1.svg</object>

This ends up just displaying as the text "file-encoding1.svg", even though the relative URL in the data attribute is pointing to the correct raw file.

Last edited at 2014-02-13T18:11:22Z by daira (previous) (diff)

comment:22 Changed at 2014-09-07T18:18:34Z by daira

#2292 was a duplicate:

At the page ​https://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/docs/about.rst file "network-and-reliance-topology.svg" is not displaying correctly (empty space instead of a picture) OS windows 7; tested with browsers firefox 26-32, google chrome 37

comment:23 Changed at 2016-03-27T18:58:27Z by warner

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

This will be fixed by moving to readthedocs.org (#2753) instead of trying to enhance Trac.

Note: See TracTickets for help on using tickets.