Problem with linked images' display in rst docs from trac

[mk.fg]

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.

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

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

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

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

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

[davidsarah]

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

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

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

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

[zooko]

#2004 was a duplicate of this.

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

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

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

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

[joepie91]

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

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

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

[daira]

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

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

[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

[warner]

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