Merge-o-Matic

127. By Adrien Cunin on 2009-01-06: {merge,manual}-status.py: added missing : after if
128. By Adrien Cunin on 2009-01-06: {merge,manual}-status.py: moved get_comments call outside do_table, and put it somewhere where it's called only once
129. By Adrien Cunin on 2009-01-07: {merge,manual}-status.py: make sure the comment exists before trying to generate a bug link
(that part of the code could probably be written better and in a more readable way)
130. By Adrien Cunin on 2009-01-07: {merge,manual}-status.py: added title to the text box, so that one can easily read long comments
131. By Adrien Cunin on 2009-01-07: addcomment.py: truncate the comment at 100 characters before saving it
132. By Adrien Cunin on 2009-01-07: {merge,manual}-status.py: replace " by " in every comment to avoid some troubles :)

Revision history for this message

Kees Cook (kees) wrote on 2009-01-07:

I'm not too familiar with python HTTP handlers, but it seems that there are race conditions updating the central comment file.

The comment content needs significantly more filtering both on input and output. e.g. the form content to make sure there are no "\n" in the comment string itself. On output, it should be fully HTML escaped, not just for double quotes. See "from cgi import escape".

review: Needs Fixing

Revision history for this message

Adrien Cunin (adri2000) wrote on 2009-01-08:

Could you elaborate a bit on the race conditions?

For \n, well, you're right, that reminds me of multi-lines spam that sometimes affects DaD. I did some tests and managed to reproduce that in w3m, but I used ^M (Ctrl+V and Ctrl+M) which actually is \r not \n. The result was just... DoS: "Internal Server Error"; the code doesn't like when a line of the comments file doesn't have ':'. Anyway I will fix that, can you just confirm that I should strip out \r (and not \n) ?

And finally, yes, I've heard of cgi.escape but thought is was unnecessary to use it. The comments are always displayed in a text box, something like <input type="text" value="$the_comment_here" />, and never directly on the page where code could be interpreted. In this context, I can't think of, apart from the double quotes character and newline, a character that could break something or make it possible to get some code interpreted. If I'm missing something, please tell me! :)

Revision history for this message

Kees Cook (kees) wrote on 2009-01-08:

Well, for the race conditions, I mean, what happens if two updates attempt
to land at the same time? There's no file locking to prevent it, from what
I can see. One might end up with interleaved data, lost comments, etc.

For the \n-removal, I would drop all characters less than 0x20. (And for
evilness injection, I recommend the firefox plugin "TamperData".)

Escaping is needed, if not for safety, just for avoiding bugs (e.g. "&"
would attempt to be turned into an entity). Best to use an existing
function to handle it. I would note that you need to use cgi.escape(s,
quote=True) in this case, to catch the quotes. (But you're right, the
quote-escaping was sufficient -- I think I was pondering a textbox, not an
inputbox.) :)

lp://staging/~adri2000/merge-o-matic/dev updated on 2009-01-30

133. By Adrien Cunin on 2009-01-25: do all the escaping when adding the comment and do it better (html and newline)
134. By Adrien Cunin on 2009-01-25: addcomment.py: put an exclusive lock on the .comments file before writing to it
135. By Adrien Cunin on 2009-01-30: addcomment.py: explicitely import cgi.escape and mention the name of the parameter quote when calling escape()