tahoe-lafs/trac

Problem with linked images' display in rst docs from trac #1828

New issue

Closed

opened 2012-10-25 14:03:22 +00:00 by mk.fg · 21 comments

mk.fg commented

2012-10-25 14:03:22 +00:00

Owner

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.

For example, it can be seen on [docs/specifications/file-encoding.rst](https://tahoe-lafs.org/trac/tahoe-lafs/browser/git/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.

tahoe-lafs added the

labels 2012-10-25 14:03:22 +00:00

tahoe-lafs added this to the undecided milestone 2012-10-25 14:03:22 +00:00

daira commented

2012-10-25 15:55:42 +00:00

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 commented

2012-10-25 18:14:44 +00:00

Author

Owner

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

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 self-assigned this 2012-10-25 18:22:57 +00:00

zooko commented

2012-10-25 18:24:10 +00:00

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 commented

2012-10-25 18:52:14 +00:00

Author

Owner

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?

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 commented

2012-10-25 19:02:56 +00:00

Author

Owner

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.

Forgot to mention the downside, which probably should be considered. Links like [this one](https://tahoe-lafs.org/trac/tahoe-lafs/browser/git/docs/specifications/file-encoding1.svg) 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.

daira commented

2012-10-25 20:52:36 +00:00

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

daira commented

2012-10-25 20:53:59 +00:00

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.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1828#issuecomment-391290): > 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 commented

2012-10-26 00:25:44 +00:00

Author

Owner

Replying to [davidsarah]comment:9:

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.

Replying to [davidsarah]comment:9: > > 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 commented

2012-10-26 00:30:13 +00:00

Author

Owner

Replying to [mk.fg]comment:10:

Replying to [davidsarah]comment:9:

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.

Replying to [mk.fg]comment:10: > Replying to [davidsarah]comment:9: > > > 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 commented

2013-06-16 00:55:23 +00:00

#2004 was a duplicate of this.

zooko commented

2013-06-16 00:59:27 +00:00

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 source:docs/about.rst?

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 source:docs/about.rst?

nejucomo commented

2013-06-17 23:48:45 +00:00

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

+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 commented

2013-06-18 00:11:46 +00:00

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.

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 commented

2013-06-18 00:13:17 +00:00

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.

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 commented

2013-11-30 10:10:23 +00:00

Author

Owner

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

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

daira commented

2013-12-01 12:26:11 +00:00

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.

Replying to [joepie91](/tahoe-lafs/trac/issues/1828#issuecomment-391299): > 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 commented

2013-12-01 18:59:43 +00:00

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 commented

2013-12-02 02:59:26 +00:00

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

daira commented

2014-02-13 18:10:37 +00:00

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 [//trac/tahoe-lafs/browser/trunk/docs/specifications/file-encoding1.svg correct raw file].

Replying to [daira](/tahoe-lafs/trac/issues/1828#issuecomment-391302): > 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 [//trac/tahoe-lafs/browser/trunk/docs/specifications/file-encoding1.svg correct raw file].

daira commented

2014-09-07 18:18:34 +00:00

#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

#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

Rows
Columns