Skip to content

fix(index): never hand the rolodex a nil file with a nil error - #638

Merged
daveshanley merged 1 commit into
mainfrom
fix/remote-fs-nil-file-panic
Sep 25, 2026
Merged

daveshanley merged 1 commit into
mainfrom
fix/remote-fs-nil-file-panic

Conversation

@daveshanley

Copy link
Copy Markdown
Member

Fixes #578. Supersedes #579.

Problem

RemoteFS.OpenWithContext could return (nil, nil). Rolodex.asRemoteFile then passed the nil file to consumeAdaptedFile → io.ReadAll(nil) → nil pointer panic. With ExtractRefsSequentially: false this runs inside singleflight.Group.Do, which re-raises the panic on a new goroutine, so the host process crashes and the caller can't recover.

Reproduced on main, both sequential (recoverable panic) and concurrent (process exit), via two triggers:

  1. BaseURL without a scheme. normalizeRemoteURL copies the base URL's scheme and host onto every remote ref unconditionally, so example.com/specs/ or //example.com/specs/ strips https from https://example.com/schemas/pet.yaml.
  2. A local ref that starts with http, e.g. $ref: httpdocs/pet.yaml. No BaseURL is needed. Rolodex.OpenWithContext routes anything with an http prefix to the remote FS, where it parses with an empty scheme.

Fix

Three small layers:

Where Change
normalizeRemoteURL only rewrites when the base URL has a scheme and a host; otherwise the ref is fetched as written. Proper https://host/... bases behave as before.
RemoteFS.OpenWithContext returns remote URL '…' has no scheme, unable to fetch it instead of (nil, nil), and passes that error to callers waiting on the same in-flight open. Client errors and empty responses are now passed to waiting callers too; before, they got a nil file with no error.
openFile (rolodex) the one place the rolodex opens files from every fs.FS, including user-supplied ones via AddLocalFS/AddRemoteFS. It now converts (nil, nil) into an error, so a misbehaving custom FS can't crash callers either.

Behavior change: a BaseURL with no scheme or no host is now ignored for normalization, the same as having no BaseURL. Absolute remote refs are fetched as written; previously they failed.

Why not #579

#579 has the right normalizeRemoteURL idea. But it changes TestNewRemoteFS_BasicCheck_NoScheme to fetch a non-existent host over the real network, and its new tests reach example.com. It also leaves custom file systems and in-flight waiters able to hand back a nil file with no error.

Tests

No test touches the network; they use canned RemoteURLHandlers. Everything also passes with HTTP(S)_PROXY set to a dead port.

  • issue578_test.go:
    • TestIssue578SchemelessBaseURLDoesNotPanic covers both base URL forms, sequential and concurrent. It asserts no panic and no error, that the handler is called once with the original https URL, and that the schema resolves.
    • TestIssue578HttpPrefixedLocalRefDoesNotPanic covers both modes. It asserts the exact build error and that the scheme error is logged.
  • TestRemoteFS_NormalizeRemoteURL_SkipsBaseWithoutSchemeOrHost covers no scheme or host, host without scheme, and scheme without host.
  • TestRemoteFS_OpenWithContext_NoSchemeReleasesWaitersWithError runs 32 concurrent callers × 50 rounds.
  • TestRemoteFS_OpenWithContext_FailedFetchReleasesWaitersWithError: waiters on a failed or empty fetch get its error. This fails on main.
  • TestRolodex_Open_FileSystemReturnsNoFileAndNoError: a custom fs.FS returning (nil, nil), local and remote.
  • TestRolodex_Open_HttpPrefixedLocationWithoutScheme.
  • TestNewRemoteFS_BasicCheck_NoScheme is updated to the new contract with a canned handler.

Each guard was switched off in turn to confirm its tests fail. Every changed line is covered: openFile, both OpenWithContext and normalizeRemoteURL are at 100%. The new tests are race-clean at -count=3.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (61ea631) to head (bec8d93).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #638   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          297       297           
  Lines        37662     37668    +6     
=========================================
+ Hits         37662     37668    +6     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@daveshanley
daveshanley merged commit 7155b77 into main Sep 25, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

a malformed BaseURI can lead to unrecoverable panics

1 participant