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

WAUrl>>#initializeFromString: error when query parameters include scheme #1216

Closed
pdebruic opened this issue Jul 14, 2020 · 8 comments
Closed

Comments

@pdebruic
Copy link
Contributor

When using Google OAuth2 they make you set absolute URLs as query parameters.
e.g.

https://www.example.com/gmailOauth2Callback?state=743bXEqXLe5PucpZLW92&code=4/1wH1DIitgV-Ko-AezApV730SJ-Sx5M64ww5ttygg5QT5eJ3GGG3KB8yJgiH-V02XJx84_DNdpGfPpZKz0XzunNg&scope=https://www.googleapis.com/auth/gmail.compose%20https://www.googleapis.com/auth/gmail.metadata

GemStone's fastcgi uses just the $request_uri part:

/gmailOauth2Callback?state=743bXEqXLe5PucpZLW92&code=4/1wH1DIitgV-Ko-AezApV730SJ-Sx5M64ww5ttygg5QT5eJ3GGG3KB8yJgiH-V02XJx84_DNdpGfPpZKz0XzunNg&scope=https://www.googleapis.com/auth/gmail.compose%20https://www.googleapis.com/auth/gmail.metadata

of the initial URL. WAUrl>>#initializeFromString: parses the scheme part of those query parameters as the initial scheme for the whole URL and then signals a syntax error.

I'll submit a PR for a fix for this issue. But maybe it should be part of a fix for #551

@marschall
Copy link
Contributor

I think erroring is correct. I believe #initializeFromString: takes a percent encoded URL. If we check RFC-3986 3.4. Query

query         = *( pchar / "/" / "?" )
pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
pct-encoded   = "%" HEXDIG HEXDIG
sub-delims    = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="

So I don't think ? should be allowed in the query.

@pdebruic
Copy link
Contributor Author

pdebruic commented Jul 16, 2020

Maybe I misunderstand. I think the error I hit is that there isn't a scheme at the start the string being parsed in #initializeFromString: and so the scheme in the query part is detected as the scheme for the entire URL which messes up the indexes used to find the path and query etc...

FastCGI on GemStone looks at the $request_uri. Nginx defaults to passing the "URL without the scheme/domain/auth/port/" parameters as the $request_uri. I could change what GemStone is using to be the "full URL"

I agree a ? should be % encoded if included in the query. right? or not at all I guess.

@pdebruic
Copy link
Contributor Author

The first URL in the first message on this issue is the entire URL, not the query parameter of another URL. Sorry that was unclear.

@pdebruic
Copy link
Contributor Author

Also is there a reason we don't process the URL as a stream? I might mess around with that.

@marschall
Copy link
Contributor

I see, I misunderstood, we somehow need to know whether the URL starts with a scheme or not.

@jbrichau
Copy link
Member

@pdebruic I think this is now fixed in #1377 which was merged into release 3.5.4.
Can you give it a try?

@jbrichau
Copy link
Member

I will close the issue as I tried the example you provided, but feel free to reopen when needed!

@Rinzwind
Copy link
Member

Regarding:

So I don't think ? should be allowed in the query.

It might be worth knowing: the PetitParser grammar for the ‘URI’ rule from RFC 3986 that I gave in a comment on Zinc issue #100 accepts both http://host/?%3F and http://host/??. Per section 2.2 in the RFC, those two URIs are not equivalent as ? is a reserved character and “URIs that differ in the replacement of a reserved character with its corresponding percent-encoded octet are not equivalent”, there’s some further explanation in Zinc issue #97.

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

No branches or pull requests

4 participants