[RFC] Allow `=` in parameter values. #207

geier · 2016-12-30T00:18:27Z

Some parameter values (e.g., BASE64 encoded binary data often ends with one or two equal signs) may contain an equal sign (=). The current implementation splits key-value pairs at all equal signs, which leads to errors. Especially icalendar files generated by Apple's software often feature BASE64 encoded binary data in parameter values.

This patch introduces a new parameter maxsplit to icalendar.parser.q_split() which works similar as python's string.split(sep, maxsplit) which we then use to split parameter key-value pairs only at the first equal sign.

This patch fixes #197.

digsim · 2016-12-30T08:24:53Z

Regarding the test introduced in #204 one could argue to remove this test now. Its only purpose was to pinpoint a problem, which is getting fixed by this commit making this test obsolete. However, I would rather argue that the test should stay. I would therefore suggest to change line 20 into
self.assertEqual(len(event.errors), 0, 'Got too many errors'
and remove lines 21 and 22.

This second approach would then verify that:

icalendar is able to parse a file containing binary information (already possible without this patch) and
icalendar can do this without producing any errors (new with this patch).

geier · 2016-12-30T13:21:58Z

I have modified the old test and added a new one that tests the fix introduced in #204.

geier · 2017-01-02T13:47:29Z

Looks like I forgot to push those changes (done now). The failing test is pypy3, which we currently do not support, I have opened #206 to deal with that.

untitaker

q_split logic looks good to me

untitaker · 2017-01-04T16:56:12Z

src/icalendar/tests/test_icalendar.py

+            for event in cal.walk('vevent'):
+                self.assertEqual(len(event.errors), 0, 'Got too many errors')
+
+        except UnicodeEncodeError as e:


Why catch the error at all?

Honestly: no idea, I've just re-used the old test, but sure, we can remove it.

geier · 2017-01-05T01:20:32Z

I removed those try/excepts.

untitaker · 2017-01-05T17:06:19Z

LGTM

…

On Wed, Jan 04, 2017 at 05:20:34PM -0800, Christian Geier wrote: I removed those try/excepts. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #207 (comment)

digsim · 2017-01-05T17:08:24Z

That should be ok. Back then, the except was used to let the test fail.

geier · 2017-01-18T12:56:43Z

I'd like to get someone else's review on this. @stlaz, @thet or @regebro, if you find the time, I'd really appreciate your input.

stlaz · 2017-01-18T22:02:10Z

Putting it on my TODO list

regebro · 2017-01-19T11:03:26Z

"=" is a SAFECHAR and should be allowed in unquoted values, so this is indeed a bug.
Only comment I have is that passing in maxsplit=0 doesn't return the expected result (no splits). It's a sufficiently silly case that you can choose not to support it.

This causes old PRs to add the changelog entry to already released versions. It happened to me while rebasing collective#207

untitaker · 2017-01-19T11:45:54Z

Apparently I can push commits to this PR, so I fixed this!

…

On Thu, Jan 19, 2017 at 03:03:28AM -0800, Lennart Regebro wrote: "=" is a SAFECHAR and should be allowed in unquoted values, so this is indeed a bug. Only comment I have is that passing in maxsplit=0 doesn't return the expected result (no splits). It's a sufficiently silly case that you can choose not to support it. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #207 (comment)

regebro · 2017-01-19T12:34:34Z

Well, maxsplit=0 should reasonably not split at all. maxsplit=0 being the same as maxsplit=1 is even more confusing than it being the same as maxsplit=-1. :-)

untitaker · 2017-01-19T12:45:16Z

maxsplit=0 returns a list with len = = 1, maxsplit=1 a list with len <= 2, just like str.split does. So 0 and 1 are indeed different cases here. Is there a particular testcase you tried that fails?

untitaker · 2017-01-19T12:48:10Z

Ah, I see it fails in cases where there's nothing between the separator.

untitaker · 2017-01-19T12:52:43Z

@regebro so that was a bug, fixed!

geier · 2017-01-19T15:15:40Z

src/icalendar/tests/test_icalendar.py

+    def test_q_split_bin(self):
+        from ..parser import q_split
+        for s in ('X-SOMETHING=ABCDE==', ',,,'):
+            for maxsplit in range(3):


