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

[MCC-286290] Add # to signature escape #7

Merged
merged 5 commits into from
Mar 29, 2017

Conversation

PoloPro
Copy link
Contributor

@PoloPro PoloPro commented Mar 16, 2017

Special characters in uri com:mdsol:study_environments:6975285c-164d-4475-b848-25b0c8852510rqcwyungouA902xcd7c9o caused MAuth to reject the signed request. This PR adds the octothorpe/pound sign (#) to the gsub which already escapes the forward slash.

Specs updated to match.

This request should have been handled properly on the client's side, since # denotes a fragment. However, since the platform does not have a rigorous uri definition, this may occur again.

@HonoreDB
Copy link
Contributor

Cool, odds of this breaking an existing integration seem low, since you'd need a requestor that was previously escaping octothorpes but not escaping forward slashes. Nonzero, though, we should warn people to check their request logs for that before updating their client.

@DMcKinnon-mdsol
Copy link
Contributor

Noice! This seems like an appropriate thing to add to the changelog as well.

@jcarres-mdsol
Copy link
Contributor

@mszenher @HonoreDB FYI

@masongup-mdsol
Copy link
Member

The code for this looks fine, but do you think we could put in a comment above the change a link to a test case or something in the client application that describes why we need it and what URL is triggering it? We already have enough trouble with testing changes like this because we don't know the exact cases that caused the need for these three different checks that it's doing so we can't confirm easily that they all still work right.

@DMcKinnon-mdsol
Copy link
Contributor

ping @mdsol/team-16

@ejinotti-mdsol
Copy link
Contributor

Would this be an opportune moment to create a function like:

def euresource_escape(s)
  CGI.escape(s).gsub(/%2F|%23/, "%2F" => "/", "%23" => "#")
end

that could then be used in the spec as well??

@masongup-mdsol
Copy link
Member

Looks good to me now!

@masongup-mdsol
Copy link
Member

Merging in 2h unless objections

@masongup-mdsol masongup-mdsol merged commit 0c4d365 into master Mar 29, 2017
@masongup-mdsol masongup-mdsol deleted the feature/strip_octothorpe branch March 29, 2017 20:37
@jfeltesse-mdsol
Copy link
Contributor

To all involved in this PR, this is a friendly reminder that this gem is published on rubygems.org and as such when a new version is made someone with the required privileges should make a release and push it to rubygems.org.
I just noticed this now, @jcarres-mdsol pushed version 4.0.3 moments ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants