Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VCardRawWriter: Conditionally allow invalid groups. #3

Closed
wants to merge 1 commit into from

Conversation

AltSysrq
Copy link

@AltSysrq AltSysrq commented Mar 4, 2016

Something (possibly Busy Contacts) added the following properties to an
iCloud vcard, which iCloud happily accepts, and EZ-VCard actually
parses:

COM \NWWW.COLPATRIA.COM <HTTP://www.colpatria.com>\n
COM\NWWW.COLPATRIA.COM <HTTP://www.colpatria.com>\n\n\n\n

Since it would be kind of awkward for us to manually go through and
validate groups, instead just silently discard groups which aren't
valid. This is kind of unfortunate, but I wasn't sure how better to
avoid this completely failing.

@blendmaster @skazhy Open for suggestions

@blendmaster
Copy link

So ezvcard will read these groups in, but refuse to write them back out? I put in a6e1f87 already, maybe it just needs extending to the write-side.

@AltSysrq
Copy link
Author

AltSysrq commented Mar 4, 2016

I put in a6e1f87 already, maybe it just needs extending to the write-side.

I think it needs extension to groups handling; I'll do that then.

@blendmaster
Copy link

Oh, upstream has dealt with this too https://github.com/mangstadt/ez-vcard/wiki/Changelog

Relaxed the logic that checks property names for validity before writing them to an output stream. Before, it would throw an exception if a property name contained any characters not allowed by the specifications. Now, it only throws an exception if the property name contains a character that breaks the syntax (such as newline sequences and colon characters). Spec-compliant character checks have been moved to the validation framework (see VCard.validate()).

@AltSysrq
Copy link
Author

AltSysrq commented Mar 4, 2016

Now, it only throws an exception if the property name contains a character that breaks the syntax (such as newline sequences and colon characters)

This example has a newline character.

@AltSysrq
Copy link
Author

AltSysrq commented Mar 4, 2016

Actually, if the writer doesn't violate the standard and escape newlines in group names, we probably need to stick with throwing them away rather than trying to write them back out. I feel like we're not losing much since clearly something's gone horribly wrong by that point anyway.

@blendmaster
Copy link

They also pulled in #1 and #2, which I actually think covers everything I patched in this repo. You could try vanilla upstream.

This example has a newline character.

Really? can you paste the vcard somewhere (this is a public repo)?

@AltSysrq
Copy link
Author

AltSysrq commented Mar 4, 2016

can you paste the vcard somewhere

The relevant portion is in the description. (The vcard parser does turn that \N into a newline, which is what it ends up screaming about during reserialisation.)

@blendmaster
Copy link

Wow, icloud really does accept that wholesale. I suppose we could match their behavior and not unescape the newlines in group names, but I guess I'd be fine with blindly dropping them as well.

the spec only seems to mention \n,\N in one place:

The method for specifying CRLF character sequences in text
type values has been changed. The CRLF character sequence in
a text type value is specified with the backslash character
sequence "\n" or "\N".

https://www.ietf.org/rfc/rfc2426.txt but it does seem like icloud is in the wrong here.

Something (possibly Busy Contacts) added the following properties to an
iCloud vcard, which iCloud happily accepts, and EZ-VCard actually
parses:
```
COM \NWWW.COLPATRIA.COM <HTTP://www.colpatria.com>\n
COM\NWWW.COLPATRIA.COM <HTTP://www.colpatria.com>\n\n\n\n
```

This extends the `propertyValidationEnabled` option to also disable
validation of group names, on the assumption that if something got into
iCloud or some other service, that service also will accept the same
thing back. (And if we were able to parse it, it obviously doesn't break
the syntax outright.)
@AltSysrq
Copy link
Author

AltSysrq commented Mar 8, 2016

@blendmaster Revised to do this the same way as invalid properties.

@blendmaster
Copy link

lgtm.

@AltSysrq AltSysrq changed the title VCardRawWriter: Discard invalid groups. VCardRawWriter: Conditionally allow invalid groups. Mar 9, 2016
@AltSysrq
Copy link
Author

AltSysrq commented Mar 9, 2016

Actually, this won't really work as-is because it doesn't re-escape the groups when writing. It would probably be a better solution to not interpret the escapes on read instead.

AltSysrq added a commit that referenced this pull request Mar 9, 2016
Supercedes #3, since this
actually can work.
@AltSysrq
Copy link
Author

AltSysrq commented Mar 9, 2016

Closing in favour of #4

@AltSysrq AltSysrq closed this Mar 9, 2016
AltSysrq added a commit that referenced this pull request Mar 9, 2016
Supercedes #3, since this
actually can work.
AltSysrq added a commit that referenced this pull request Mar 9, 2016
Supercedes #3, since this
actually can work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants