diff --git a/internal/file/file_operator.go b/internal/file/file_operator.go index c54482199..afd1a171d 100644 --- a/internal/file/file_operator.go +++ b/internal/file/file_operator.go @@ -155,7 +155,7 @@ func (fo *FileOperator) ReadChunk( func (fo *FileOperator) WriteManifestFile( ctx context.Context, updatedFiles map[string]*model.ManifestFile, manifestDir, manifestPath string, -) (writeError error) { +) error { slog.DebugContext(ctx, "Writing manifest file", "updated_files", updatedFiles) manifestJSON, err := json.MarshalIndent(updatedFiles, "", " ") if err != nil { @@ -164,28 +164,50 @@ func (fo *FileOperator) WriteManifestFile( fo.manifestLock.Lock() defer fo.manifestLock.Unlock() + // 0755 allows read/execute for all, write for owner if err = os.MkdirAll(manifestDir, dirPerm); err != nil { return fmt.Errorf("unable to create directory %s: %w", manifestDir, err) } - // 0600 ensures only root can read/write - newFile, err := os.OpenFile(manifestPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePerm) + // Write to a temporary file first to ensure atomicity + tempManifestFilePath := manifestPath + ".tmp" + tempFile, err := os.OpenFile(tempManifestFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, filePerm) if err != nil { - return fmt.Errorf("failed to read manifest file: %w", err) + return fmt.Errorf("failed to open file %s: %w", tempManifestFilePath, err) } - defer func() { - if closeErr := newFile.Close(); closeErr != nil { - writeError = closeErr - } - }() - _, err = newFile.Write(manifestJSON) + if _, err = tempFile.Write(manifestJSON); err != nil { + closeFile(ctx, tempFile) + + return fmt.Errorf("failed to write to file %s: %w", tempManifestFilePath, err) + } + + closeFile(ctx, tempFile) + + // Verify the contents of the temporary file is JSON + file, err := os.ReadFile(tempManifestFilePath) if err != nil { - return fmt.Errorf("failed to write manifest file: %w", err) + return fmt.Errorf("failed to read file %s: %w", tempManifestFilePath, err) } - return writeError + var manifestFiles map[string]*model.ManifestFile + + err = json.Unmarshal(file, &manifestFiles) + if err != nil { + if len(file) == 0 { + return fmt.Errorf("file %s is empty: %w", tempManifestFilePath, err) + } + + return fmt.Errorf("failed to parse file %s: %w", tempManifestFilePath, err) + } + + // Rename the temporary file to the actual manifest file path + if renameError := os.Rename(tempManifestFilePath, manifestPath); renameError != nil { + return fmt.Errorf("failed to file %s to %s: %w", tempManifestFilePath, manifestPath, renameError) + } + + return nil } func (fo *FileOperator) MoveFile(ctx context.Context, sourcePath, destPath string) error { diff --git a/internal/file/file_operator_test.go b/internal/file/file_operator_test.go index bf1a00d44..a16c9140f 100644 --- a/internal/file/file_operator_test.go +++ b/internal/file/file_operator_test.go @@ -7,6 +7,7 @@ package file import ( "context" + "encoding/json" "os" "path" "path/filepath" @@ -44,13 +45,89 @@ func TestFileOperator_Write(t *testing.T) { assert.Equal(t, fileContent, data) } +func TestFileOperator_WriteManifestFile(t *testing.T) { + tempDir := t.TempDir() + manifestPath := path.Join(tempDir, "manifest.json") + + manifestFiles := map[string]*model.ManifestFile{ + "/etc/nginx/nginx.conf": { + ManifestFileMeta: &model.ManifestFileMeta{ + Name: "/etc/nginx/nginx.conf", + Size: 1024, + Hash: "6d232d32d44", + Referenced: true, + Unmanaged: false, + }, + }, + "/etc/nginx/conf.d/default.conf": { + ManifestFileMeta: &model.ManifestFileMeta{ + Name: "/etc/nginx/conf.d/default.conf", + Size: 32342, + Hash: "1eh32hd3792hd329", + Referenced: true, + Unmanaged: false, + }, + }, + } + + fileOperator := NewFileOperator(&sync.RWMutex{}) + err := fileOperator.WriteManifestFile(t.Context(), manifestFiles, tempDir, manifestPath) + require.NoError(t, err) + + assert.FileExists(t, manifestPath) + assert.NoFileExists(t, manifestPath+".tmp") + + // Verify the contents can be read back + data, err := os.ReadFile(manifestPath) + require.NoError(t, err) + + var readBack map[string]*model.ManifestFile + err = json.Unmarshal(data, &readBack) + require.NoError(t, err) + assert.Equal(t, manifestFiles, readBack) +} + +func TestFileOperator_WriteManifestFile_directoryCreationError(t *testing.T) { + manifestPath := "/unknown/manifest.json" + manifestDir := "/unknown" + + fileOperator := NewFileOperator(&sync.RWMutex{}) + err := fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), manifestDir, manifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "unable to create directory") +} + +func TestFileOperator_WriteManifestFile_tempFileCreationError(t *testing.T) { + tempDir := t.TempDir() + + // Create a file where we want to write the manifest + manifestPath := path.Join(tempDir, "manifest.json") + err := os.WriteFile(manifestPath, []byte("existing"), 0o400) // readonly + require.NoError(t, err) + + // Create readonly directory to prevent temp file creation + err = os.Chmod(tempDir, 0o444) + require.NoError(t, err) + defer func() { + revertPermissionsError := os.Chmod(tempDir, 0o755) + require.NoError(t, revertPermissionsError) + }() + + fileOperator := NewFileOperator(&sync.RWMutex{}) + err = fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), tempDir, manifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to open file") +} + func TestFileOperator_WriteManifestFile_fileMissing(t *testing.T) { tempDir := t.TempDir() manifestPath := "/unknown/manifest.json" fileOperator := NewFileOperator(&sync.RWMutex{}) err := fileOperator.WriteManifestFile(t.Context(), make(map[string]*model.ManifestFile), tempDir, manifestPath) - assert.Error(t, err) + require.Error(t, err) + + assert.NoFileExists(t, manifestPath+".tmp") } func TestFileOperator_MoveFile_fileExists(t *testing.T) {