Don't assume port 80 when generating nextLink in ASP.NET Core #1559 #1560
Conversation
|
LGTM |
biaol-odata
requested review from
raheph,
mikepizzo,
xuzhg,
AlanWong-MS,
KanishManuja-MS and
biaol-odata
|
The underlying ASP.NET Core issue https://github.com/aspnet/BasicMiddleware/issues/204 is being re-triaged. |
|
Just in my opinion, even if the underlying issue is fixed, I feel like the code is more-correct after the PR, as it does not assume any specific port to use for link generation. |
|
@andrewlock Thanks for your contribution. We looked at the PR in today's team meeting, the code looks good. As general, we usually require test coverage for code. Can you please add some simple test? Thanks! |
|
I'll do my best, could take a while as I can't build the solution or run any tests ( |
|
@andrewlock I think ASP.NET Core version of WebApi requires VS 2017. Are you using VS2017? |
|
|
|
@biaol-odata , yes I am - I found the solution files eventually! It's the Given that all your readme states under "Build" is I've added some unit tests that cover general link generation (similar to the tests of the non-core ASP.NET) and some tests for this specific case. |
| public void GetNextPageLink_UsesPortWhenDefaultPortIsProvided(string requestUri, int pageSize, string nextPageUri) | ||
| { | ||
| // Arrange | ||
| // The RequestFactory removes the default port, so we explicitly add it |
There was a problem hiding this comment.
For this case, when the port specified in uri is same as the default value. request.Host is formalized, so we add it back, and the generated linked is still the one w/o port number?
There was a problem hiding this comment.
Yeah - UriBuilder formalizes the uri inside GetNextPageLink() too. Which is absolutely fine, the problem before was that in these cases, port 80 would be enforced, even if it was https!
|
Copyright added 👍 |
|
Merged. Closed. |
andrewlock
deleted the
fix-assuming-port-80-when-generating-https-links
branch
Issues
This pull request fixes issue #1559
Description
Don't explicitly set a port when generating the nextLink
If the port isn't set on
request.Host, port 80 was assumed previously. If the app is running behind a reverse proxy, and the port was not forwarded, this can result in invalid URLs - for example an HTTPS URL using port 80. Instead, we just omit the port completely if it's not explicitly set.Checklist (Uncheck if it is not completed)
Additional work necessary
Unfortunately, I couldn't build the repo after cloning (
Could not load file or assembly 'Microsoft.Build, Version=14.0.0.0), and there are no existing tests forHttpRequestExtensionsas far as I can see, so I haven't tried to add any.