-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a port number in this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a467fd7
to
9dc5694
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @relu, please rebase.
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 <[email protected]>
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