From a61e9ec78c295f302e33857e6bab20b7b013a028 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Thu, 6 Aug 2026 11:54:11 +1000 Subject: [PATCH 1/2] fix: always write a nuspec into generated NuGet packages The manifest was only generated when the caller supplied a description, title, release notes or author. Ask for nothing but an ID and a version and the command still reported success, but produced a .nupkg with no .nuspec in it: a valid zip, and not a valid NuGet package. Readers locate the manifest by looking for a single .nuspec in the archive root, so there was nothing for them to find. Generate one for every package. The nuspec schema requires description and authors alongside id and version, so both now default rather than being filled in only when the caller had already opted into metadata. The description default is the one the --description flag has always documented, and the author default is the current user, which is what the flag's own comment says it should be and what the old Octopus CLI did. Pass --author to override it. A base path that already contains {id}.nuspec is left alone and packed as-is. That file is the user's own: generating over the top would discard their metadata, and the cleanup step would then delete it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/package/nuget/create/create.go | 99 +++++++--- pkg/cmd/package/nuget/create/create_test.go | 199 ++++++++++++++++++++ 2 files changed, 268 insertions(+), 30 deletions(-) create mode 100644 pkg/cmd/package/nuget/create/create_test.go diff --git a/pkg/cmd/package/nuget/create/create.go b/pkg/cmd/package/nuget/create/create.go index 72d7c66f..ebc9481a 100644 --- a/pkg/cmd/package/nuget/create/create.go +++ b/pkg/cmd/package/nuget/create/create.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "os/user" "path/filepath" "strings" "time" @@ -27,6 +28,11 @@ const ( FlagReleaseNotesFile = "releaseNotesFile" ) +// DefaultDescription is used when the caller supplies no description. The nuspec +// schema requires the element, and this is what the flag help has always +// promised. +const DefaultDescription = "A deployment package created from files on disk." + type NuPkgCreateFlags struct { Author *flag.Flag[[]string] // this need to be multiple and default to current user Title *flag.Flag[string] @@ -112,23 +118,33 @@ func createRun(cmd *cobra.Command, opts *NuPkgCreateOptions) error { return err } - nuspecFilePath := "" - if shouldGenerateNuSpec(opts) { - defer func() { - if nuspecFilePath != "" { - err := os.Remove(nuspecFilePath) - if err != nil { - panic(err) - } - } - }() - nuspecFilePath, err = GenerateNuSpec(opts) + // Every .nupkg needs a manifest: readers locate it by looking for a single + // .nuspec in the archive root, so a package without one is not a valid NuGet + // package even though it is a perfectly good zip. Generate one unless the + // base path already supplies it, in which case that file is authoritative and + // must not be touched. + nuspecFileName := opts.Id.Value + ".nuspec" + suppliedNuSpec, err := hasSuppliedNuSpec(opts.BasePath.Value, nuspecFileName) + if err != nil { + return err + } + + if suppliedNuSpec { + pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Using existing nuspec file \"%s\"\n", nuspecFileName) + } else { + nuspecFilePath, err := GenerateNuSpec(opts) if err != nil { return err } - opts.Include.Value = append(opts.Include.Value, opts.Id.Value+".nuspec") + defer func() { + if err := os.Remove(nuspecFilePath); err != nil { + pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Could not remove generated nuspec file \"%s\": %v\n", nuspecFilePath, err) + } + }() } + opts.Include.Value = append(opts.Include.Value, nuspecFileName) + pack.VerboseOut(opts.Writer, opts.Verbose.Value, "Packing \"%s\" version \"%s\"...\n", opts.Id.Value, opts.Version.Value) outFilePath := pack.BuildOutFileName("nupkg", opts.Id.Value, opts.Version.Value) @@ -198,7 +214,7 @@ func PromptMissing(opts *NuPkgCreateOptions) error { if err := opts.Ask(&survey.Input{ Message: "Nuspec description", Help: "The description to include in the Nuspec file.", - Default: "A deployment package created from files on disk.", + Default: DefaultDescription, }, &opts.Description.Value); err != nil { return err } @@ -246,15 +262,38 @@ func applyDefaultsToUnspecifiedPackageOptions(opts *NuPkgCreateOptions) error { opts.Include.Value = append(opts.Include.Value, "**") } - if len(opts.Author.Value) > 0 { - if opts.Description.Value == "" { - opts.Description.Value = "A deployment package created from files on disk." - } + // A manifest is generated for every package now, so these two are no longer + // only relevant when the caller opted into metadata: the nuspec schema + // requires both, and leaving them out produces a package strict readers + // reject. + if opts.Description.Value == "" { + opts.Description.Value = DefaultDescription + } + + if util.Empty(opts.Author.Value) { + opts.Author.Value = []string{defaultAuthor(opts.Id.Value)} } return nil } +// defaultAuthor mirrors what the old Octopus CLI did, and what the Author flag +// has always said it should do. Where the user cannot be determined the package +// ID stands in: the element is required, and an obvious placeholder beats +// failing the command over metadata nobody asked for. +// currentUser is a seam: user lookup fails on some minimal container images, and +// the fallback needs to be reachable in a test. +var currentUser = user.Current + +func defaultAuthor(fallback string) string { + if current, err := currentUser(); err == nil { + if name := strings.TrimSpace(current.Username); name != "" { + return name + } + } + return fallback +} + func getReleaseNotesFromFile(filePath string) (string, error) { _, err := os.Stat(filePath) if err != nil { @@ -269,23 +308,23 @@ func getReleaseNotesFromFile(filePath string) (string, error) { return string(notes), nil } -func shouldGenerateNuSpec(opts *NuPkgCreateOptions) bool { - return opts.Description.Value != "" || - opts.Title.Value != "" || - opts.ReleaseNotes.Value != "" || - opts.ReleaseNotesFile.Value != "" || - !util.Empty(opts.Author.Value) +// hasSuppliedNuSpec reports whether the base path already contains a manifest +// for this package. One written by hand is the user's own file: it is theirs to +// keep, and generating over the top of it would both discard their metadata and +// delete the file on the way out. +func hasSuppliedNuSpec(basePath string, nuspecFileName string) (bool, error) { + _, err := os.Stat(filepath.Join(basePath, nuspecFileName)) + if err == nil { + return true, nil + } + if os.IsNotExist(err) { + return false, nil + } + return false, err } func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) { - if opts.Description.Value == "" { - return "", errors.New("description is required when generating nuspec metadata") - } - if len(opts.Author.Value) == 0 { - return "", errors.New("at least one author is required when generating nuspec metadata") - } - releaseNotes := opts.ReleaseNotes.Value if opts.ReleaseNotesFile.Value != "" { if releaseNotes != "" { diff --git a/pkg/cmd/package/nuget/create/create_test.go b/pkg/cmd/package/nuget/create/create_test.go new file mode 100644 index 00000000..a23fd8dc --- /dev/null +++ b/pkg/cmd/package/nuget/create/create_test.go @@ -0,0 +1,199 @@ +package create + +import ( + "encoding/xml" + "errors" + "os" + "os/user" + "path/filepath" + "strings" + "testing" + + pack "github.com/OctopusDeploy/cli/pkg/cmd/package/support" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// nuspec is the subset of the manifest these tests care about, used to assert +// against parsed XML rather than raw substrings. +type nuspec struct { + Metadata struct { + Id string `xml:"id"` + Version string `xml:"version"` + Title string `xml:"title"` + Description string `xml:"description"` + Authors string `xml:"authors"` + ReleaseNotes string `xml:"releaseNotes"` + } `xml:"metadata"` +} + +func newTestOptions(t *testing.T, basePath string) *NuPkgCreateOptions { + t.Helper() + + opts := &NuPkgCreateOptions{ + NuPkgCreateFlags: NewNuPkgCreateFlags(), + PackageCreateOptions: &pack.PackageCreateOptions{PackageCreateFlags: pack.NewPackageCreateFlags()}, + } + opts.Id.Value = "Acme.Web" + opts.Version.Value = "1.2.3" + opts.BasePath.Value = basePath + return opts +} + +func generateAndParse(t *testing.T, opts *NuPkgCreateOptions) (nuspec, string) { + t.Helper() + + path, err := GenerateNuSpec(opts) + require.NoError(t, err) + + contents, err := os.ReadFile(path) + require.NoError(t, err) + + var parsed nuspec + require.NoError(t, xml.Unmarshal(contents, &parsed), "generated nuspec should be well-formed XML") + + return parsed, string(contents) +} + +// The manifest is what makes a .nupkg a NuGet package rather than a zip, and +// the OPC relationships part points at it by name whether or not it exists. +func TestGeneratesNuSpecWithNoMetadataSupplied(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + parsed, _ := generateAndParse(t, opts) + + assert.Equal(t, "Acme.Web", parsed.Metadata.Id) + assert.Equal(t, "1.2.3", parsed.Metadata.Version) + assert.NotEmpty(t, parsed.Metadata.Description, "description is required by the nuspec schema") + assert.NotEmpty(t, parsed.Metadata.Authors, "authors is required by the nuspec schema") +} + +func TestDescriptionDefaultsWhenNotSupplied(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + assert.Equal(t, DefaultDescription, opts.Description.Value) +} + +func TestSuppliedDescriptionIsKept(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + opts.Description.Value = "Something specific" + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + assert.Equal(t, "Something specific", opts.Description.Value) +} + +func TestAuthorDefaultsToCurrentUser(t *testing.T) { + current, err := user.Current() + require.NoError(t, err) + + opts := newTestOptions(t, t.TempDir()) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + assert.Equal(t, []string{current.Username}, opts.Author.Value) +} + +func TestSuppliedAuthorsAreKept(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + opts.Author.Value = []string{"Ada", "Grace"} + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + parsed, _ := generateAndParse(t, opts) + + assert.Equal(t, []string{"Ada", "Grace"}, opts.Author.Value) + assert.Equal(t, "Ada,Grace", parsed.Metadata.Authors) +} + +// User lookup fails on some minimal container images. The command should still +// produce a schema-valid manifest rather than failing over metadata nobody asked +// for. +func TestDefaultAuthorFallsBackWhenUserIsUnknown(t *testing.T) { + original := currentUser + t.Cleanup(func() { currentUser = original }) + currentUser = func() (*user.User, error) { return nil, errors.New("no user") } + + assert.Equal(t, "Acme.Web", defaultAuthor("Acme.Web")) +} + +func TestDefaultAuthorFallsBackWhenUsernameIsBlank(t *testing.T) { + original := currentUser + t.Cleanup(func() { currentUser = original }) + currentUser = func() (*user.User, error) { return &user.User{Username: " "}, nil } + + assert.Equal(t, "Acme.Web", defaultAuthor("Acme.Web")) +} + +func TestPackageIsStillUsableWhenUserLookupFails(t *testing.T) { + original := currentUser + t.Cleanup(func() { currentUser = original }) + currentUser = func() (*user.User, error) { return nil, errors.New("no user") } + + opts := newTestOptions(t, t.TempDir()) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + parsed, _ := generateAndParse(t, opts) + + assert.Equal(t, "Acme.Web", parsed.Metadata.Authors) +} + +// A hand-written manifest is the user's own file. Generating over the top would +// discard their metadata, and the cleanup step would then delete it outright. +func TestExistingNuSpecIsDetected(t *testing.T) { + basePath := t.TempDir() + existing := filepath.Join(basePath, "Acme.Web.nuspec") + require.NoError(t, os.WriteFile(existing, []byte("Acme.Web"), 0644)) + + supplied, err := hasSuppliedNuSpec(basePath, "Acme.Web.nuspec") + + require.NoError(t, err) + assert.True(t, supplied) +} + +func TestMissingNuSpecIsNotMistakenForOne(t *testing.T) { + supplied, err := hasSuppliedNuSpec(t.TempDir(), "Acme.Web.nuspec") + + require.NoError(t, err) + assert.False(t, supplied) +} + +func TestTitleIsOmittedWhenNotSupplied(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + _, raw := generateAndParse(t, opts) + + assert.NotContains(t, raw, "") +} + +func TestReleaseNotesAreIncludedWhenSupplied(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + opts.ReleaseNotes.Value = "Fixed a thing" + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + parsed, _ := generateAndParse(t, opts) + + assert.Equal(t, "Fixed a thing", parsed.Metadata.ReleaseNotes) +} + +func TestReleaseNotesAndReleaseNotesFileAreMutuallyExclusive(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + opts.ReleaseNotes.Value = "Fixed a thing" + opts.ReleaseNotesFile.Value = "notes.txt" + + _, err := GenerateNuSpec(opts) + + assert.ErrorContains(t, err, "cannot specify both") +} + +func TestNuSpecIsWrittenIntoTheBasePath(t *testing.T) { + basePath := t.TempDir() + opts := newTestOptions(t, basePath) + require.NoError(t, applyDefaultsToUnspecifiedPackageOptions(opts)) + + path, err := GenerateNuSpec(opts) + + require.NoError(t, err) + assert.Equal(t, filepath.Join(basePath, "Acme.Web.nuspec"), path) + assert.True(t, strings.HasSuffix(path, ".nuspec")) +} From 5d04abfaeeb238294d9e4660a5e992500a792c41 Mon Sep 17 00:00:00 2001 From: Nick Josevski <nick.josevski@octopus.com> Date: Thu, 6 Aug 2026 11:54:26 +1000 Subject: [PATCH 2/2] fix: XML-escape values written into the nuspec The manifest is assembled by string concatenation, so an ampersand or angle bracket in a description, title or release notes produced a document no XML parser would accept. Release notes in particular are free text, and "fixed A & B" is enough to do it. This was survivable while the nuspec was only written for callers who had opted into metadata. Now that one is generated for every package it is on the path everyone takes, so escape each value on the way in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- pkg/cmd/package/nuget/create/create.go | 26 ++++++++++++++++----- pkg/cmd/package/nuget/create/create_test.go | 22 +++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/package/nuget/create/create.go b/pkg/cmd/package/nuget/create/create.go index ebc9481a..bc529fee 100644 --- a/pkg/cmd/package/nuget/create/create.go +++ b/pkg/cmd/package/nuget/create/create.go @@ -1,6 +1,8 @@ package create import ( + "bytes" + "encoding/xml" "errors" "fmt" "os" @@ -308,6 +310,18 @@ func getReleaseNotesFromFile(filePath string) (string, error) { return string(notes), nil } +// escapeXML makes a value safe to drop between two tags. Release notes and +// descriptions are free text, and an unescaped ampersand or angle bracket is +// enough to make the whole manifest unparseable. +func escapeXML(value string) string { + var buf bytes.Buffer + if err := xml.EscapeText(&buf, []byte(value)); err != nil { + // EscapeText only fails if the writer fails, and bytes.Buffer does not. + return value + } + return buf.String() +} + // hasSuppliedNuSpec reports whether the base path already contains a manifest // for this package. One written by hand is the user's own file: it is theirs to // keep, and generating over the top of it would both discard their metadata and @@ -344,15 +358,15 @@ func GenerateNuSpec(opts *NuPkgCreateOptions) (string, error) { sb.WriteString(`<?xml version="1.0" encoding="utf-8"?>` + "\n") sb.WriteString(`<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">` + "\n") sb.WriteString(" <metadata>\n") - sb.WriteString(" <id>" + opts.Id.Value + "</id>\n") - sb.WriteString(" <version>" + opts.Version.Value + "</version>\n") + sb.WriteString(" <id>" + escapeXML(opts.Id.Value) + "</id>\n") + sb.WriteString(" <version>" + escapeXML(opts.Version.Value) + "</version>\n") if opts.Title.Value != "" { - sb.WriteString(" <title>" + opts.Title.Value + "\n") + sb.WriteString(" " + escapeXML(opts.Title.Value) + "\n") } - sb.WriteString(" " + opts.Description.Value + "\n") - sb.WriteString(" " + strings.Join(opts.Author.Value, ",") + "\n") + sb.WriteString(" " + escapeXML(opts.Description.Value) + "\n") + sb.WriteString(" " + escapeXML(strings.Join(opts.Author.Value, ",")) + "\n") if releaseNotes != "" { - sb.WriteString(" " + releaseNotes + "\n") + sb.WriteString(" " + escapeXML(releaseNotes) + "\n") } sb.WriteString(" \n") sb.WriteString("\n") diff --git a/pkg/cmd/package/nuget/create/create_test.go b/pkg/cmd/package/nuget/create/create_test.go index a23fd8dc..d535a5f0 100644 --- a/pkg/cmd/package/nuget/create/create_test.go +++ b/pkg/cmd/package/nuget/create/create_test.go @@ -186,6 +186,28 @@ func TestReleaseNotesAndReleaseNotesFileAreMutuallyExclusive(t *testing.T) { assert.ErrorContains(t, err, "cannot specify both") } +// Descriptions and release notes are free text. An unescaped ampersand or angle +// bracket is enough to make the whole manifest unparseable, which matters more +// now that one is written for every package. +func TestMetadataIsXmlEscaped(t *testing.T) { + opts := newTestOptions(t, t.TempDir()) + opts.Description.Value = `Fish & chips bold` + opts.Title.Value = `A "quoted" title` + opts.ReleaseNotes.Value = `1 < 2 && 3 > 2` + opts.Author.Value = []string{"Ada & Grace"} + + parsed, raw := generateAndParse(t, opts) + + assert.NotContains(t, raw, "Fish & chips", "raw ampersand should have been escaped") + assert.Contains(t, raw, "&") + + // Round-tripping is the real assertion: what went in is what comes back out. + assert.Equal(t, `Fish & chips bold`, parsed.Metadata.Description) + assert.Equal(t, `A "quoted" title`, parsed.Metadata.Title) + assert.Equal(t, `1 < 2 && 3 > 2`, parsed.Metadata.ReleaseNotes) + assert.Equal(t, "Ada & Grace", parsed.Metadata.Authors) +} + func TestNuSpecIsWrittenIntoTheBasePath(t *testing.T) { basePath := t.TempDir() opts := newTestOptions(t, basePath)