Skip to content

Bug: repository_resource.go defers resp.Body.Close() before nil check — panics on network/request failures #2068

@marlonbarreto-git

Description

@marlonbarreto-git

Bug Description

In pkg/github/repository_resource.go (lines 193-199), defer resp.Body.Close() is registered before the error check on the response. When the underlying HTTP call fails and returns (nil, err), the deferred function panics with a nil pointer dereference.

Affected Code

File: pkg/github/repository_resource.go, lines 193-200

resp, err := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts)
defer func() {
    _ = resp.Body.Close()   // defer registered HERE — captures resp by reference
}()
switch {
case err != nil:
    return nil, fmt.Errorf("failed to get raw content: %w", err)  // returns with resp==nil

Panic Path

GetRawContent in pkg/raw/raw.go returns (nil, err) when:

  • newRequest fails (e.g., invalid URL, cancelled context) — line 69
  • client.Do(req) fails (DNS failure, TLS error, network timeout) — per Go stdlib http.Client.Do contract

When this happens:

  1. resp is nil
  2. The defer was already registered on line 194
  3. Function returns via the case err != nil branch
  4. Deferred function runs: resp.Body.Close()nil pointer dereference → panic

Expected Behavior

Deferred close should guard against nil:

defer func() {
    if resp != nil && resp.Body != nil {
        _ = resp.Body.Close()
    }
}()

Or move the defer after the error check (matching the pattern used in other files like repositories.go line ~1307-1312).

Actual Behavior

Server panics if the raw content HTTP call fails at the transport level.

Proposed Solution

Option A (recommended): Add nil guard in the defer

defer func() {
    if resp != nil && resp.Body != nil {
        _ = resp.Body.Close()
    }
}()

Option B: Move the defer below the error check (same pattern as repositories.go)

resp, err := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts)
if err != nil {
    return nil, fmt.Errorf("failed to get raw content: %w", err)
}
defer func() { _ = resp.Body.Close() }()
Aspect Option A Option B
Safety Handles partial responses (non-nil resp with error) Cleaner but skips closing on partial responses
Consistency Matches guarded pattern in other functions Matches sequential pattern in repositories.go

Happy to submit a PR for this fix.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions