diff --git a/expander.go b/expander.go index c9ffed5..2922fcc 100644 --- a/expander.go +++ b/expander.go @@ -583,7 +583,7 @@ func expandPathItem(pathItem *PathItem, resolver *schemaLoader, basePath string) pathItem.Ref = Ref{} for i := range pathItem.Parameters { - if err := expandParameterOrResponse(&(pathItem.Parameters[i]), resolver, basePath); resolver.shouldStopOnError(err) { + if err := expandParameterOrResponse(&pathItem.Parameters[i], resolver, basePath); resolver.shouldStopOnError(err) { return err } } diff --git a/go.mod b/go.mod index 5610c8d..2d1b96f 100644 --- a/go.mod +++ b/go.mod @@ -2,19 +2,19 @@ module github.com/go-openapi/spec require ( github.com/go-openapi/jsonpointer v1.0.0 - github.com/go-openapi/jsonreference v1.0.0 - github.com/go-openapi/swag/conv v0.29.0 - github.com/go-openapi/swag/jsonutils v0.29.0 - github.com/go-openapi/swag/loading v0.29.0 - github.com/go-openapi/swag/stringutils v0.29.0 + github.com/go-openapi/jsonreference v1.0.1 + github.com/go-openapi/swag/conv v0.29.1 + github.com/go-openapi/swag/jsonutils v0.29.1 + github.com/go-openapi/swag/loading v0.29.1 + github.com/go-openapi/swag/stringutils v0.29.1 github.com/go-openapi/testify/enable/yaml/v2 v2.6.1 github.com/go-openapi/testify/v2 v2.6.1 ) require ( - github.com/go-openapi/swag/pools v0.29.0 // indirect - github.com/go-openapi/swag/typeutils v0.29.0 // indirect - github.com/go-openapi/swag/yamlutils v0.29.0 // indirect + github.com/go-openapi/swag/pools v0.29.1 // indirect + github.com/go-openapi/swag/typeutils v0.29.1 // indirect + github.com/go-openapi/swag/yamlutils v0.29.1 // indirect go.yaml.in/yaml/v3 v3.0.5 // indirect ) diff --git a/go.sum b/go.sum index e65c1ee..82dad4f 100644 --- a/go.sum +++ b/go.sum @@ -1,23 +1,23 @@ github.com/go-openapi/jsonpointer v1.0.0 h1:kR9tHqY0CtZaOPVFm622dPVNhrvYpwr4uCxgL3h1H8s= github.com/go-openapi/jsonpointer v1.0.0/go.mod h1:Z3rw7dWu1p9IgitXCFamSlA5lmDiklEB6vkaxcNZW5Y= -github.com/go-openapi/jsonreference v1.0.0 h1:jlmTr6torcd1YgDQvSfNmRtKzYDO4FGBkrAdlAVWnpY= -github.com/go-openapi/jsonreference v1.0.0/go.mod h1:jtwdyGbJk0Xhe5Y+rwtglQP6Sb1WZST4rT32LWB+sv0= -github.com/go-openapi/swag/conv v0.29.0 h1:4+1TogWpOIzMPzVKrvx1BfqBYlApB7D7DW3EAWpwmp4= -github.com/go-openapi/swag/conv v0.29.0/go.mod h1:ch1l7V87F6zQXuLs5s0RFvrro6aFvrVcfVXn2PTZnu8= -github.com/go-openapi/swag/jsonutils v0.29.0 h1:Xgnf9g32ycQjQUnDxkhqLraH2FhitcSE3w7ayQB3TgA= -github.com/go-openapi/swag/jsonutils v0.29.0/go.mod h1:5WYmjf6hJcBve+ArzBaUsYy4M1GXsgjIQTmwJKfZHrA= -github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.0 h1:bpSF6LFkJJVtaRtJCzbZADVPVHQYKPwPdKthOQA2/5o= -github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.0/go.mod h1:julgTUKZ9/D0j6O7GKajmRs+812FWxQg/mMpGunWSjg= -github.com/go-openapi/swag/loading v0.29.0 h1:r1lg2DQbT1VgBwgiPYXBM059RNswFI6r36CC0QCcRGw= -github.com/go-openapi/swag/loading v0.29.0/go.mod h1:l/Z4MNbom0jSqzvWJqK2VUUWEceBknGEuVbLHLq4KN0= -github.com/go-openapi/swag/pools v0.29.0 h1:uMQcoJeHJ8fWkdfEXJZMMpqk6hpfW8qTL5Q/IoRFFII= -github.com/go-openapi/swag/pools v0.29.0/go.mod h1:leDcaghjkRAhCuCRv9NfJU5f0mjoU3cT/XZObhMk3pc= -github.com/go-openapi/swag/stringutils v0.29.0 h1:/IEOuZ7PGJi6lqgH83dVt7/A9eHsDGEH1459lm+gpEo= -github.com/go-openapi/swag/stringutils v0.29.0/go.mod h1:7fSqZ+z8Qc0tOfAAK0jVa5qFGrnIlRi6n7NeGGrr1vc= -github.com/go-openapi/swag/typeutils v0.29.0 h1:HrWCYZeXVVNDo/7QQPRaYk33XeIDxksbxpalID3bWR8= -github.com/go-openapi/swag/typeutils v0.29.0/go.mod h1:hxpgDZJVBkBsi/d3MIUosafoFdE5exaQRmVp0zwu3YE= -github.com/go-openapi/swag/yamlutils v0.29.0 h1:JOKKuhMnBx4HYTM+kPEYw8S5YKKU9PnC4Mwb+c69BBA= -github.com/go-openapi/swag/yamlutils v0.29.0/go.mod h1:/+FVozjFWZzku6mRz5U/Qmq5Yk8PLFxBLLWA/jHaxYE= +github.com/go-openapi/jsonreference v1.0.1 h1:4zJ7AmYDKNmD3aSpfPnFNCFA5E80/xMHUNKgydaLh38= +github.com/go-openapi/jsonreference v1.0.1/go.mod h1:dYplQXa6p5lXprLcJ8LE2iU7vNpXsAHDQ5ZAgL+Qx3A= +github.com/go-openapi/swag/conv v0.29.1 h1:AC4Eh/5c/eUDOUCzzsRC9ghmFgOSBHeRMGIngY0ZUGA= +github.com/go-openapi/swag/conv v0.29.1/go.mod h1:S1X7/ZrBEZOC0Wc8AGxjbcGS92l3WEjA7aPtpl+RaqM= +github.com/go-openapi/swag/jsonutils v0.29.1 h1:AFCxs0eQZ24/QyfhVHM2t49rMz7Vv3XCsZQI6yrNy+c= +github.com/go-openapi/swag/jsonutils v0.29.1/go.mod h1:u3+sCfJpttDpcmS5kpm0yxL6GK0eWgODsx8Yw8fcqNM= +github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.1 h1:BiiXE31Bx9SfpsMmOQj5KYpUhTZBpLVriVhJDuLuY2o= +github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.1/go.mod h1:julgTUKZ9/D0j6O7GKajmRs+812FWxQg/mMpGunWSjg= +github.com/go-openapi/swag/loading v0.29.1 h1:FCv5fG8UhTdDJa2R7w+5O9Ekpcbw7tt0nFWvmDKGBjc= +github.com/go-openapi/swag/loading v0.29.1/go.mod h1:N0ESuem4p2oedKal8EJhciqnJ9Q9Wmt83L1CRB3Fouw= +github.com/go-openapi/swag/pools v0.29.1 h1:NRogYxdEW9SjRM4mkAOji9iefO4MRXq3p/ZJcoQbUKg= +github.com/go-openapi/swag/pools v0.29.1/go.mod h1:leDcaghjkRAhCuCRv9NfJU5f0mjoU3cT/XZObhMk3pc= +github.com/go-openapi/swag/stringutils v0.29.1 h1:1ykunK7iJQk1uOO7+oUH1ukbsK85fFCOiCFMOVSY+F0= +github.com/go-openapi/swag/stringutils v0.29.1/go.mod h1:7fSqZ+z8Qc0tOfAAK0jVa5qFGrnIlRi6n7NeGGrr1vc= +github.com/go-openapi/swag/typeutils v0.29.1 h1:Nzv9nhnlLCRBPQqfOX+7lB6Guju370or8StT+lIOf6M= +github.com/go-openapi/swag/typeutils v0.29.1/go.mod h1:hxpgDZJVBkBsi/d3MIUosafoFdE5exaQRmVp0zwu3YE= +github.com/go-openapi/swag/yamlutils v0.29.1 h1:69w3tsBajm7MR/fejLy7HD/3J68Ys1SeeZMEzZ3w2sk= +github.com/go-openapi/swag/yamlutils v0.29.1/go.mod h1:rgsp3vT/QdWzKwn43CigDwjOGIenPyTZMKnxEM8jZOA= github.com/go-openapi/testify/enable/yaml/v2 v2.6.1 h1:Jm+/ze2rMtbD98yen92AhATGLGREDYXG56Xr4gMjEtE= github.com/go-openapi/testify/enable/yaml/v2 v2.6.1/go.mod h1:YDPnwCRDu38/oJBVMBVXOUDiJ9cIeBHWvfImHaXqnv4= github.com/go-openapi/testify/v2 v2.6.1 h1:6CNJhTjMzgaeaH8WhshcsZNPIvRemiOcFpU7seO/y7Q= diff --git a/normalizer.go b/normalizer.go index 3ca3f73..ced7f99 100644 --- a/normalizer.go +++ b/normalizer.go @@ -41,10 +41,11 @@ func normalizeURI(refPath, base string) string { if refURL.Path == "." { refURL.Path = "" } + forgetSpelling(refURL) r := MustCreateRef(refURL.String()) if r.IsCanonical() { - return refURL.String() + return r.String() } baseURL, _ := parseURL(base) @@ -252,11 +253,12 @@ func normalizeBase(in string) string { if u.Path == "." { // empty after Clean() u.Path = "" } + forgetSpelling(u) if u.Scheme != "" { if path.IsAbs(u.Path) || u.Scheme != fileScheme { // this is absolute or explicitly not a local file: we're good - return u.String() + return canonicalString(u) } } @@ -269,5 +271,41 @@ func normalizeBase(in string) string { u.Scheme = fileScheme u.Path = absPath(u.Path) // platform-dependent u.RawQuery = "" // any query component is irrelevant for a base - return u.String() + return canonicalString(u) +} + +// forgetSpelling drops the escaped spelling url.Parse recorded, so that rendering the URL +// derives it from Path and Fragment again. +// +// url.URL.String returns RawPath whenever it still decodes to Path, and the normalizer rewrites +// Path - path.Clean, then a join onto the base. A RawPath describing the path as it was written +// no longer describes the path we now hold, and rendering it can produce a URI that does not +// parse: normalizeBase("%2F") used to return "file://%2F", where the escaped form lost the +// leading slash that Path carries and what is left reads as an authority. +func forgetSpelling(u *url.URL) { + u.RawPath = "" + u.RawFragment = "" +} + +// canonicalString renders a URL the way jsonreference does. +// +// The normalizer and jsonreference each canonicalize a URI, and they used to disagree: the +// normalizer kept the spelling it was handed while a Ref lower-cases the host, drops a default +// port and re-escapes path and fragment. Everything the expander stores - cache keys, the +// parentRefs chain, a $ref written back into the document - goes through both, so the two must +// agree or one document splits into two spellings of itself. +// +// A URI that jsonreference cannot parse is returned as it stands, rather than panicking through +// MustCreateRef. Such a URI is a defect in the normalizer, and the fuzz target reports it as one. +func canonicalString(u *url.URL) string { + rendered := u.String() + + r, err := NewRef(rendered) + if err != nil { + specLogger.Printf("warning: the normalizer produced an unparseable URI %q: %v", rendered, err) + + return rendered + } + + return r.String() } diff --git a/normalizer_canonical_test.go b/normalizer_canonical_test.go new file mode 100644 index 0000000..53a60ab --- /dev/null +++ b/normalizer_canonical_test.go @@ -0,0 +1,224 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package spec + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// TestNormalizer_Canonicalization pins the rules the normalizer and jsonreference both apply, +// on the edge cases where they used to part company. +// +// Two canonicalizers meet on every $ref. The normalizer builds a URI out of a $ref and a base; +// jsonreference re-renders it whenever the string becomes a Ref, which happens as soon as the +// document is unmarshalled. When the two disagreed, one document could be cached under two +// spellings of itself, and a $ref written back by denormalizeRef did not normalize to what it +// came from. normalizeURI and normalizeBase now end in canonicalString, so both sides answer +// with jsonreference's spelling. +func TestNormalizer_Canonicalization(t *testing.T) { + t.Parallel() + + const base = "https://example.com/base/spec.json" + + tests := []struct { + name string + rule string + refPath string + expected string + }{ + { + name: "host case", + rule: "the host is lower-cased, the path is not", + refPath: "https://EXAMPLE.com/OTHER.json", + expected: "https://example.com/OTHER.json", + }, + { + name: "default port", + rule: ":443 comes off an https URL", + refPath: "https://example.com:443/other.json", + expected: "https://example.com/other.json", + }, + { + name: "non-default port", + rule: "any other port stays", + refPath: "https://example.com:8443/other.json", + expected: "https://example.com:8443/other.json", + }, + { + name: "IPv6 literal", + rule: "a bracketed host is lower-cased and loses its default port", + refPath: "https://[2001:DB8::1]:443/other.json", + expected: "https://[2001:db8::1]/other.json", + }, + { //nolint:gosec // test URLs carrying userinfo, not credentials + name: "userinfo", + rule: "the colon in userinfo is not a port", + refPath: "https://user:pw@EXAMPLE.com:443/other.json", + expected: "https://user:pw@example.com/other.json", + }, + { + name: "degenerate authority, port kept", + rule: "url.Parse reads the host as \":a\" on port 443, and dropping the port would leave a URI that no longer parses", + refPath: "https://:a:443/other.json", + expected: "https://:a:443/other.json", + }, + { + name: "degenerate authority, port twice", + rule: "the host \"0:443\" spells a default port of its own, so removal repeats", + refPath: "https://0:443:443/other.json", + expected: "https://0/other.json", + }, + { + name: "duplicate slashes, relative", + rule: "a run of slashes collapses to one, however long", + refPath: "a//b///c////d.json", + expected: "https://example.com/base/a/b/c/d.json", + }, + { + name: "duplicate slashes, absolute", + rule: "same, on a $ref that carries its own host", + refPath: "https://example.com/a//b///c////d.json", + expected: "https://example.com/a/b/c/d.json", + }, + { + name: "unreserved character, escaped", + rule: "%41 is decoded: RFC 3986 §6.2.2.2 allows decoding an unreserved character", + refPath: "https://example.com/%41.json", + expected: "https://example.com/A.json", + }, + { + name: "reserved character, escaped", + rule: "%2F is decoded too, which changes what the URI denotes - see the note below", + refPath: "https://example.com/a%2Fb.json", + expected: "https://example.com/a/b.json", + }, + { + name: "fragment, unescaped", + rule: "a quote in a JSON pointer is escaped on the way out", + refPath: "other.json#/definitions/it's", + expected: "https://example.com/base/other.json#/definitions/it%27s", + }, + { + name: "fragment, escaped", + rule: "and the escaped spelling is left as it is, so the two agree", + refPath: "other.json#/definitions/it%27s", + expected: "https://example.com/base/other.json#/definitions/it%27s", + }, + { + name: "fragment, brackets", + rule: "same for a definition named o[k]", + refPath: "other.json#/definitions/o[k]", + expected: "https://example.com/base/other.json#/definitions/o%5Bk%5D", + }, + { + name: "fragment, space", + rule: "and for one named \"a b\"", + refPath: "other.json#/definitions/a b", + expected: "https://example.com/base/other.json#/definitions/a%20b", + }, + } + + canonicalBase := normalizeBase(base) + require.EqualT(t, base, canonicalBase, "the base of these cases is already canonical") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + normalized := normalizeURI(test.refPath, canonicalBase) + assert.EqualTf(t, test.expected, normalized, "%s: %s", test.name, test.rule) + + // the rule that matters: what the normalizer returns is what jsonreference would + // have made of it, so turning it into a Ref changes nothing + ref := MustCreateRef(normalized) + assert.EqualTf(t, normalized, ref.String(), + "%q is not a fixpoint of jsonreference's canonicalization", normalized) + + assert.EqualTf(t, normalized, normalizeURI(normalized, canonicalBase), + "normalizing %q again changed it", normalized) + }) + } +} + +// TestNormalizer_CanonicalBase pins the same rules on the base, which normalizeBase canonicalizes +// too, and covers the rendering defect that produced an unparseable base. +func TestNormalizer_CanonicalBase(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rule string + base string + expected string + }{ + { + name: "host case and default port", + rule: "a base is canonicalized like a $ref, so two spellings key one document", + base: "https://EXAMPLE.com:443/base/spec.json", + expected: "https://example.com/base/spec.json", + }, + { + name: "duplicate slashes", + rule: "path.Clean already collapsed these; the rule is pinned here too", + base: "https://example.com/base//sub///spec.json", + expected: "https://example.com/base/sub/spec.json", + }, + { + name: "fragment dropped", + rule: "a fragment in the base names nothing", + base: "https://example.com/base/spec.json#/definitions/x", + expected: "https://example.com/base/spec.json", + }, + { + name: "escaped path rendering", + rule: `"%2F" decodes to "/", and the escaped form has no leading slash to render, ` + + `so it used to come out as the authority of "file://%2F"`, + base: "%2F", + expected: "file:///", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + canonical := normalizeBase(test.base) + assert.EqualTf(t, test.expected, canonical, "%s: %s", test.name, test.rule) + + ref := MustCreateRef(canonical) + assert.EqualTf(t, canonical, ref.String(), + "%q is not a fixpoint of jsonreference's canonicalization", canonical) + + assert.EqualTf(t, canonical, normalizeBase(canonical), + "normalizing %q again changed it", canonical) + }) + } +} + +// TestNormalizer_EscapedSlashIsDecoded records a rule that is not ours to change here. +// +// jsonreference clears url.URL.RawPath, so "%2F" comes back as a separator. RFC 3986 §6.2.2.2 +// allows decoding an unreserved character and forbids decoding a reserved one, so this is a +// change of meaning rather than of spelling: the GitLab API requires the project path +// percent-encoded, and the decoded URL names a different endpoint. +// +// It is pinned rather than fixed because a $ref loses the escape anyway - the document model +// turns every $ref into a Ref, and jsonreference decodes it there. Making the normalizer keep +// the escape would only hide that. The fix belongs upstream. +func TestNormalizer_EscapedSlashIsDecoded(t *testing.T) { + t.Parallel() + + const gitlab = "https://gitlab.com/api/v4/projects/mygroup%2Fmyproject/repository/files/swagger.json/raw" + const decoded = "https://gitlab.com/api/v4/projects/mygroup/myproject/repository/files/swagger.json/raw" + + var ref Ref + require.NoError(t, ref.UnmarshalJSON([]byte(`{"$ref":"`+gitlab+`"}`))) + assert.EqualT(t, decoded, ref.String(), "the document model decodes the escape on its own") + + assert.EqualT(t, decoded, normalizeBase(gitlab)) + assert.EqualT(t, decoded, normalizeURI(gitlab, normalizeBase("https://example.com/spec.json"))) +} diff --git a/normalizer_fuzz_test.go b/normalizer_fuzz_test.go index 4eb6bce..9a2a4b3 100644 --- a/normalizer_fuzz_test.go +++ b/normalizer_fuzz_test.go @@ -18,6 +18,10 @@ import ( // Beyond that, the properties asserted here are the invariants the expander relies on: // normalizing yields a canonical URI, normalizing twice changes nothing, and // denormalizing a canonical $ref yields a shorthand that normalizes back to it. +// +// The last property used to skip a URI that jsonreference spells otherwise than the normalizer +// did. Both now canonicalize the same way - see TestNormalizer_Canonicalization - so it holds +// for every input. func FuzzNormalizer(f *testing.F) { for _, seed := range normalizerSeeds() { f.Add(seed.refPath, seed.base) @@ -34,10 +38,6 @@ func FuzzNormalizer(f *testing.F) { require.EqualTf(t, normalized, normalizeURI(normalized, canonicalBase), "normalizeURI is not idempotent on $ref %q against base %q", refPath, canonicalBase) - if respelled(normalized) { - return - } - ref := MustCreateRef(normalized) denormalized := denormalizeRef(&ref, canonicalBase, "") require.EqualTf(t, normalized, normalizeURI(denormalized.String(), canonicalBase), @@ -46,17 +46,6 @@ func FuzzNormalizer(f *testing.F) { }) } -// respelled reports whether jsonreference renders a URI otherwise than the normalizer does. -// -// Turning a URI into a Ref lower-cases the host, drops a default port and re-escapes path and -// fragment in their canonical form, whereas the normalizer keeps the spelling it was handed. -// Where the two disagree, denormalizing cannot be asked to round-trip. -func respelled(in string) bool { - ref := MustCreateRef(in) - - return ref.String() != in -} - // requireCanonicalURI asserts the postcondition shared by normalizeBase and normalizeURI: // whatever comes out is a parseable, absolute URI, safe to use as a document cache key. func requireCanonicalURI(t *testing.T, in string) { diff --git a/testdata/fuzz/FuzzNormalizer/568a337b9c3336ed b/testdata/fuzz/FuzzNormalizer/568a337b9c3336ed new file mode 100644 index 0000000..91ce637 --- /dev/null +++ b/testdata/fuzz/FuzzNormalizer/568a337b9c3336ed @@ -0,0 +1,3 @@ +go test fuzz v1 +string("0") +string("%2F")