vscode
vscode copied to clipboard
microsoft
Tunnel: Extend port mapping lookup also for querystring (take 2)
When running az login, the URL has localhost in redirect_uri query param. This should trigger automatic port mapping.
Improve localhost port mapping to cover this case as well.
This is a revised and tested version of #203908 which was reverted in #205370.
Fixes #203869.
@alexr00 Sorry, but I still can't test it locally. Remote SSH doesn't work (installed both Remote Development and Remote SSH from vsix files), and TestResolver is not really remote, and it doesn't activate port forwarding.
@alexr00 Thanks. TestResolver doesn't work out of the box with az login (it only passes the first query param to the host), but I was able to test the feature by ctrl+clicking the full link. Need a bit more tweaking.
@alexr00 this time I tested it as much as I could, and also added unit tests (notice the separate commits, if you prefer, I can push the refactoring and tests in a separate PR before merging this one).
One issue I noticed with az login is that Microsoft login verifies that redirect_uri is unchanged, so if the mapped port is not the same, it fails. It looks like the current implementations do keep the port (at least for SSH), but I'm not sure how we should handle cases that don't preserve it. Suggestions?
One issue I noticed with az login is that Microsoft login verifies that redirect_uri is unchanged, so if the mapped port is not the same, it fails. It looks like the current implementations do keep the port (at least for SSH), but I'm not sure how we should handle cases that don't preserve it. Suggestions?
Note that I was in contact with the maintainers of az cli on https://github.com/Azure/azure-cli/issues/26556, and they seem to be willing to help solve such issues also at their side.
But as the creation of a the temporary port is done only after the command to open the browser is called, it's probably too late for doing anything. The only way I can think of is supplying some cli command to allocate a temporary port/url and having the code of the cli tool use it.
@alexr00 , is this a common scenario of getting a different port? Is it worth working on? In any case, I think that solving the original bug is a great advance, and the unhandled scenario should not prevent merging the fix.
We are in the middle of our endgame release prep right now! I will take a look at this PR next week.
@alexr00 , is this a common scenario of getting a different port? Is it worth working on? In any case, I think that solving the original bug is a great advance, and the unhandled scenario should not prevent merging the fix.
The port being different happens when something else is already listening on that port locally. It probably isn't too common. In the case that the port changes, do you think it might be worth not updating the query and simply un-forwarding the port?
I'm really not sure if rejecting the request when the redirect URI is changed is a common scenario. If it is, then I guess your suggestion is preferred.
@alexr00 I prefer not to squash. I used separate commits for a reason. Do you support rebase in this project, or do you want me to rebase?