fix: Avoid unicode-escaping ampersand in recovery URL query string #1212
Conversation
fenech
changed the title
| @@ -191,7 +191,8 @@ func (s *Strategy) createRecoveryLink(w http.ResponseWriter, r *http.Request, _ | |||
| url.Values{ | |||
| "token": {token.Token}, | |||
| "flow": {req.ID.String()}, | |||
| }).String()}) | |||
| }).String()}, | |||
| herodot.UnescapedHTML) | |||
There was a problem hiding this comment.
Could you please add a test for this?
There was a problem hiding this comment.
As I mentioned in the PR description, I wasn't sure about the best way to do it, do you have any suggestions?
Since "\u0026" == "&" in Go, I'm not sure how we could test it from within checkLink, defined here https:/ory/kratos/blob/master/selfservice/strategy/link/strategy_recovery_test.go#L56.
I guess that the way to do it would involve looking at the raw body of the response, before it is decoded into *admin.CreateRecoveryLinkOK, but I'm not sure how to go about doing that in the current tests.
There was a problem hiding this comment.
Hm, it also appears that current tests fail due to this? Basically \u0026 is & in UTF-8 speak so all clients that are capable of decoding UTF8 should know what to do here. What library where you facing issues with here?
There was a problem hiding this comment.
The original issue was observed using curl on the command line: the link returned was not clickable.
There was a problem hiding this comment.
I see, could you try curl ... | jq? I guess curl is returning the raw payload
There was a problem hiding this comment.
Yeah, that was what I did to get around it in my script 😄
There was a problem hiding this comment.
Ok, unless we find a good solution on how to fix the tests or figure out why they fail in the first place we might have to stick with the UTF8 encoded variant, although I agree that it is a bit counterintuitive
|
@aeneasr the tests are passing now - the issue was that Encode adds a newline to the end of the JSON output. |
|
Thank you! |
Related issue
#1188
@aeneasr
Proposed changes
Specify option to output JSON without escaping HTML entities, to keep the
&in the query string intact.Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
Tested manually using cURL, but I'm not sure about the best way to add automatic tests for this.
I ran
go get github.com/ory/[email protected]go mod tidy, which led to the changes ingo.modandgo.sum.