Skip to content

Commit

Permalink
fix: prevent policy file overwrite on downloads (open-policy-agent#1039)
Browse files Browse the repository at this point in the history
File existence check before downloading policies. Errors out
and no overwrites. Maintains data integrity by preventing
accidental policy overwrites.

Added a test which verified the behaviour.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
  • Loading branch information
thevilledev authored Jan 22, 2025
1 parent 5b3e926 commit 163bdd8
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 0 deletions.
9 changes: 9 additions & 0 deletions downloader/downloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package downloader
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

getter "github.com/hashicorp/go-getter"
Expand Down Expand Up @@ -38,6 +40,13 @@ func Download(ctx context.Context, dst string, urls []string) error {
return fmt.Errorf("detecting url: %w", err)
}

// Check if file already exists
filename := filepath.Base(detectedURL)
targetPath := filepath.Join(dst, filename)
if _, err := os.Stat(targetPath); err == nil {
return fmt.Errorf("policy file already exists at %s, refusing to overwrite", targetPath)
}

client := &getter.Client{
Ctx: ctx,
Src: detectedURL,
Expand Down
62 changes: 62 additions & 0 deletions downloader/downloader_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package downloader

import (
"context"
"fmt"
"net"
"net/http"
"os"
"path/filepath"
"testing"
"time"
)

func TestDownloadFailsWhenFileExists(t *testing.T) {
tmpDir := t.TempDir()

// Create a file that would conflict with the download
existingFile := filepath.Join(tmpDir, "policy.rego")
if err := os.WriteFile(existingFile, []byte("existing content"), os.FileMode(0600)); err != nil {
t.Fatal(err)
}

// Start a test HTTP server on an ephemeral port
listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
defer listener.Close()

server := &http.Server{
Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
fmt.Fprint(w, "new content")
}),
ReadHeaderTimeout: 1 * time.Second,
}
errCh := make(chan error, 1)
go func() {
errCh <- server.Serve(listener)
}()
defer server.Close()

// Try to download a policy file with the same name
urls := []string{fmt.Sprintf("http://%s/policy.rego", listener.Addr().String())}
downloadErr := Download(context.Background(), tmpDir, urls)

// Verify that download fails with the expected error
if downloadErr == nil {
t.Error("Expected download to fail when file exists, but it succeeded")
}
if downloadErr != nil && !filepath.IsAbs(existingFile) {
t.Errorf("Expected error message to contain absolute path, got: %v", downloadErr)
}

// Verify the original file is unchanged
content, err := os.ReadFile(existingFile)
if err != nil {
t.Fatal(err)
}
if string(content) != "existing content" {
t.Error("Existing file was modified")
}
}

0 comments on commit 163bdd8

Please sign in to comment.