Proposal: Omit default port (80 and 443) by default

[andreynering]

I recently dealt with a server that misbehaves when the :443 was added to the Host header, which required me a lot of investigation to understand what was happening. Setting omit_default_port to true was the solution.

Omitting when default is the most common behavior. Browsers omit them, curl omit as well, etc.

The RFC does not say it's prohibited to add the port, but say it's not required, suggesting that omitting is preferred.

[geemus]

Hey, sorry to hear you ran into this.

My inclination is also to set it to true, but I have worried this might inadvertently be a breaking change for some existing use cases, so I've been holding off for a major release (which I seem never quite to get around to). I thought there was an issue related to this pinned, but can't find it now. It's a good reminder to circle back to that plan sooner rather than later.

I don't think I would necessarily read the spec say that it is not required as being the same as saying that leaving it out is preferred. I would tend to argue that servers that don't allow it to be optional are the ones breaking the spec, rather than a client including it when it is not required. But ultimately it doesn't really matter who is correct, as it causes real problems for people and the client is what we have control to change.

Anyway, I'm glad you found your fix for now, and I certainly plan to make this change, but I still would feel more comfortable doing it as part of a major release (in case there are servers that would behave differently were you to omit it, which seems at least plausible given what we see when you include it). Does that make sense?

[andreynering]

Hi @geemus,

You're right that these servers are certainly breaking the specification.

I personally don't believe it would be a breaking change, but I certainly respect your opinion as the author. Waiting for the next major release sounds good to me, too.

Regarding the existing issues you mentioned, I only noticed them after I opened this one, but there's #357 and #448.

[geemus]

Thanks for understanding. As mentioned in one of those issues, I believe it would at least break the implementation in some of the fog-aws stuff (as some of the signing depends on the host being a particular way), but I suspect some ssl setups might also be overly strict and cause problems (this is likely very uncommon, but I wouldn't rule it out). In any event, I feel I should be cautious here, though I wish it weren't true as this is a recurring pain point for people (myself included).

Thanks for digging up those issues again, I must have not dug deep enough (I think I must not have looked at closed issues)..

I don't think I have bandwidth right at the moment to work on the major release stuff, but I will pin this issue to make sure it doesn't get closed to keep it in mind for now.

[andreynering]

@geemus Thanks for this great library! We use it extensively at JobScore in a lot of different integrations in our app.

[geemus]

@andreynering I'm very glad to hear it! Sorry for this bump along the road and hopefully I'll have a chance to iron it out in the not too distant future.

[ssa-wtag]

If anyone wonders, this PR is also the fix for this issue #786