Handle URLs with a colon after host but no port#5108
Conversation
Core git copes with URLs that have a colon after the port, but no actual numeric value. eg `http://example.com:/foo.git` or `http://example.com:`. That's horrible, but RFC 3986 says: > URI producers and normalizers should omit the port component and its > ":" delimiter if port is empty or if its value would be the same as > that of the scheme's default. Which indicates that they may and therefore we must accept it. Test that we can handle URLs with a colon but no following port number.
When the end of the host is reached, and we're at the colon separating the host with the port (ie, there is no numeric port) then do not error. This is allowed by RFC 3986.
|
I suspect this will be contentious. 😀 |
f135d57 to
d8d0ac6
Compare
|
I updated this to prefer out http-parser but allow users to specify the system http-parser if they want, even sans a version check, because this bug really isn't that serious. Basically, it allows them to opt-in to very slightly broken URL parsing if they want. I updated the CMake option for this so that users with an existing option wouldn't accidentally opt-in to it, which also allowed us to align closer to the zlib option. |
pks-t
left a comment
There was a problem hiding this comment.
After reading RFC3986 several times I agree that your fix is correct. Some bikeshedding on the CMake option name, otherwise I'm fine with your changes. Thanks!
CMakeLists.txt
Outdated
| OPTION(DEBUG_POOL "Enable debug pool allocator" OFF) | ||
| OPTION(ENABLE_WERROR "Enable compilation with -Werror" OFF) | ||
| OPTION(USE_BUNDLED_ZLIB "Use the bundled version of zlib" OFF) | ||
| OPTION(USE_BUNDLED_HTTP_PARSER "Use the bundled HTTP Parser" ON) |
There was a problem hiding this comment.
Ah, didn't know we already had an option. If we're about to change, then we should use a generic USE_HTTP_PARSER=(bundled|system), if you ask me. The reasoning is that this would be much easier to extend. For example at a future point, we'd be able to add an option USE_HTTP_PARSER=/path/to/http_parser.
docs/changelog.md
Outdated
| http-parser that we have fixed in our implementation. The bundled | ||
| library is now the default, but if you wish to force the use of the | ||
| system http-parser implementation despite incompatibilities, you can | ||
| specify `-DUSE_BUNDLED_HTTP_PARSER=OFF` to CMake. |
There was a problem hiding this comment.
We should treat changes to CMake options as breaking changes and thus highlight this in a separate section
There was a problem hiding this comment.
Generally we have a section titled "Breaking API Changes" at the bottom. CMake changes don't feel like they fit there, so I created a new section "Breaking CMake configuration changes" and slotted this in there. I also moved the "breaking" stuff to the top. We should be done breaking APIs, but if not, it should be the first thing people see in the release notes, IMO.
d8d0ac6 to
8b151b0
Compare
|
On Thu, Jun 13, 2019 at 11:58:11AM -0700, Edward Thomson wrote:
ethomson commented on this pull request.
> @@ -1,3 +1,18 @@
+v0.28 + 1
+---------
+
+### Changes or improvements
+
+* The CMake option to use a system http-parser library, instead of the
+ bundled dependency, has changed. This is due to a deficiency in
+ http-parser that we have fixed in our implementation. The bundled
+ library is now the default, but if you wish to force the use of the
+ system http-parser implementation despite incompatibilities, you can
+ specify `-DUSE_BUNDLED_HTTP_PARSER=OFF` to CMake.
Generally we have a section titled "Breaking API Changes" at
the bottom. CMake changes don't feel like they fit there, so I
created a new section "Breaking CMake configuration changes"
and slotted this in there. I also moved the "breaking" stuff
to the top. We should be done breaking APIs, but if not, it
should be the first thing people see in the release notes, IMO.
Fully agreed, thanks!
|
531d9b3 to
3fb6d3d
Compare
Our bundled http-parser includes bugfixes, therefore we should prefer our http-parser until such time as we can identify that the system http-parser has these bugfixes (using a version check). Since these bugs are - at present - minor, retain the ability for users to force that they want to use the system http-parser anyway. This does change the cmake specification so that people _must_ opt-in to the new behavior knowingly.
3fb6d3d to
fb529a0
Compare
|
Upstream: nodejs/http-parser#483 |
Core git copes with URLs that have a colon after the port, but no actual numeric value. eg
http://example.com:/foo.gitorhttp://example.com:. That's horrible, but RFC 3986 says:Which indicates that they may and therefore we must accept it.
Test that we can handle URLs with a colon but no following port number, fix our http-parser implementation, and require use of our http-parser which corrects this issue.
Fixes #5100.