search

jasongoodwin / authentikat-jwt

JWT Scala Implementation - Claims based auth for Scala.
Apache License 2.0
133 stars 45 forks source link

Should escape json #27

Closed rvermeulen closed 7 years ago

rvermeulen commented 7 years ago

escape json comments

jasongoodwin commented 7 years ago

hey looking at this. thanks.

jasongoodwin commented 7 years ago

I guess that comments should not be allowed during parsing of the json (eg "user//name" should be literally parsed as "user//name".) Disabling comments in jackson doesn't seem to fix the issue so I'll have to dig around. Thanks again - I'll have something out for this shortly.

rvermeulen commented 7 years ago

Thanks for picking this up!

I quickly browsed the code and the issue is with

https://github.com/jasongoodwin/authentikat-jwt/blob/master/src/main/scala/authentikat/jwt/JwtClaimsSet.scala#L21 asJsonString method that uses your SimpleJsonSerializer.scala

The SimpleJsonSerializer doesn't escape the values.

jasongoodwin commented 7 years ago

Ah right - I forgot that was there. I removed it in the newer version. Flipping the serialization to jackson escapes the quotes but it won't parse the scenario correctly - the unapply returns None now. Getting closer though.

The claims set at this point looks like this: { "username": "x\",\"username\":\"kevinmitnick\",\"role\":\"admin\"}//", "role": "user" }

Edit: sorry it was escaping things in the display I just have to fix the parsing now as that should be parsed without any issues.

jasongoodwin commented 7 years ago

sorry - Fixed the presentation of that

rvermeulen commented 7 years ago

Is it the comment at the end of the username that causes the parsing to go wrong? Everything looks correctly escaped with exception of the comments part. I would expect something like \/\/ instead.

Could you check if unapply works without the comments part in the username?

jasongoodwin commented 7 years ago

The tokens produced look okay but the deserialization won't work with or without the "//" at the end of the string. I pushed a branch with the fix so far. I suspect that I need to change the jackson settings under json4s but doing so doesn't appear to impact the deserialization how I'm doing it. Check the branch if you want to have a look.

jasongoodwin commented 7 years ago

Released an initial fix as 0.4.4 It needs more work because // can break the deserialization. But I think it's important to release this.

jasongoodwin commented 7 years ago

Resolved. 0.4.5