I'd suggest range(-1, 3) here.

geier · 2017-01-19T17:58:36Z

Because it doesn't split if we are in between "s. Quoting Stanislav Láznička (2017-01-19 18:49:27)

…

I hope I am not missing anything but is the `q_split()` function really needed? Why not either use the Python's native `str.split()` or if we really really don't like the "." notation, map it in a function like this: ```python def q_split(st, sep=",", max_split=0): st.split(sep, max_split) ``` The `q_split()` function looks like a hack from Python-prehistoric times to me. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #207 (comment)

stlaz · 2017-01-19T19:45:26Z

@geier Thank you, I found that out right after I wrote that comment so I removed it :) Been in work for too long today.

untitaker · 2017-01-30T21:28:41Z

src/icalendar/tests/test_icalendar.py

+        Test if we support base64 encoded binary data in parameter values.
+        """
+        directory = os.path.dirname(__file__)
+        data = open(os.path.join(directory, 'x_location.ics'), 'rb').read()


Please use a context manager here to fix the resource warning.

That's the way it's done throughout the test suite. I believe it's better to be consistent than to be right (in this case).

It's no longer done this way, see #213

oh, it isn't any more... alright, will fix

untitaker · 2017-01-30T21:29:07Z

This conflicts with master, otherwise LGTM

geier · 2017-01-30T21:58:02Z

done

geier · 2017-01-30T22:09:16Z

I can force push into the branch, and your approval doesn't get auto-revoked!?

geier · 2017-02-15T13:37:41Z

anybody got any idea why python 2.6 now fails on travis with

import unittest2 as unittest
E   ImportError: No module named unittest2

?

geier · 2017-02-15T14:25:37Z

As this group maintained, I'm not going to merge this myself, but I'd be grateful if somebody else would do it or voice their concerns.

I believe the current failure with python2.6 can safely be ignored, as it fails on master, too 😬

WhyNotHugo · 2017-03-16T15:50:54Z

Any update on this? Looks ready, right?

untitaker

Yeah, though we should have multiple approvers

untitaker · 2017-03-16T16:04:59Z

@regebro could you take a quick look at this again?

Some parameter values (e.g., BASE64 encoded binary data often ends with one or two equal signs) may contain an equal sign (`=`). The current implementation splits key-value pairs at all equal signs, which leads to errors. Especially icalendar files generated by Apple's software often feature BASE64 encoded binary data in parameter values. This patch introduces a new parameter `maxsplit` to icalendar.parser.q_split() which works similar as python's string.split(sep, maxsplit) which we then use to split parameter key-value pairs only at the first equal sign. This patch fixes collective#197.

The fix for collective#197 makes the test data used for testing the error messages for broken properties (which was valid data) work with icalendar, we therefor need a new test with actually invalid data.

geier · 2017-04-18T22:20:37Z

Thanks everyone who contributed!

geier force-pushed the fix_base64_equal branch from 654e079 to 63025ee Compare December 30, 2016 00:22

geier force-pushed the fix_base64_equal branch from 63025ee to f3a3dd6 Compare January 2, 2017 13:23

untitaker reviewed Jan 4, 2017

View reviewed changes

geier force-pushed the fix_base64_equal branch 3 times, most recently from 605ec2c to 91a1648 Compare January 5, 2017 01:19

geier mentioned this pull request Jan 8, 2017

"Content line could not be parsed into parts" exceptions flood the screen pimutils/khal#541

Closed

untitaker force-pushed the fix_base64_equal branch from a4137f1 to a9e14e3 Compare January 19, 2017 11:41

untitaker added a commit to untitaker/icalendar that referenced this pull request Jan 19, 2017

Remove custom merge strategy for changelog

a01dda2

This causes old PRs to add the changelog entry to already released versions. It happened to me while rebasing collective#207

untitaker mentioned this pull request Jan 19, 2017

Remove custom merge strategy for changelog #208

Merged

untitaker force-pushed the fix_base64_equal branch from a9e14e3 to 5b875e8 Compare January 19, 2017 11:48

untitaker force-pushed the fix_base64_equal branch from 5b875e8 to 0b88c90 Compare January 19, 2017 12:52

geier commented Jan 19, 2017

View reviewed changes

untitaker reviewed Jan 30, 2017

View reviewed changes

geier force-pushed the fix_base64_equal branch from 8ebe85f to db31584 Compare January 30, 2017 21:56

untitaker approved these changes Jan 30, 2017

View reviewed changes

geier force-pushed the fix_base64_equal branch from db31584 to d3f9afe Compare January 30, 2017 22:08

geier force-pushed the fix_base64_equal branch 2 times, most recently from 3594e97 to c37b983 Compare February 15, 2017 13:31

untitaker approved these changes Mar 16, 2017

View reviewed changes

geier mentioned this pull request Mar 16, 2017

Support DQUOTE character in property parameter values #219

Open

kevin-brown approved these changes Apr 17, 2017

View reviewed changes

geier and others added 5 commits April 19, 2017 00:07

Moved test_apple_xlocation() to test_icalendar.py

124b328

New test for broken properties.

b82893f

The fix for collective#197 makes the test data used for testing the error messages for broken properties (which was valid data) work with icalendar, we therefor need a new test with actually invalid data.

Fix q_split for maxsplit=0

0f408d8

Extend tests

805f59d

geier force-pushed the fix_base64_equal branch from c37b983 to 805f59d Compare April 18, 2017 22:07

geier merged commit 8a52e56 into collective:master Apr 18, 2017

geier deleted the fix_base64_equal branch April 18, 2017 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Allow `=` in parameter values. #207

[RFC] Allow `=` in parameter values. #207

geier commented Dec 30, 2016 •

edited

Loading

digsim commented Dec 30, 2016

geier commented Dec 30, 2016

geier commented Jan 2, 2017

untitaker left a comment

untitaker Jan 4, 2017

geier Jan 5, 2017

geier commented Jan 5, 2017

untitaker commented Jan 5, 2017 via email

digsim commented Jan 5, 2017

geier commented Jan 18, 2017

stlaz commented Jan 18, 2017

regebro commented Jan 19, 2017

untitaker commented Jan 19, 2017 via email

regebro commented Jan 19, 2017

untitaker commented Jan 19, 2017 •

edited

Loading

untitaker commented Jan 19, 2017

untitaker commented Jan 19, 2017

geier Jan 19, 2017

untitaker Jan 19, 2017

geier commented Jan 19, 2017 via email

stlaz commented Jan 19, 2017

untitaker Jan 30, 2017

geier Jan 30, 2017

untitaker Jan 30, 2017

geier Jan 30, 2017

untitaker commented Jan 30, 2017

geier commented Jan 30, 2017

geier commented Jan 30, 2017

geier commented Feb 15, 2017

geier commented Feb 15, 2017

WhyNotHugo commented Mar 16, 2017

untitaker left a comment

untitaker commented Mar 16, 2017

geier commented Apr 18, 2017

[RFC] Allow = in parameter values. #207

[RFC] Allow = in parameter values. #207

Conversation

geier commented Dec 30, 2016 • edited Loading

digsim commented Dec 30, 2016

geier commented Dec 30, 2016

geier commented Jan 2, 2017

untitaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geier commented Jan 5, 2017

untitaker commented Jan 5, 2017 via email

digsim commented Jan 5, 2017

geier commented Jan 18, 2017

stlaz commented Jan 18, 2017

regebro commented Jan 19, 2017

untitaker commented Jan 19, 2017 via email

regebro commented Jan 19, 2017

untitaker commented Jan 19, 2017 • edited Loading

untitaker commented Jan 19, 2017

untitaker commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geier commented Jan 19, 2017 via email

stlaz commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Jan 30, 2017

geier commented Jan 30, 2017

geier commented Jan 30, 2017

geier commented Feb 15, 2017

geier commented Feb 15, 2017

WhyNotHugo commented Mar 16, 2017

untitaker left a comment

Choose a reason for hiding this comment

untitaker commented Mar 16, 2017

geier commented Apr 18, 2017

[RFC] Allow `=` in parameter values. #207

[RFC] Allow `=` in parameter values. #207

geier commented Dec 30, 2016 •

edited

Loading

untitaker commented Jan 19, 2017 •

edited

Loading