Allow loopback redirect URIs using ports as described in RFC8252#953
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Codecov Report
@@ Coverage Diff @@## master #953 +/- ## ======================================= Coverage 96.61% 96.62% ======================================= Files 31 31 Lines 1713 1716 +3 ======================================= + Hits 1655 1658 +3 Misses 58 58
Continue to review full report at Codecov.
|
auvipy left a comment
auvipy left a comment There was a problem hiding this comment.
thanks for your PR. can you check the PR checklist and update as appripriate? changelog, entry to the author, and possibly unit test?
pauldekkers commented Apr 4, 2021
I've modified the patch slightly to only accept random redirect_uri ports when there is no explicit port configured in the Application. I've added a line in the documentation, updated the CHANGELOG and AUTHORS. |
rtpg commented Apr 5, 2021
While reading over this back and forth I took the opportunity to just add in some tests and factor out this logic to make it easier to unit test. Thanks for doing all the legwork on this first though! I think that this loopback special casing in redirect code (pretty sensitive area, securitywise) warranted a nice chunky comment, but my tests left me pretty happy that this is the "right" way of going forward. @auvipy since I made a change I feel a bit uncomfortable approving the PR, please look over what I pushed in. |
| andparsed_allowed_uri.path==parsed_uri.path | ||
| ) or ( | ||
| parsed_allowed_uri.scheme==parsed_uri.scheme | ||
| andparsed_allowed_uri.netloc==parsed_uri.netloc |
There was a problem hiding this comment.
at first it looks like both of these branches could be merged! And for scheme and path it can, but netloc is actually inclusive of the port (it's roughly login info + hostname + port) , and for loopback IPs we don't want to be requiring the port to be the same (the whole point is wanting to support ephemeral IPs)
This adds some unit tests for loopback IP code in particular, as part of reviewing the change
rtpgforce-pushed the fix/loopback-redirect-uri branch from ff7c2e2 to 659e1a9Compare
pauldekkers commented Apr 6, 2021
@rtpg Thanks for your modifications and adding those tests, I can now tick the unit-test checkbox too ;-) FWIW, I've tested your changes locally and of course it works just as well. |
pauldekkers commented Apr 12, 2021
rtpg left a comment
rtpg left a comment There was a problem hiding this comment.
alright a bit convoluted but given that @pauldekkers and I have been going back and forth on this I think we can consider this to be reviewed sufficiently
n2ygk commented Apr 12, 2021
@rtpg@pauldekkers - Hey welcome to the project. Thanks for your PR. @MattBlack85 and I are trying to be good project leads and stay on top of changes and releases and will try to slot this in to a future release after we review it. Notably @rtpg I do not see your name in AUTHORS. Please submit a PR to correct that. |
kargaj commented May 18, 2021
Hi, when you are planning the next release and adding this feature? |
5 participants
Description of the Change
Allow loopback redirect URIs with ports using http scheme and localhost addresses (127.0.0.1 or ::1),
as described in RFC8252 (section 7.3).
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS