Description
MoinMoin dies horribly (ValueError: too many values to unpack) when parsing image markup containing more than one align= value.
Steps to reproduce
- Edit a page
Add an image, and write more than one align= value by mistake
- Save or preview
- Stare at the "Unhandled Exception" message, bewildered
Example
{{a||align="top" align="left"}}This bug appears to be triggered by any combination of multiple values for this attribute, such as align="bottom" align="left" or align="right" align="right" align="right" align="right". (in fact, it doesn't even have to be a recognized attribute: {{a||a="" a=""}})
Component selection
- Wiki text parser
Details
This Wiki
Traceback from my own installation: aligntopalignleft.txt
Workaround
Don't put silly things in the image params.
Patch:
1 diff -r 354356b125b4 MoinMoin/wikiutil.py
2 --- a/MoinMoin/wikiutil.py Thu Mar 04 04:13:19 2010 +0300
3 +++ b/MoinMoin/wikiutil.py Fri Mar 05 13:35:45 2010 +0300
4 @@ -1399,14 +1399,16 @@
5 elif not quoted and char == name_value_separator:
6 if multikey or len(cur) == 1:
7 cur.append(None)
8 + noquote = False
9 else:
10 if not multikey:
11 if cur[-1] is None:
12 cur[-1] = ''
13 cur[-1] += name_value_separator
14 + noquote = True
15 else:
16 cur.append(None)
17 - noquote = False
18 + noquote = False
19 elif not quoted and not seplimit_reached and char in separators:
20 (cur, noquote, separator_count, seplimit_reached,
21 nextitemsep) = additem(result, cur, separator_count, nextitemsep)
TODO: add (failing) test first, then fix it
1 diff -r 92e6d4ce7049 MoinMoin/parser/_tests/test_text_moin_wiki.py
2 --- a/MoinMoin/parser/_tests/test_text_moin_wiki.py Tue Mar 09 18:06:25 2010 +0300
3 +++ b/MoinMoin/parser/_tests/test_text_moin_wiki.py Tue Mar 09 20:07:37 2010 +0300
4 @@ -540,12 +540,26 @@
5 class TestTransclusionMarkup(ParserTestCase):
6 """ Test wiki markup """
7
8 - text = 'AAA %s AAA'
9 + text = u'AAA %s AAA'
10 needle = re.compile(text % r'(.+)')
11 _tests = [
12 # test, expected
13 + # XXX these tests also relay on definite order of attributes generated
14 + # by html formatter, but we should only check presence and correctness
15 + # of them.
16 ('{{http://moinmo.in/wiki/common/moinmoin.png}}', '<img alt="http://moinmo.in/wiki/common/moinmoin.png" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="http://moinmo.in/wiki/common/moinmoin.png" />'),
17 ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo}}', '<img alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" />'),
18 + ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo|width=100}}',
19 + '<img alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" width="100" />'),
20 + ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo|width=100, align=top}}',
21 + '<img align="top" alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" width="100" />'),
22 + ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo|width=100, align=top, align=middle}}',
23 + '<img align="middle" alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" width="100" />'),
24 + # some tests with erroneous, but still needed to be handled correctly, params
25 + ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo|width="100" align=top}}',
26 + '<img alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" width="100 align=top" />'),
27 + ('{{http://moinmo.in/wiki/common/moinmoin.png|moin logo|align="top" align="middle"}}',
28 + '<img align="top align="middle"" alt="moin logo" class="external_image" src="http://moinmo.in/wiki/common/moinmoin.png" title="moin logo" />'),
29 ]
30
31 def testTransclusionFormating(self):
Comments
So ...
"a="b" c="d"" triggers it as well
- the unit test for it is completely wrong -- it should be a parameter parser test not a HTML test
- the input is obviously malformed, so it should be rejected by the parameter parser
- This fix makes the parameter parser accept invalid input as though it was valid, which is bound to cause (hidden) problems in the future.
- the patch doesn't even seem to fix it for me, at least not in the generic case I put above when added into the unit testing for the parameter parser
- code using the parameter parser often needs to be catching Value and Type errors anyway
Therefore, I disagree with this fix. It would be better to have the HTML formatter catch the error and display a "confused by your input" message to the user, like e.g. the macro code does when you give bogus parameters.
One thing that could be useful would be to actually detect this situation in the parameter parser and raise a proper error with a decent error message. Was looking at doing that but couldn't figure it out right away.
Also, I just noticed that the suggested unit tests above are completely bogus, they actually verify that totally invalid HTML goes into the output.
Plan
- Priority:
- Assigned to:
Status: pending http://paste.pocoo.org/show/223772/