Escape JSONP breaking characters #1597
Conversation
| @@ -66,6 +66,8 @@ public void serializeWithType(JsonGenerator jgen, SerializerProvider provider, T | |||
| public void serialize(JsonGenerator jgen, SerializerProvider provider) | |||
| throws IOException, JsonProcessingException | |||
| { | |||
| // Escape line-separator characters that break JSONP | |||
| jgen.setCharacterEscapes(JsonpCharacterEscapes.instance()); | |||
There was a problem hiding this comment.
This is not safe: generator may have existing escapes user has specified, and this would just replace them.
|
First of all, thank you for submitting this. As per earlier discussions it is an open issue and potential concern. But this exhibits the same problem as the PR which is not merged: overwriting of character escapes it not something that may be done at low-level code. It is something user controls. |
|
Thanks for replying so promptly. Sorry for not paying enough attention there. One possibility would be to store the current generator's escapes in a variable as the first thing in the A better option would probably be to combine any escapes set by the users with the It seems that the first option is the only feasible one. Also, I feel like overriding the escape chars temporarily to serialize What are your thoughts on this? |
|
@catanm Another possibility would be to add a new write method that applies escapes for just that call. So I guess I am ok with temporary change with reset. One more thought: if there is custom escapes settings in effect, perhaps it should not be changed at all; and only applying escapes if defaults are in use. |
…et escapes before method returns
There was a problem hiding this comment.
@cowtowncoder I made changes that only apply JSONP-breaking escapes if no custom escapes settings are in place. Escapes are reset as the last operation of the method. Any thoughts?
|
Hi @cowtowncoder - just a nudge to get your thoughts on this. Thanks! Marco |
| @@ -78,6 +88,7 @@ public void serialize(JsonGenerator jgen, SerializerProvider provider) | |||
| provider.findTypedValueSerializer(cls, true, null).serialize(_value, jgen, provider); | |||
| } | |||
| jgen.writeRaw(')'); | |||
| jgen.setCharacterEscapes(currentCharacterEscapes); | |||
There was a problem hiding this comment.
I think this should go in finally block, just in case? Probably not big difference but as a matter of principle.
|
@catanm thanks for the nudge -- I am bit overloaded, so I don't mind reminders. I think this would do. One last (?) remaining question: have I asked for CLA yet? If not, it's from https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and usually follows "print, fill & sign, scan, email to |
|
@cowtowncoder I wrapped all the I just sent an email with my CLA attached. Please let me know if you need anything else from me. One last request: at HubSpot we are on version Many thanks, Marco |
|
@catanm I think best chance for backporting would be 2.8(.9). 2.7 is technically still not-closed but release from that is uncertain and will probably take longer. 2.8.9 will be released sooner than that. |
|
@cowtowncoder sounds good to me. Thanks! Marco |
This is a follow-up to this PR: FasterXML/jackson-core#136 from @malaporte.
I ran into the same problem that malaporte had years ago, where the JSONP API response would cause an
Uncaught SyntaxError: Invalid or unexpected tokenwhen the response contains the charactersU+2028and/orU+2029. It took me quite a bit of time to find theJsonpCharacterEscapesclass that he added whose purpose is just to escape those two characters.I thought it would be nice to incorporate it in the
JSONPObjectclass, since its purpose is just to create a JSONP object.This is a small change that should make life easier for anyone who is using
JSONPObject. Please let me know if you have any questions or would like me to make changes.