Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Exclude domain from name length check #9

Merged
merged 1 commit into from
Mar 20, 2024
+33 −16
Merged

Exclude domain from name length check #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions reference.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,13 @@ import (
)

const (
// RepositoryNameTotalLengthMax is the maximum total number of characters in a repository name.
RepositoryNameTotalLengthMax = 255

// NameTotalLengthMax is the maximum total number of characters in a repository name.
NameTotalLengthMax = 255
//
// Deprecated: use [RepositoryNameTotalLengthMax] instead.
NameTotalLengthMax = RepositoryNameTotalLengthMax
)

var (
Expand All @@ -55,8 +60,8 @@ var (
// ErrNameEmpty is returned for empty, invalid repository names.
ErrNameEmpty = errors.New("repository name must have at least one component")

// ErrNameTooLong is returned when a repository name is longer than NameTotalLengthMax.
ErrNameTooLong = fmt.Errorf("repository name must not be more than %v characters", NameTotalLengthMax)
// ErrNameTooLong is returned when a repository name is longer than RepositoryNameTotalLengthMax.
ErrNameTooLong = fmt.Errorf("repository name must not be more than %v characters", RepositoryNameTotalLengthMax)

// ErrNameNotCanonical is returned when a name is not canonical.
ErrNameNotCanonical = errors.New("repository name must be canonical")
Expand Down Expand Up @@ -190,10 +195,6 @@ func Parse(s string) (Reference, error) {
return nil, ErrReferenceInvalidFormat
}

if len(matches[1]) > NameTotalLengthMax {
return nil, ErrNameTooLong
}

var repo repository

nameMatch := anchoredNameRegexp.FindStringSubmatch(matches[1])
Expand All @@ -205,6 +206,10 @@ func Parse(s string) (Reference, error) {
repo.path = matches[1]
}

if len(repo.path) > RepositoryNameTotalLengthMax {
return nil, ErrNameTooLong
}

ref := reference{
namedRepository: repo,
tag: matches[2],
Expand Down Expand Up @@ -243,14 +248,15 @@ func ParseNamed(s string) (Named, error) {
// WithName returns a named object representing the given string. If the input
// is invalid ErrReferenceInvalidFormat will be returned.
func WithName(name string) (Named, error) {
if len(name) > NameTotalLengthMax {
return nil, ErrNameTooLong
}

match := anchoredNameRegexp.FindStringSubmatch(name)
if match == nil || len(match) != 3 {
return nil, ErrReferenceInvalidFormat
}

if len(match[2]) > RepositoryNameTotalLengthMax {
return nil, ErrNameTooLong
}

return repository{
domain: match[1],
path: match[2],
Expand Down
21 changes: 16 additions & 5 deletions reference_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
_ "crypto/sha256"
_ "crypto/sha512"
"encoding/json"
"errors"
"strings"
"testing"

Expand Down Expand Up @@ -117,7 +118,7 @@ func TestReferenceParse(t *testing.T) {
tag: "Uppercase",
},
{
input: strings.Repeat("a/", 128) + "a:tag",
input: "domain/" + strings.Repeat("a", 256) + ":tag",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example may be incorrect here, because the first component is only considered to be a domain if it either contains a period (.), a colon (:) or it's literally localhost

reference/reference.go

Lines 8 to 10 in 8507c7f

// domain := host [':' port-number]
// host := domain-name | IPv4address | \[ IPv6address \] ; rfc3986 appendix-A
// domain-name := domain-component ['.' domain-component]*

If we intend to have a domain here, it should probably contain a ., and use one of the designated RFC 6761https://www.rfc-editor.org/rfc/rfc6761.html) domains for this (e.g., example.com or example.org, or <something>.test

err: ErrNameTooLong,
},
{
Expand Down Expand Up @@ -266,6 +267,12 @@ func TestReferenceParse(t *testing.T) {
input: "[fe80::1%@invalidzone]:5000/repo",
err: ErrReferenceInvalidFormat,
},
{
input: "example.com/" + strings.Repeat("a", 255) + ":tag",
domain: "example.com",
repository: "example.com/" + strings.Repeat("a", 255),
tag: "tag",
},
}
for _, tc := range tests {
tc := tc
Expand Down Expand Up @@ -337,7 +344,7 @@ func TestWithNameFailure(t *testing.T) {
}{
{
input: "",
err: ErrNameEmpty,
err: ErrReferenceInvalidFormat,
Comment on lines -340 to +347
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. why did this one change? Not sure if that's expected 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already returning ErrReferenceInvalidFormat on main, without the new changes. Should I rather make the test pass by changing the behaviour instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I got that, you mean it's currently failing on main? I just give this test a run, and it seems to pass, or is it broken in some other way?

go test -v -run TestWithNameFailure
=== RUN   TestWithNameFailure
=== PAUSE TestWithNameFailure
=== CONT  TestWithNameFailure
=== RUN   TestWithNameFailure/#00
=== PAUSE TestWithNameFailure/#00
=== RUN   TestWithNameFailure/:justtag
=== PAUSE TestWithNameFailure/:justtag
=== RUN   TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== PAUSE TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== RUN   TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== PAUSE TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== RUN   TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== PAUSE TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== RUN   TestWithNameFailure/aa/asdf$$^/aa
=== PAUSE TestWithNameFailure/aa/asdf$$^/aa
=== CONT  TestWithNameFailure/#00
=== CONT  TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== CONT  TestWithNameFailure/:justtag
=== CONT  TestWithNameFailure/aa/asdf$$^/aa
=== CONT  TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== CONT  TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
--- PASS: TestWithNameFailure (0.00s)
    --- PASS: TestWithNameFailure/#00 (0.00s)
    --- PASS: TestWithNameFailure/:justtag (0.00s)
    --- PASS: TestWithNameFailure/aa/asdf$$^/aa (0.00s)
    --- PASS: TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (0.00s)
    --- PASS: TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag (0.00s)
    --- PASS: TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (0.00s)
PASS
ok  	github.com/distribution/reference	0.689s

Copy link
Contributor Author

@ozairasim ozairasim Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion about making the test pass. The test would pass, but it would return ErrReferenceInvalidFormat instead of ErrNameEmpty which is the expectation. The test will not fail as we are only asserting on main that there is an error and not asserting which error it is.

If you change the assertion from:

if err == nil {
    t.Errorf("no error parsing name. expected: %s", tc.err)
}

to:

if !errors.Is(err, tc.err) {
    t.Errorf("unexpected error parsing name. expected: %s, got: %s", tc.err, err)
}

to check the actual error that is returned is equal to the expectation in the test, the test will fail. So the test is passing because we are not asserting correctly what is expected in the test input

},
{
input: ":justtag",
Expand All @@ -352,7 +359,11 @@ func TestWithNameFailure(t *testing.T) {
err: ErrReferenceInvalidFormat,
},
{
input: strings.Repeat("a/", 128) + "a:tag",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if there's still constraints (or if there should be) on maximum length of individual path components ("namespace", "repository-name"). At a quick glance, HTTP itself doesn't seem to define constraints; https://www.rfc-editor.org/rfc/rfc3986#section-3.3. Maybe it's fine to have the "total" alone. (it might be fun though to have a "valid" repository, but not able to use it because there's no space left for the total length 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be fun though to have a "valid" repository, but not able to use it because there's no space left for the total length

Can you please provide an example for this? I didn't completely understand it 😞

input: "example.com/repo:tag",
err: ErrReferenceInvalidFormat,
},
{
input: "example.com/" + strings.Repeat("a", 256),
err: ErrNameTooLong,
},
{
Expand All @@ -365,8 +376,8 @@ func TestWithNameFailure(t *testing.T) {
t.Run(tc.input, func(t *testing.T) {
t.Parallel()
_, err := WithName(tc.input)
if err == nil {
t.Errorf("no error parsing name. expected: %s", tc.err)
if !errors.Is(err, tc.err) {
t.Errorf("unexpected error parsing name. expected: %s, got: %s", tc.err, err)
}
})
}
Expand Down
Loading