Skip to content

Security: Handle Close() errors on writable files in zip extraction - #9332

Merged
pelikhan merged 3 commits into
mainfrom
copilot/fix-unhandled-file-close-error
Jan 8, 2026
Merged

pelikhan merged 3 commits into
mainfrom
copilot/fix-unhandled-file-close-error

Conversation

Copilot AI commented Jan 8, 2026 •

Copy link
Copy Markdown
Contributor

Security Fix: CWE-252 (Unchecked Return Value)

The extractZipFile function opened files for writing but ignored Close() errors. Since buffered writes may only flush on close, this could cause silent data loss if Close() failed (disk full, I/O errors).

Changes

  • Function signature: Added named return value (extractErr error) to enable error capture in deferred closure
  • Error handling: Replaced defer destFile.Close() with deferred function that captures and propagates Close() errors
  • Consistency: Updated io.Copy error handling to use named return value
// Before
func extractZipFile(f *zip.File, destDir string, verbose bool) error {
    destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
    if err != nil {
        return fmt.Errorf("failed to create destination file: %w", err)
    }
    defer destFile.Close() // Error ignored
    
    if _, err := io.Copy(destFile, srcFile); err != nil {
        return fmt.Errorf("failed to extract file: %w", err)
    }
    return nil
}

// After
func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error) {
    destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
    if err != nil {
        return fmt.Errorf("failed to create destination file: %w", err)
    }
    defer func() {
        if err := destFile.Close(); extractErr == nil && err != nil {
            extractErr = fmt.Errorf("failed to close destination file: %w", err)
        }
    }()
    
    if _, err := io.Copy(destFile, srcFile); err != nil {
        extractErr = fmt.Errorf("failed to extract file: %w", err)
        return extractErr
    }
    return nil
}

Test Coverage

Added comprehensive test suite covering:

  • Normal extraction flow
  • Directory handling
  • Path traversal prevention (zip slip)
  • File permission preservation
  • Nested directory structures
  • Error propagation

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/user
    • Triggering command: /usr/bin/gh gh api user --jq .login 7748965/.github/workflows -tests /usr/bin/infocmp -json GO111MODULE 64/bin/go infocmp -1 /ref/tags/v8 go /usr/bin/infocmp 100351/run-1 GO111MODULE x_amd64/link infocmp (http block)
    • Triggering command: /usr/bin/gh gh api user --jq .login ithub-script/git/ref/tags/v8 rev-parse (http block)
    • Triggering command: /usr/bin/gh gh api user --jq .login /run/containerd/io.containerd.runtime.v2.task/moby/54dbac9f144934f7a4a699d3772c740a82c13b79bd7a0c256529e6deff339c00/init.pid 54dbac9f144934f7a4a699d3772c740a82c13b79bd7a0c256529e6deff339c00 /usr/bin/git 40a82c13b79bd7a0c256529e6deff339c00/log.json GO111MODULE ache/go/1.25.0/x/repos/actions/github-script/git/ref/tags/v8 git -C ithub/workflows rev-parse (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

This section details on the original issue you should resolve

<issue_title>Security Fix: Handle errors from closing writable file (Alert #501)</issue_title>
<issue_description>## Security Fix: Unhandled Writable File Close Error

Alert Number: #501
Severity: Warning
Rule: go/unhandled-writable-file-close (CWE-252)
File: pkg/cli/logs_download.go:397

Vulnerability Description

The extractZipFile function opens a file for writing using os.OpenFile with os.O_WRONLY flag, but does not handle errors that may occur when closing the file. Data written to a file may be cached in memory and only flushed when the file is closed. If Close() fails and the error is ignored, data loss can occur silently without the program being aware of it.

The CodeQL scanner flagged this as: "File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly."

Fix Applied

Applied the recommended pattern from the CodeQL documentation:

  1. Changed function signature to use a named return value: func extractZipFile(...) (extractErr error)

  2. Replaced defer destFile.Close() with a deferred anonymous function that:

    • Calls destFile.Close() and captures any error
    • If Close() fails AND no other error has occurred, sets the return error to the Close() error
    • Prevents silent data loss by ensuring Close() errors are propagated to the caller
  3. Updated the io.Copy error handling to use the named return value for consistency

Code Changes

Before:

func extractZipFile(f *zip.File, destDir string, verbose bool) error {
    // ... setup code ...
    
    destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
    if err != nil {
        return fmt.Errorf("failed to create destination file: %w", err)
    }
    defer destFile.Close() // Error ignored!
    
    if _, err := io.Copy(destFile, srcFile); err != nil {
        return fmt.Errorf("failed to extract file: %w", err)
    }
    
    return nil
}

After:

func extractZipFile(f *zip.File, destDir string, verbose bool) (extractErr error) {
    // ... setup code ...
    
    destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
    if err != nil {
        return fmt.Errorf("failed to create destination file: %w", err)
    }
    defer func() {
        // Handle errors from closing the writable file to prevent data loss
        if err := destFile.Close(); extractErr == nil && err != nil {
            extractErr = fmt.Errorf("failed to close destination file: %w", err)
        }
    }()
    
    if _, err := io.Copy(destFile, srcFile); err != nil {
        extractErr = fmt.Errorf("failed to extract file: %w", err)
        return extractErr
    }
    
    return nil
}

Security Best Practices Applied

  • Error Handling: Explicit error handling for all critical operations that can fail
  • Data Integrity: Ensures that file write operations are fully flushed before function returns
  • Reliability: Prevents silent data loss by capturing and reporting Close() errors
  • Go Best Practices: Follows the pattern recommended in Go documentation for handling deferred Close() on writable files

Testing Considerations

  • Verify that zip extraction continues to work correctly for normal cases
  • Test error conditions where disk is full or file system has errors during flush
  • Ensure that Close() errors are properly propagated to callers
  • Validate that extracted files are complete and not corrupted

References

  • CWE-252: Unchecked Return Value
  • Go Language Specification: Defer statements
  • CodeQL Query: go/unhandled-writable-file-close

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

AI generated by Security Fix PR


[!NOTE]
This was originally intended as a pull request, but the git push operation failed.

Workflow Run: View run details and download patch artifact

The patch file is available in the agent-artifacts artifact in the workflow run linked above.

To apply the patch locally:

# Download the artifact from the workflow run https://github.com/githubnext/gh-aw/actions/runs/20810109004
# (Use GitHub MCP tools if gh CLI is not available)
gh run download 20810109004 -n agent-artifacts

# The patch file will be at agent-artifacts/tmp/gh-aw/aw.patch after download
# Apply the patch
git am agent-artifacts/tmp/gh-aw/aw.patch
Show patch preview (58 of 58 lines)
From d73182a0dcfd166f2d333143e594dc403944d6aa Mon Sep 17 00:00:00 2001
From: "github-actions[bot]" <github-actions[bot]@users.noreply.github.com>
Date: Thu, 8 Jan 2026 08:18:32 +0000
Subject: [PATCH] Security ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes githubnext/gh-aw#9328

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/githubnext/gh-aw/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 8, 2026 09:08
…mprehensive tests

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix unhandled writable file close error in extractZipFile function Security: Handle Close() errors on writable files in zip extraction Jan 8, 2026
Copilot AI requested a review from mnkiefer January 8, 2026 09:16
@pelikhan
pelikhan marked this pull request as ready for review January 8, 2026 12:19
@pelikhan
pelikhan merged commit be61873 into main Jan 8, 2026
113 checks passed
@pelikhan
pelikhan deleted the copilot/fix-unhandled-file-close-error branch January 8, 2026 12:20
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.

3 participants