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

Escaping backslash incorrectly when backslash is used to escape forward slash as in ASP.NET JSON date format #21

Closed
mattspradley opened this issue Apr 30, 2011 · 10 comments

Comments

@mattspradley
Copy link

When JSONData is called on a NSDictionary that has a key value pair that has a string encoded with ASP.NET Date format (http://weblogs.asp.net/bleroy/archive/2008/01/18/dates-and-json.aspx) the backslash in the date format is incorrectly escaped with another backslash.

For example:

"/Date(1303502009425)/"

is encoded as:

"/Date(1303502009425)/"

This should not be the case because the JSON spec indicates that the forward slash can optionally be escaped.

@johnezang
Copy link
Owner

I can't say I'm familiar with Microsofts "solution" to encoding dates, but their "solution" has a few potential problems. Well, at least for anything that wants to interoperate with Microsofts "solution". Technically, this is not a bug in JSONKit, it's really more of a design flaw on Microsofts part.

When you ask JSONKit to encode /Date(1303502009425)/, the resulting JSON string will be /Date(1303502009425)/. There is absolutely no requirement that a RFC 4627 compliant JSON implementation MUST (RFC 2119) backslash escape the / character- it's the only character that can be optionally backslash escaped. JSONKit does not backslash escape / characters, it passes them straight through, which is permitted by RFC 4627.

The \ character, on the other hand, must be backslash escaped. A NSString of \ is encoded as the JSON string \\. If you try to encode a NSString of \/Date(1303502009425)\/, JSONKit converts it to the JSON string of \\/Date(1303502009425)\\/. And when you ask JSONKit to convert some JSON that contains a string of \\/Date(1303502009425)\\/, it becomes a NSString of \/Date(1303502009425)\/- The string round trips exactly as it should.

Now consider the NSString of /Date(1303502009425)/- it encodes as a JSON string of /Date(1303502009425)/, and when that JSON string is parsed it becomes a NSString of /Date(1303502009425)/- again, the string round trips perfectly.

Then consider Microsofts "solution"- A JSON string of \/Date(1303502009425)\/ will be converted in to a NSString of /Date(1303502009425)/, but when you convert that NSString back in to JSON, JSONKit encodes it as /Date(1303502009425)/. According to RFC 4627, the JSON strings \/Date(1303502009425)\/ and /Date(1303502009425)/ are "identical" and "compare equal".

If Microsoft absolutely, unconditionally requires the JSON string to be \/Date(1303502009425)\/, then they've royally screwed up- they have turned the RFC 4627 optional backslash escaping of the / character in to a MUST.

Not really sure how to deal with this issue. All of the solutions I can think of suck, and I have to admit that it really rubs me the wrong way to hack up the string encoding functions with what amounts to a one-off hack just to work around Microsofts mistake- a hack that would add at least another branch and compare required for every single character for every single string that needs to be encoded. sigh

@mattspradley
Copy link
Author

It's definitely a pain and adds overhead to JSONKit which is highly optimized. However, Microsoft does use this loophole in their new WCF REST APIs and other places.

What about adding a parsing option?

Bavarious has demonstrated a patch to the code (without a parse option) here: http://stackoverflow.com/questions/5844525/how-to-prevent-jsonkit-from-escaping-backslash-from-asp-net-json-date-format

@ghost
Copy link

ghost commented Apr 30, 2011

I’d answered that question on Stack Overflow before having read John’s comment on this bug report. I agree with John’s arguments. That said, would it be possible to implement this feature as preprocessor dependent code? This would leave JSONKit as is unless a preprocessor macro is defined.

@johnezang
Copy link
Owner

The solution in stackoverflow answer doesn't work. Well, it works in that it handles the immediate problem, but it just shifts the problem somewhere else and actually causes more problems than it fixes.

The gist of the problem is this: Suppose you have a NSString with \\/. The proposed solution would produce the following JSON string: \\\/ (at least, as far as I can tell with a once over reading of the answer + code, which could be very wrong). That JSON string, when converted back to a NSString becomes \/- it looses one of the backslashes in the process.

So far the "best" solution I can think of is a flag that optionally turns on backslash escaping of / characters. You would then pass JSONKit a NSString with /Date(...)/, which, when the forward slash option is enabled, would become a JSON string of \/Date(...)\/.

If you need a solution right now, you can do the following: Use a NSString of /Date(...)/, that is to say, do not pre-backslash escape the forward slashes. Use JSONKit to convert it to JSON in a NSString, not NSData. Then use one of the NSString replacement methods to do a search and replace of / with \/, probably something along the lines of:

NSString *escapedJSONString = [convertedToJSONString stringByReplacingOccurrencesOfString:@"/" withString:@"\\/"];

Crude, not terribly efficient, but I think that might be your best bet for now.

I'll need to think about how to deal with the issue in JSONKit.

@mattspradley
Copy link
Author

That's kind of how the post here: http://weblogs.asp.net/bleroy/archive/2008/01/18/dates-and-json.aspx says to deal with it.

I can understand if you don't want to put this case in JSONKit.

I just wanted to make everyone aware of the issue. I'm looking forward to the final decision.

@johnezang
Copy link
Owner

I will probably add an encoding option that allows you to optionally escape forward slashes. Even though I don't much care for the reason why it has to be done, people have to use the library in the real world, and realistically, this can't be avoided. And since you pretty much have to bite the bullet, you're stuck with either putting it in to the guts of JSONKit or using something like the stringByReplacingOccurrencesOfString:withString: work around... and that solution is just comically inefficient in terms of speed and memory, which is sort of counter to the entire reason why JSONKit exists... so, pragmatism wins out.

... though don't expect me to "get right on it". :) Let me know if the proposed stringByReplacingOccurrencesOfString:withString: work around works- it shouldn't be too bad for "small" amounts of JSON, but using that solution on >40-50 Kbytes of JSON starts to cross over in to "comically inefficient" land. I'd suggest leaving a comment tag like // XXX Fix me when forward slash escaping is in JSONKit where ever you do this so you can come back to it (and find it) later and remove the work around and use the more efficient JSONKit built-in solution.

@mattspradley
Copy link
Author

Thanks for being so pragmatic in your approach. I have implemented a workaround but look forward to the option being implemented in the future.

@johnezang
Copy link
Owner

I've committed a change that should "fix" this. When serializing a Cocoa collection with one of JSONKits serialization methods that accepts options, you can pass it JKSerializeOptionEscapeForwardSlashes, which should backslash escape all the forward slashes in the string.

aussiegeek added a commit to playup/JSONKit that referenced this issue May 4, 2011
* 'master' of github.com:playup/JSONKit:
  Adds a serializing option to backslash escape forward slashes.  This is for issue johnezang#21.
  Change a NSException from exceptionWithName: to raise:.  Closes johnezang#20.
  Fixes a bug when removing items from a JKDictionary.  Since JKDictionary is implemented using a hash table that uses linear probing, the removal function needs to "re-add" items that follow the removed item so that linear probe hash collisions are not "lost".  Closes johnezang#17
@johnezang
Copy link
Owner

@mattspradley, did the committed change resolve this problem to your satisfaction? If so, go ahead and close this issue. Otherwise, let me know what else I can do to help.

@mattspradley
Copy link
Author

That fixed it.

@ghost ghost assigned johnezang May 21, 2011
jasongregori pushed a commit to jasongregori/JSONKit that referenced this issue Sep 23, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants