From bec8d93480bba5d92ad35aa62e64f2189e8578a6 Mon Sep 17 00:00:00 2001 From: quobix Date: Fri, 25 Sep 2026 09:01:31 -0400 Subject: [PATCH] fix(index): never hand the rolodex a nil file with a nil error A BaseURL without a scheme (e.g. "example.com/specs/" or "//example.com/specs/") made normalizeRemoteURL overwrite every remote ref's scheme with an empty one. RemoteFS.OpenWithContext then returned (nil, nil) for the scheme-less URL, and Rolodex.asRemoteFile passed the nil file to io.ReadAll, which panicked. When refs are extracted concurrently the panic happens inside singleflight, which re-raises it on a fresh goroutine, so the host process crashed with nothing able to recover. A local ref that merely starts with "http" (httpdocs/pet.yaml) reached the same (nil, nil) without any BaseURL. - normalizeRemoteURL only rewrites a remote URL when the base URL has both a scheme and a host; otherwise the ref is fetched as written. - OpenWithContext returns an error for a URL with no scheme, and hands that error to callers waiting on the same in-flight open. Client errors and empty responses are now passed to waiting callers too, instead of a nil file and no error. - openFile, which the rolodex uses for every file system including user-supplied ones, turns a (nil, nil) result into an error. Fixes #578 Co-Authored-By: Claude Opus 5.5 --- index/rolodex.go | 5 ++ index/rolodex_remote_loader.go | 17 ++-- index/rolodex_remote_loader_test.go | 108 ++++++++++++++++++++-- index/rolodex_test.go | 47 ++++++++++ issue578_test.go | 134 ++++++++++++++++++++++++++++ 5 files changed, 301 insertions(+), 10 deletions(-) create mode 100644 issue578_test.go diff --git a/index/rolodex.go b/index/rolodex.go index bd955a836..abe35c02a 100644 --- a/index/rolodex.go +++ b/index/rolodex.go @@ -905,6 +905,11 @@ func openFile(ctx context.Context, location string, v fs.FS) (fs.File, error) { } else { f, err = v.Open(location) } + // a file system returning neither a file nor an error breaks the fs.FS contract, + // reading from the nil file would panic, so treat it as a failed open. + if f == nil && err == nil { + return nil, fmt.Errorf("file system returned no file and no error when opening '%s'", location) + } return f, err } diff --git a/index/rolodex_remote_loader.go b/index/rolodex_remote_loader.go index 3a81f2354..7f9b01447 100644 --- a/index/rolodex_remote_loader.go +++ b/index/rolodex_remote_loader.go @@ -586,8 +586,9 @@ func (i *RemoteFS) OpenWithContext(ctx context.Context, remoteURL string) (fs.Fi } if remoteParsedURL.Scheme == "" { - i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, nil) - return nil, nil // not a remote file — scheme is empty, skip processing. + schemeErr := fmt.Errorf("remote URL '%s' has no scheme, unable to fetch it", remoteURL) + i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, schemeErr) + return nil, schemeErr } i.logger.Debug("[rolodex remote loader] loading remote file", "file", remoteURL, "remoteURL", remoteParsedURL.String()) @@ -595,7 +596,7 @@ func (i *RemoteFS) OpenWithContext(ctx context.Context, remoteURL string) (fs.Fi response, clientErr := i.RemoteHandlerFunc(remoteParsedURL.String()) if clientErr != nil { i.appendRemoteError(clientErr) - i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, nil) + i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, clientErr) if response != nil && response.Body != nil { _ = response.Body.Close() } @@ -607,8 +608,9 @@ func (i *RemoteFS) OpenWithContext(ctx context.Context, remoteURL string) (fs.Fi return nil, clientErr } if response == nil { - i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, nil) - return nil, fmt.Errorf("empty response from remote URL: %s", remoteParsedURL.String()) + emptyErr := fmt.Errorf("empty response from remote URL: %s", remoteParsedURL.String()) + i.releaseRemoteProcessingWaiter(processingWaiter, cacheKey, nil, emptyErr) + return nil, emptyErr } defer func() { if response.Body != nil { @@ -650,6 +652,11 @@ func (i *RemoteFS) normalizeRemoteURL(remoteParsedURL *url.URL) { if i.rootURLParsed == nil || remoteParsedURL == nil { return } + // a base URL without a scheme or host (e.g. 'example.com/specs/') cannot be fetched from, + // rewriting with it would strip the scheme from every absolute remote reference. + if i.rootURLParsed.Scheme == "" || i.rootURLParsed.Host == "" { + return + } remoteParsedURL.Host = i.rootURLParsed.Host remoteParsedURL.Scheme = i.rootURLParsed.Scheme } diff --git a/index/rolodex_remote_loader_test.go b/index/rolodex_remote_loader_test.go index ec42305ab..354d8ffbb 100644 --- a/index/rolodex_remote_loader_test.go +++ b/index/rolodex_remote_loader_test.go @@ -129,16 +129,90 @@ func TestNewRemoteFS_BasicCheck_Valid(t *testing.T) { } func TestNewRemoteFS_BasicCheck_NoScheme(t *testing.T) { - server := test_buildServer() - defer server.Close() - remoteFS, _ := NewRemoteFSWithRootURL("") - remoteFS.RemoteHandlerFunc = test_httpClient.Get + var requested []string + remoteFS.RemoteHandlerFunc = func(u string) (*http.Response, error) { + requested = append(requested, u) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("type: object")), + }, nil + } + // an empty root URL has no scheme or host, so the remote URL is fetched untouched. file, err := remoteFS.Open("https://ding-dong-bing-bong.com/file1.yaml") - assert.NoError(t, err) + assert.NotNil(t, file) + assert.Equal(t, "https://ding-dong-bing-bong.com/file1.yaml", file.(*RemoteFile).GetFullPath()) + + // a location without a scheme cannot be fetched, it must fail instead of returning no file and no error. + file, err = remoteFS.Open("httpdocs/file1.yaml") assert.Nil(t, file) + assert.EqualError(t, err, "remote URL 'httpdocs/file1.yaml' has no scheme, unable to fetch it") + assert.Equal(t, []string{"https://ding-dong-bing-bong.com/file1.yaml"}, requested) +} + +func TestRemoteFS_OpenWithContext_NoSchemeReleasesWaitersWithError(t *testing.T) { + remoteFS, _ := NewRemoteFSWithRootURL("") + remoteFS.RemoteHandlerFunc = func(u string) (*http.Response, error) { + t.Errorf("a location without a scheme must never be fetched: %s", u) + return nil, errors.New("unexpected fetch") + } + + // callers that wait on an in-flight open of the same location must receive the same error. + const workers = 32 + for round := 0; round < 50; round++ { + start := make(chan struct{}) + errs := make(chan error, workers) + for w := 0; w < workers; w++ { + go func() { + <-start + _, err := remoteFS.OpenWithContext(context.Background(), "httpdocs/shared.yaml") + errs <- err + }() + } + close(start) + for w := 0; w < workers; w++ { + assert.EqualError(t, <-errs, "remote URL 'httpdocs/shared.yaml' has no scheme, unable to fetch it") + } + } +} + +func TestRemoteFS_OpenWithContext_FailedFetchReleasesWaitersWithError(t *testing.T) { + tests := []struct { + name string + response *http.Response + err error + expected string + }{ + {name: "client error", err: errors.New("connection refused"), expected: "connection refused"}, + {name: "no response", expected: "empty response from remote URL: https://example.com/shared.yaml"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + remoteFS, _ := NewRemoteFSWithRootURL("") + release := make(chan struct{}) + remoteFS.RemoteHandlerFunc = func(string) (*http.Response, error) { + <-release + return tt.response, tt.err + } + + // callers that wait on the in-flight fetch must receive its error, not a nil file and no error. + const workers = 16 + errs := make(chan error, workers) + for w := 0; w < workers; w++ { + go func() { + _, err := remoteFS.OpenWithContext(context.Background(), "https://example.com/shared.yaml") + errs <- err + }() + } + time.Sleep(50 * time.Millisecond) + close(release) + for w := 0; w < workers; w++ { + assert.EqualError(t, <-errs, tt.expected) + } + }) + } } func TestNewRemoteFS_BasicCheck_Relative(t *testing.T) { @@ -559,6 +633,30 @@ func TestRemoteFS_NormalizeAndLoadCachedHelpers(t *testing.T) { assert.Same(t, legacyFile, rfs.loadCachedRemoteFile("https://root.example/spec.yaml", "/spec.yaml")) } +func TestRemoteFS_NormalizeRemoteURL_SkipsBaseWithoutSchemeOrHost(t *testing.T) { + tests := []struct { + name string + base string + }{ + {name: "no scheme or host", base: "example.com/specs/"}, + {name: "host without scheme", base: "//example.com/specs/"}, + {name: "scheme without host", base: "file:///specs/"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := CreateOpenAPIIndexConfig() + config.BaseURL, _ = url.Parse(tt.base) + rfs, err := NewRemoteFSWithConfig(config) + assert.NoError(t, err) + + target, err := url.Parse("https://other.example/schemas/pet.yaml") + assert.NoError(t, err) + rfs.normalizeRemoteURL(target) + assert.Equal(t, "https://other.example/schemas/pet.yaml", target.String()) + }) + } +} + func TestRemoteFS_CreateRemoteHelpers(t *testing.T) { config := CreateOpenAPIIndexConfig() rfs, err := NewRemoteFSWithConfig(config) diff --git a/index/rolodex_test.go b/index/rolodex_test.go index 5d1080592..357ddd6f3 100644 --- a/index/rolodex_test.go +++ b/index/rolodex_test.go @@ -3002,3 +3002,50 @@ func TestRolodex_Release_ConcurrentSafe(t *testing.T) { _ = rolodex.GetIndexes() <-done } + +// nilFileFS breaks the fs.FS contract by returning neither a file nor an error. +type nilFileFS struct{} + +func (nilFileFS) Open(string) (fs.File, error) { + return nil, nil +} + +func TestRolodex_Open_FileSystemReturnsNoFileAndNoError(t *testing.T) { + rolo := NewRolodex(CreateOpenAPIIndexConfig()) + rolo.AddLocalFS(t.TempDir(), nilFileFS{}) + rolo.AddRemoteFS("", nilFileFS{}) + + var rf RolodexFile + var err error + assert.NotPanics(t, func() { + rf, err = rolo.Open("https://example.com/spec.yaml") + }) + assert.Nil(t, rf) + assert.EqualError(t, err, "file system returned no file and no error when opening 'https://example.com/spec.yaml'") + + assert.NotPanics(t, func() { + rf, err = rolo.Open("spec.yaml") + }) + assert.Nil(t, rf) + assert.EqualError(t, err, "file system returned no file and no error when opening 'spec.yaml'") +} + +func TestRolodex_Open_HttpPrefixedLocationWithoutScheme(t *testing.T) { + cfg := CreateOpenAPIIndexConfig() + remoteFS, err := NewRemoteFSWithConfig(cfg) + assert.NoError(t, err) + remoteFS.RemoteHandlerFunc = func(u string) (*http.Response, error) { + t.Errorf("a location without a scheme must never be fetched: %s", u) + return nil, errors.New("unexpected fetch") + } + rolo := NewRolodex(cfg) + rolo.AddRemoteFS("", remoteFS) + + // 'httpdocs/pet.yaml' is routed to the remote file system because it starts with 'http'. + var rf RolodexFile + assert.NotPanics(t, func() { + rf, err = rolo.Open("httpdocs/pet.yaml") + }) + assert.Nil(t, rf) + assert.EqualError(t, err, "remote URL 'httpdocs/pet.yaml' has no scheme, unable to fetch it") +} diff --git a/issue578_test.go b/issue578_test.go new file mode 100644 index 000000000..f4eeaea72 --- /dev/null +++ b/issue578_test.go @@ -0,0 +1,134 @@ +// Copyright 2026 Princess Beef Heavy Industries / Dave Shanley +// https://pb33f.io +// SPDX-License-Identifier: MIT +package libopenapi + +import ( + "bytes" + "fmt" + "io" + "log/slog" + "net/http" + "net/url" + "sync" + "testing" + + "github.com/pb33f/libopenapi/datamodel" + v3high "github.com/pb33f/libopenapi/datamodel/high/v3" + "github.com/pb33f/testify/require" +) + +const issue578Spec = `openapi: 3.1.0 +info: + title: t + version: 1.0.0 +paths: + /x: + get: + responses: + "200": + description: ok + content: + application/json: + schema: + $ref: "%s" +` + +func issue578Config(sequential bool, requested *[]string, mu *sync.Mutex) *datamodel.DocumentConfiguration { + cfg := datamodel.NewDocumentConfiguration() + cfg.AllowRemoteReferences = true + cfg.ExtractRefsSequentially = sequential + cfg.RemoteURLHandler = func(u string) (*http.Response, error) { + mu.Lock() + *requested = append(*requested, u) + mu.Unlock() + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString("type: object\n")), + }, nil + } + return cfg +} + +// A BaseURL without a scheme must not strip the scheme from absolute remote refs. The rolodex +// used to receive no file and no error, then panic reading it; when refs were extracted +// concurrently the panic was re-raised on a fresh goroutine and killed the process. +func TestIssue578SchemelessBaseURLDoesNotPanic(t *testing.T) { + tests := []struct { + name string + base string + }{ + {name: "no scheme or host", base: "example.com/specs/"}, + {name: "host without scheme", base: "//example.com/specs/"}, + } + for _, tt := range tests { + for _, sequential := range []bool{true, false} { + name := tt.name + " concurrent" + if sequential { + name = tt.name + " sequential" + } + t.Run(name, func(t *testing.T) { + var mu sync.Mutex + var requested []string + cfg := issue578Config(sequential, &requested, &mu) + baseURL, err := url.Parse(tt.base) + require.NoError(t, err) + cfg.BaseURL = baseURL + + spec := []byte(fmt.Sprintf(issue578Spec, "https://example.com/schemas/pet.yaml")) + doc, err := NewDocumentWithConfiguration(spec, cfg) + require.NoError(t, err) + + var model *DocumentModel[v3high.Document] + require.NotPanics(t, func() { + model, err = doc.BuildV3Model() + }) + require.NoError(t, err) + require.NotNil(t, model) + + mu.Lock() + require.Equal(t, []string{"https://example.com/schemas/pet.yaml"}, requested) + mu.Unlock() + + pathItem, ok := model.Model.Paths.PathItems.Get("/x") + require.True(t, ok) + mediaType, ok := pathItem.Get.Responses.Codes.GetOrZero("200").Content.Get("application/json") + require.True(t, ok) + schema := mediaType.Schema.Schema() + require.NotNil(t, schema) + require.Equal(t, []string{"object"}, schema.Type) + }) + } + } +} + +// A local ref that merely starts with 'http' is routed to the remote file system, where it has no +// scheme. It must fail to resolve with an error rather than panic. +func TestIssue578HttpPrefixedLocalRefDoesNotPanic(t *testing.T) { + for _, sequential := range []bool{true, false} { + name := "concurrent" + if sequential { + name = "sequential" + } + t.Run(name, func(t *testing.T) { + var mu sync.Mutex + var requested []string + var logs bytes.Buffer + cfg := issue578Config(sequential, &requested, &mu) + cfg.Logger = slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelError})) + + doc, err := NewDocumentWithConfiguration([]byte(fmt.Sprintf(issue578Spec, "httpdocs/pet.yaml")), cfg) + require.NoError(t, err) + + require.NotPanics(t, func() { + _, err = doc.BuildV3Model() + }) + require.EqualError(t, err, "component `httpdocs/pet.yaml` does not exist in the specification") + require.Contains(t, logs.String(), "remote URL 'httpdocs/pet.yaml' has no scheme, unable to fetch it") + + mu.Lock() + require.Empty(t, requested) + mu.Unlock() + }) + } +}