Repository navigation
fix(oauth): encode authorization URL parameters - #1981
fallintoplace wants to merge 2 commits into
Conversation
|
Thanks for the contribution! Before we can merge this, we need @fallintoplace to sign the Salesforce Inc. Contributor License Agreement. |
|
hi @fallintoplace! thank you for taking the time to open this PR 💟 it looks great, lets get that CLA signed so we can look into merging 😸 |
|
Just signed CLA. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1981 +/- ##
==========================================
+ Coverage 84.28% 84.30% +0.01%
==========================================
Files 118 118
Lines 13679 13682 +3
==========================================
+ Hits 11530 11534 +4
+ Misses 2149 2148 -1 ☔ View full report in Codecov by Harness. |
srtaalej
left a comment
srtaalej
left a comment
There was a problem hiding this comment.
left a comment about documentation but looking great! test seem to be failing in CI, once thats addressed i can drop a ✅
| params = { | ||
| "state": state, | ||
| "client_id": self.client_id, | ||
| "scope": scopes, | ||
| "user_scope": user_scopes, | ||
| } | ||
| if self.redirect_uri is not None: | ||
| url += f"&redirect_uri={self.redirect_uri}" | ||
| params["redirect_uri"] = self.redirect_uri | ||
| if team is not None: | ||
| url += f"&team={team}" | ||
| return url | ||
| params["team"] = team | ||
| query = urlencode(params, safe=":,/") | ||
| return f"{self.authorization_url}?{query}" |
There was a problem hiding this comment.
this may change behavior for apps that were already percent-encoding these values themselves. before this change, that was the only way to pass a redirect_uri with a query string, or a state containing + or &. those apps will
now get the values encoded twice:
- redirect_uri="https://x.com/cb%3Fa%3Db" → sent as cb%253Fa%253Db. Slack decodes it only once, so it won't match the registered redirect URL and the OAuth flow fails.
- state="abc%2B" → comes back as the literal abc%2B, so the check against the stored state fails.
we should document this in the pr description!
| def test_query_parameters_round_trip(self): | ||
| redirect_uri = "https://www.example.com/callback?view=home&lang=en" | ||
| generator = AuthorizeUrlGenerator( | ||
| client_id="111.222", | ||
| redirect_uri=redirect_uri, | ||
| scopes=["chat:write", "commands"], | ||
| user_scopes=["search:read"], | ||
| ) | ||
| for state in ( | ||
| "", | ||
| "plus+value", | ||
| "space value", | ||
| "key=value&other=value", | ||
| "percent%20value", | ||
| "fragment#value", | ||
| "日本語", | ||
| ): | ||
| with self.subTest(state=state): | ||
| url = generator.generate(state=state, team="T12345") | ||
| self.assertDictEqual( | ||
| { | ||
| "state": [state], | ||
| "client_id": ["111.222"], | ||
| "scope": ["chat:write,commands"], | ||
| "user_scope": ["search:read"], | ||
| "redirect_uri": [redirect_uri], | ||
| "team": ["T12345"], | ||
| }, | ||
| parse_qs(urlsplit(url).query, keep_blank_values=True), | ||
| ) | ||
|
|
||
| def test_openid_connect_query_parameters_round_trip(self): |
There was a problem hiding this comment.
thank you for adding tests!
This branch has not been deployed
2 participants
What
Why
+in state or nonce is decoded as a space.Implementation
urlencode, keeping existing scope formatting and parameter order.