diff --git a/agent/agentfiles/files.go b/agent/agentfiles/files.go index b526c60eba..b82b57ce6d 100644 --- a/agent/agentfiles/files.go +++ b/agent/agentfiles/files.go @@ -14,7 +14,6 @@ import ( "syscall" "github.com/google/uuid" - "github.com/spf13/afero" "golang.org/x/xerrors" "cdr.dev/slog/v3" @@ -335,69 +334,16 @@ func (api *API) writeFile(ctx context.Context, r *http.Request, path string) (HT // Check if the target already exists so we can preserve its // permissions on the temp file before rename. - var origMode os.FileMode - var haveOrigMode bool + var mode *os.FileMode if stat, serr := api.filesystem.Stat(path); serr == nil { if stat.IsDir() { return http.StatusBadRequest, xerrors.Errorf("open %s: is a directory", path) } - origMode = stat.Mode() - haveOrigMode = true + m := stat.Mode() + mode = &m } - // Write to a temp file in the same directory so the rename is - // always on the same device (atomic). - tmpfile, err := afero.TempFile(api.filesystem, dir, filepath.Base(path)) - if err != nil { - status := http.StatusInternalServerError - if errors.Is(err, os.ErrPermission) { - status = http.StatusForbidden - } - return status, err - } - tmpName := tmpfile.Name() - - _, err = io.Copy(tmpfile, r.Body) - if err != nil && !errors.Is(err, io.EOF) { - _ = tmpfile.Close() - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } - return http.StatusInternalServerError, xerrors.Errorf("write %s: %w", path, err) - } - - // Close before rename to flush buffered data and catch write - // errors (e.g. delayed allocation failures). - if err := tmpfile.Close(); err != nil { - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } - return http.StatusInternalServerError, xerrors.Errorf("write %s: %w", path, err) - } - - // Set permissions on the temp file before rename so there is - // no window where the target has wrong permissions. - if haveOrigMode { - if err := api.filesystem.Chmod(tmpName, origMode); err != nil { - api.logger.Warn(ctx, "unable to set file permissions", - slog.F("path", path), - slog.Error(err), - ) - } - } - - if err := api.filesystem.Rename(tmpName, path); err != nil { - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } - status := http.StatusInternalServerError - if errors.Is(err, os.ErrPermission) { - status = http.StatusForbidden - } - return status, err - } - - return 0, nil + return api.atomicWrite(ctx, path, mode, r.Body) } func (api *API) HandleEditFiles(rw http.ResponseWriter, r *http.Request) { @@ -500,45 +446,20 @@ func (api *API) editFile(ctx context.Context, path string, edits []workspacesdk. } } - // Create an adjacent file to ensure it will be on the same device and can be - // moved atomically. - tmpfile, err := afero.TempFile(api.filesystem, filepath.Dir(path), filepath.Base(path)) + m := stat.Mode() + return api.atomicWrite(ctx, path, &m, strings.NewReader(content)) +} + +// atomicWrite writes content from r to path via a temp file in the +// same directory. If the target exists, its permissions are preserved. +// On failure the temp file is cleaned up and the original is +// untouched. +func (api *API) atomicWrite(ctx context.Context, path string, mode *os.FileMode, r io.Reader) (int, error) { + dir := filepath.Dir(path) + tmpName := filepath.Join(dir, fmt.Sprintf(".%s.tmp.%s", filepath.Base(path), uuid.New().String()[:8])) + + tmpfile, err := api.filesystem.OpenFile(tmpName, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o666) if err != nil { - return http.StatusInternalServerError, err - } - tmpName := tmpfile.Name() - - if _, err := tmpfile.Write([]byte(content)); err != nil { - _ = tmpfile.Close() - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } - return http.StatusInternalServerError, xerrors.Errorf("edit %s: %w", path, err) - } - - // Close before rename to flush buffered data and catch write - // errors (e.g. delayed allocation failures). - if err := tmpfile.Close(); err != nil { - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } - return http.StatusInternalServerError, xerrors.Errorf("edit %s: %w", path, err) - } - - // Set permissions on the temp file before rename so there is - // no window where the target has wrong permissions. - if err := api.filesystem.Chmod(tmpName, stat.Mode()); err != nil { - api.logger.Warn(ctx, "unable to set file permissions", - slog.F("path", path), - slog.Error(err), - ) - } - - err = api.filesystem.Rename(tmpName, path) - if err != nil { - if rerr := api.filesystem.Remove(tmpName); rerr != nil { - api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(rerr)) - } status := http.StatusInternalServerError if errors.Is(err, os.ErrPermission) { status = http.StatusForbidden @@ -546,6 +467,46 @@ func (api *API) editFile(ctx context.Context, path string, edits []workspacesdk. return status, err } + cleanup := func() { + if err := api.filesystem.Remove(tmpName); err != nil { + api.logger.Warn(ctx, "unable to clean up temp file", slog.Error(err)) + } + } + + _, err = io.Copy(tmpfile, r) + if err != nil { + _ = tmpfile.Close() + cleanup() + return http.StatusInternalServerError, xerrors.Errorf("write %s: %w", path, err) + } + + // Close before rename to flush buffered data and catch write + // errors (e.g. delayed allocation failures). + if err := tmpfile.Close(); err != nil { + cleanup() + return http.StatusInternalServerError, xerrors.Errorf("write %s: %w", path, err) + } + + // Set permissions on the temp file before rename so there is + // no window where the target has wrong permissions. + if mode != nil { + if err := api.filesystem.Chmod(tmpName, *mode); err != nil { + api.logger.Warn(ctx, "unable to set file permissions", + slog.F("path", path), + slog.Error(err), + ) + } + } + + if err := api.filesystem.Rename(tmpName, path); err != nil { + cleanup() + status := http.StatusInternalServerError + if errors.Is(err, os.ErrPermission) { + status = http.StatusForbidden + } + return status, xerrors.Errorf("write %s: %w", path, err) + } + return 0, nil } diff --git a/agent/agentfiles/files_test.go b/agent/agentfiles/files_test.go index a59204429e..d962b80da0 100644 --- a/agent/agentfiles/files_test.go +++ b/agent/agentfiles/files_test.go @@ -636,6 +636,8 @@ func TestEditFiles(t *testing.T) { }, errCode: http.StatusInternalServerError, errors: []string{"rename failed"}, + // Original file must survive the failed rename. + expected: map[string]string{failRenameFilePath: "foo bar"}, }, { name: "Edit1",