This issue is similar to issue2550836.
We have a detector that prevents closing an issue if it has open
dependencies:
http://hg.python.org/tracker/python-dev/file/b9748aeeecd8/detectors/statusauditor.py#l40
This is done by raising a ValueError with a message that contains an
HTML link.

AFAIU, this ValueError is caught in cgi/actions.py, and the
add_error_message() escapes the HTML included in the message:
http://hg.python.org/tracker/roundup/file/c783a6df3ffe/roundup/cgi/actions.py#l628

This could be fixed easily by adding escape=False (see attached patch).
I verified that this doesn't affect the exploit presented in
issue2550817 (unknown properties are handled elsewhere), however it
seems that the error message could arrive from several different places,
so I'm not entirely sure it's safe to add escape=False.
Can someone more familiar with this code comment?

Hi Ezio:

In message <1404748667.97.0.474031338361.issue2550847@psf.upfronthosting.co.za>
 <1404748667.97.0.474031338361.issue2550847@psf.upfronthosting.co.za>,
Ezio Melotti writes:
>AFAIU, this ValueError is caught in cgi/actions.py, and the
>add_error_message() escapes the HTML included in the message:
>  http://hg.python.org/tracker/roundup/file/c783a6df3ffe/roundup/cgi/actions.py#l628
>
>This could be fixed easily by adding escape=False (see attached patch).
>I verified that this doesn't affect the exploit presented in
>issue2550817 (unknown properties are handled elsewhere), however it
>seems that the error message could arrive from several different places,
>so I'm not entirely sure it's safe to add escape=False.

I have the same concern you do. I wonder if there is a cleaner way to
handle this.

I wonder if we can add some data to the ValueError to tell it that the
string is already cleaned and not to re-escape it.

So you would either create or santize the string and then

  raise ValueError({ 'value': my_clean_string, 'escape_string': false })

and in actions.py, it check s to see if it's a dict and if
escape_string exists and is not false, it escapes the string otherwise
it just adds it as is.

Would this be a reasonable thing to support?

(Alternatively we can create/raise a new ValueErrorNoescape exception
but that seems like a real hack).

Simply remving the escaping seems wrong as a ValueError might contain a
text which needs escaping, just imagine a detector contains the code
"x = int(foo)" and foo happens to contain "<br>":
ValueError: invalid literal for int() with base 10: '<br>'

I like the approach of providing a separate exception class, but the
name ValueErrorNoescape sounds wrong. Maybe something like
"RawErrorMessage"?

I don't know right now, but isn't the error message sent in the email
reply if one submits changes via the email interface? So different ways
of escaping (or not escaping) might be needed here. HTML links, links to
other issues etc. could be done in a similar way to
property.plain(hyperlink=1)
i.e. "issue123" gets converted to a link to the issue and
http://example.com/ will turn into a link to this URL.

Note that before the XSS fix the HTML wasn't being escaped, no one
complained it and some probably used this feature (like we did).  The
XSS fix introduced a regression, but at the same time it might have made
things safer (in case it was possible to use this path to inject
malicious code like <script>).  In other words, I think that
interpreting HTML is the desired behavior, unless the HTML might come
from untrusted sources.

Attached a new patch (untested) that adds an escape_html arg to the
"Reject" exception:
http://hg.python.org/tracker/roundup/file/c783a6df3ffe/roundup/exceptions.py#l12
This exception is already caught in same code path, and the docstring
said this is what auditors should use to stop an operation (in our case
avoid closing an issue when there are dependencies open).  This will
allow people to use `raise Reject(msg_with_html, escape_html=False)`.
I also modified all the places where Reject is caught and its message
used with add_error_message.
Is this a reasonable solution?

Some discussion from the mailinglist:

----------Cut Message----------
From: anatoly techtonik <techtonik@gmail.com>
Sent: Friday 18 July 2014, 11:45:12
Cc: "roundup-devel" <roundup-devel@lists.sourceforge.net>
Subject: Re: [Roundup-devel] Release planning May 2014

On Fri, Jul 18, 2014 at 12:00 PM, Ralf Schlatterbeck <rsc@runtux.com> 
wrote:
> - Look more closely into issue2550847: We had XSS fixes but now use
>   cases pop up where html is being escaped when it shouldn't
>   I already have an implementation committed for escaping error 
messages
>   that are generated internally maybe we can extend this.
>   If someone has input, please comment in the issue. I've not looked at
>   the patch yet. I'm also not sure if we should make unescaped output 
a
>   feature as this may reintroduce XSS issues.

All this escaping stuff is confusing. I am against allowing non-escaped 
output
in user messages, so if people need them - they should be able to 
implement
solution in template themselves.

But people do need to render links and bold test. The only solution I know 
to
make it safe is to provide a markup processor for error messages that 
will
process markup first, and escape everything else.

I have a piece of code that can be brought to make this:
https://pypi.python.org/pypi/wikify/

----------Original Message----------
From: Ralf Schlatterbeck <rsc@runtux.com>
Sent: Friday 18 July 2014, 12:03:31
To: anatoly techtonik <techtonik@gmail.com>
Cc: "roundup-devel" <roundup-devel@lists.sourceforge.net>
Subject: Re: [Roundup-devel] Release planning May 2014
[..]
Yes, good idea. I think we have optional ReStructuredText rendering,
maybe we can reuse that for error messages.

> All this escaping stuff is confusing. I am against allowing non-escaped
> output in user messages, so if people need them - they should be able
> to implement solution in template themselves.

If user wants to ability to return non-escaped output back to the client
via a detector I don't think we should be taking that option away from
them - there is a good reason why just about every
web-framework/templating language allows overriding HTML auto-escaping.

However, like those web-frameworks/templating languages the user should
have to explicitly state that they don't want the output to be escaped
and assume the responsibility to "not do unsafe things" that comes with
that ability.

Of all the options mentioned so far I think having a new Exception type
would be the cleanest implementation. Given that Reject is currently the
recommended Exception, I would suggest creating a new RejectRaw that
does not escape the output.

I am happy to put a patch together if there is some agreement around this.

patch attach implementing RejectRaw

It is a similar approach to Ezio's second patch but uses a difference
exception type instead of a new argument for the existing Reject exception.

Slightly updated version of the RejectRaw patch to include a small
amount of documentation in doc/customizing.txt

I'll hold off on committing this patch for about a week to see if anyone
has any comments. If no one has anything to add I'll look at committing
this patch next week in an attempt to see if a v1.5.1 release can happen.

I have committed a slightly modified version of my last patch (still
works essentially the same), so I'll close this issue.

If anyone is unhappy with the solution we can reopen the issue.

commit in rev494d255043c9