Fix scheme validation check when using host:port #222
Conversation
| @@ -311,5 +312,35 @@ var _ = Describe("ImageRepository controller", func() { | |||
| }, timeout, interval).Should(BeTrue()) | |||
| Expect(ready.Message).To(ContainSubstring("should not start with URL scheme")) | |||
| }) | |||
| It("does not fail if using a hostname with a port number", func() { | |||
| imgRepo := loadImages(registryServer, "test-fetch", []string{"1.0.0"}) | |||
| imgRepo = strings.ReplaceAll(imgRepo, "127.0.0.1", "localhost") | |||
There was a problem hiding this comment.
I didn't realize the behavior would change when dealing with hostnames vs an IP address. Here's a test to cover that situation.
There was a problem hiding this comment.
Is there a port number in this test?
There was a problem hiding this comment.
I don't think it does change -- https://play.golang.com/p/0Vw2HyPHb4P. Now I don't know how it worked before :-S
There was a problem hiding this comment.
@stefanprodan there is although not explicitly, the registryServer listens on a random port which ends up being included in the URI (e.g. 127.0.0.1:33571/test-fetch).
@squaremo it does change, try with localhost:5000/bar which will succeed parsing but will fail to identify the (missing) scheme. The IP address format will just fail to parse. It worked before because we didn't look at the error but rather just checked if the parsing succeeded and if there was a Scheme attribute set (which was not correctly set in the situation when a port would be part of the URI when using a proper hostname).
There was a problem hiding this comment.
I see, localhost:5000/bar parses as a URL with scheme of localhost and a path, so falls foul of that condition; but 127.0.0.1:5000 doesn't parse as a URL, and skips ahead.
relu
force-pushed
the
fix-scheme-validation
branch
from
a467fd7
to
9dc5694
Compare
There was a problem hiding this comment.
From eyeballing this, I think it catches the special case (a user supplies an image repo string starting with "http://" or "https://"), and won't accidentally catch e.g., localhost:5000/bar as reported in #220.
It might be worth factoring the image repo validation into its own func and testing / fuzzing that directly. But, up to you :-) Thanks @relu 🍏
|
Thaks, @squaremo! I'm actually looking into some improvements on this side and will take your suggestion into consideration. |
|
I'm for including this in the next release. @relu could you create an issue for fuzzing so we can merge this PR as it is? Or you prefer to add fuzzing this week? |
|
@stefanprodan I'd like to add fuzzing and might be able to get to it tomorrow. |
The validation check for the presence of a scheme did not take into account situations when the registry hostname would be accompanied by a port number. In this situation the hostname would be erroneously parsed as a scheme and validation would fail. Fixes #220 Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>
relu
force-pushed
the
fix-scheme-validation
branch
from
9dc5694
to
4dd87b8
Compare
The validation check for the presence of a scheme did not take into
account situations when the registry hostname would be accompanied by a
port number. In this situation, the hostname would be erroneously parsed
as a scheme and validation would fail.
Fixes #220