diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index de50e5ddc..19136fd56 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -17,6 +17,7 @@ import ( "github.com/fatih/color" "github.com/spf13/cobra" + "github.com/PerpetualSoftware/pad/internal/attachments" "github.com/PerpetualSoftware/pad/internal/cli" "github.com/PerpetualSoftware/pad/internal/items" @@ -2845,13 +2846,9 @@ Examples: // Default filename: the server-suggested Content-Disposition // filename (.pad.md), falling back to the ref when the - // header is missing. + // header is missing. See defaultExportPath for the device rule. if outPath == "" { - if res.Filename != "" { - outPath = res.Filename - } else { - outPath = args[0] + ".pad.md" - } + outPath = defaultExportPath(res.Filename, args[0]) } if err := writeFileAtomic(outPath, res.Body, 0o644); err != nil { @@ -2865,6 +2862,25 @@ Examples: return cmd } +// defaultExportPath is the file `pad item export` writes when no -o is given: +// the server-suggested name, else ".pad.md". +// +// BUG-3185: when that name's stem is a Windows device ("nul.pad.md", +// "con.pad.md"), it is prefixed "_" here as well as on the server. The CLI is +// the process that writes the file, and a server older than the fix still +// suggests the device name; the ref fallback never passed through the server +// at all. +func defaultExportPath(serverName, ref string) string { + name := serverName + if name == "" { + name = ref + ".pad.md" + } + if attachments.WindowsDeviceName(name) { + return "_" + name + } + return name +} + // writeFileAtomic writes data to a sibling temp file, fsyncs it, then // atomically renames it onto path. Unlike os.WriteFile it never // truncates an existing destination before the bytes are known-good, so diff --git a/cmd/pad/item_export_device_test.go b/cmd/pad/item_export_device_test.go new file mode 100644 index 000000000..84b1a58ec --- /dev/null +++ b/cmd/pad/item_export_device_test.go @@ -0,0 +1,26 @@ +package main + +import "testing" + +// BUG-3185: the CLI writes the export file itself, so it applies the device +// rule to the name it will write, whether that name came from the server +// (an older server still suggests "nul.pad.md") or from the ref fallback. +func TestDefaultExportPath_BUG3185(t *testing.T) { + cases := []struct{ server, ref, want string }{ + // Server-suggested names. + {"nul.pad.md", "PLAYB-1", "_nul.pad.md"}, + {"CON.pad.md", "PLAYB-1", "_CON.pad.md"}, + {"_nul.pad.md", "PLAYB-1", "_nul.pad.md"}, // a fixed server's name is not prefixed twice + {"ship.pad.md", "PLAYB-1", "ship.pad.md"}, + // The ref fallback, when the header is absent. + {"", "nul", "_nul.pad.md"}, + {"", "com1", "_com1.pad.md"}, + {"", "PLAYB-1", "PLAYB-1.pad.md"}, + {"", "nulls", "nulls.pad.md"}, + } + for _, c := range cases { + if got := defaultExportPath(c.server, c.ref); got != c.want { + t.Errorf("defaultExportPath(%q, %q) = %q, want %q", c.server, c.ref, got, c.want) + } + } +} diff --git a/internal/attachments/filename.go b/internal/attachments/filename.go index ff9245cdf..e73b7c1cf 100644 --- a/internal/attachments/filename.go +++ b/internal/attachments/filename.go @@ -72,7 +72,7 @@ const fallbackFilename = "upload.bin" // becomes "upload.bin". // 6. A Windows reserved device name gets a "_" prefix (BUG-2822): Windows // opens "nul.txt" as the null device, so a download saved under the stored -// name is lost or refused. See windowsDeviceName for the match. +// name is lost or refused. See WindowsDeviceName for the match. func NormalizeFilename(raw string) string { name := filepath.Base(raw) if i := strings.LastIndexByte(name, '\\'); i >= 0 { @@ -99,7 +99,7 @@ func NormalizeFilename(raw string) string { if name == "" || name == "." || name == ".." || name == "/" { return fallbackFilename } - if windowsDeviceName(name) { + if WindowsDeviceName(name) { return "_" + name } return name @@ -118,12 +118,16 @@ var windowsReservedStems = func() map[string]bool { return m }() -// windowsDeviceName reports whether Windows would open name as a device. It +// WindowsDeviceName reports whether Windows would open name as a device. It // reads the name as Windows does: the stem is everything before the FIRST dot // or colon, so "con.tar.gz" and "nul:x.txt" are devices (a colon opens the // stream syntax, codex round 1), and trailing spaces in the stem are ignored, // so "con .txt" is one too. The match folds case. -func windowsDeviceName(name string) bool { +// +// Exported for BUG-3185: every name offered for download or written by the +// CLI that is derived from user data goes through this one rule. Do not write +// a second predicate; the stem rule has already been corrected once. +func WindowsDeviceName(name string) bool { stem := name if i := strings.IndexAny(name, ".:"); i >= 0 { stem = name[:i] diff --git a/internal/server/handlers_artifact_export.go b/internal/server/handlers_artifact_export.go index ffaf13d05..c3fece841 100644 --- a/internal/server/handlers_artifact_export.go +++ b/internal/server/handlers_artifact_export.go @@ -9,6 +9,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/PerpetualSoftware/pad/internal/artifact" + "github.com/PerpetualSoftware/pad/internal/attachments" "github.com/PerpetualSoftware/pad/internal/models" ) @@ -147,5 +148,12 @@ func (s *Server) handleExportItemArtifact(w http.ResponseWriter, r *http.Request // artifactExportFilename builds the download filename for an exported item: // ".pad.md". func artifactExportFilename(item *models.Item) string { - return item.Slug + ".pad.md" + name := item.Slug + ".pad.md" + // BUG-3185: the stem is the slug, so an item slugged "nul", "con", "com1" + // and so on names a Windows device, and a client saving the suggested name + // writes nowhere. Prefixed "_" as attachment names are (BUG-2822). + if attachments.WindowsDeviceName(name) { + return "_" + name + } + return name } diff --git a/internal/server/handlers_artifact_export_device_test.go b/internal/server/handlers_artifact_export_device_test.go new file mode 100644 index 000000000..3d1371bab --- /dev/null +++ b/internal/server/handlers_artifact_export_device_test.go @@ -0,0 +1,55 @@ +package server + +import ( + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3185: an export's suggested file name is ".pad.md", so an item +// slugged like a Windows device named a device. The name is prefixed "_", +// by the one device rule attachments use (BUG-2822). +func TestArtifactExportFilename_BUG3185_DeviceStemIsPrefixed(t *testing.T) { + cases := []struct{ slug, want string }{ + {"nul", "_nul.pad.md"}, + {"con", "_con.pad.md"}, + {"com1", "_com1.pad.md"}, + {"lpt9", "_lpt9.pad.md"}, + {"aux", "_aux.pad.md"}, + // Near misses: a longer stem is not a device. + {"nulls", "nulls.pad.md"}, + {"com10", "com10.pad.md"}, + {"con-x", "con-x.pad.md"}, + {"ship-it", "ship-it.pad.md"}, + } + for _, c := range cases { + if got := artifactExportFilename(&models.Item{Slug: c.slug}); got != c.want { + t.Errorf("slug %q: got %q, want %q", c.slug, got, c.want) + } + } +} + +// The binding leg: the export ENDPOINT carries the prefixed name in its +// Content-Disposition, which is what both clients save under. +func TestExportArtifact_BUG3185_DeviceSlugHeaderIsPrefixed(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + pb := createItem(t, srv, ws, "playbooks", map[string]interface{}{ + "title": "NUL", + "content": "## Steps\n\n1. Nothing\n", + "fields": `{"status":"active","trigger":"manual","scope":"all"}`, + }) + if pb.Slug != "nul" { + t.Fatalf("precondition: the title must slug to exactly %q to exercise the device rule, got %q", "nul", pb.Slug) + } + rr := doRequest(srv, "GET", "/api/v1/workspaces/"+ws+"/items/"+pb.Slug+"/export", nil) + if rr.Code != http.StatusOK { + t.Fatalf("export: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + cd := rr.Header().Get("Content-Disposition") + if !strings.Contains(cd, `filename="_nul.pad.md"`) { + t.Errorf("Content-Disposition %q does not name _nul.pad.md", cd) + } +}