Reject a -H value with no colon

README.md

@@ -64,7 +64,7 @@ http-assert [flags] <URL>
 |------|-------|-------------|
 | `--request` | `-X` | HTTP method (default: GET) |
 | `--data` | `-d` | Request body data |
-| `--header` | `-H` | Set request headers (can be used multiple times) |
+| `--header` | `-H` | Set request headers as `name: value` (can be used multiple times) |
 | `--max-time` | `-m` | Request timeout in seconds (default: 20) |
 | `--insecure` | `-k` | Skip SSL certificate verification |
 | `--maphost` | | Map hostname:port to different destination |
@@ -74,6 +74,8 @@ http-assert [flags] <URL>
 | `--retry-delay` | | Delay between attempts (default: 1s) |
 | `--retry-max-time` | | Stop retrying after this long (default: no limit) |
 
+A `-H` value needs a colon. A bare name exits `71` rather than being sent as a header with an empty value, which is what `curl` reads as "remove this header" — so the two would have meant opposite things. Write `-H 'X-Foo:'` when an empty value is what you want.
+
 ### Assertion Options
 
 | Flag | Description |

e2e_assert_test.go

@@ -295,3 +295,86 @@ func TestE2EAssertBooleanNegation(t *testing.T) {
 		assertContains(t, r, "was given 2 times")
 	})
 }
+
+// TestE2EHeaderRequiresASeparator covers the -H values the CLI refuses.
+//
+// A name on its own used to be sent as a header with an empty value. curl reads
+// the same input as "remove this header", so a user reaching for that idiom got
+// the opposite of what they asked for, silently (#33).
+func TestE2EHeaderRequiresASeparator(t *testing.T) {
+	for _, tc := range []struct {
+		Name string
+		Arg  string
+		Diag string
+	}{
+		{
+			Name: "a bare name",
+			Arg:  "BareHeader",
+			Diag: `Invalid value for --header flag: "BareHeader" has no ':' separator`,
+		},
+		{
+			// The common typo, and the reason this is worth an error rather
+			// than a best guess.
+			Name: "a name with a value but no colon",
+			Arg:  "X-Api-Key abc123",
+			Diag: "has no ':' separator",
+		},
+		{
+			// A colon is present but there is nothing in front of it, which
+			// would put a nameless header on the wire.
+			Name: "a colon with no name",
+			Arg:  ": value",
+			Diag: `has no header name before the ':'`,
+		},
+	} {
+		t.Run(tc.Name, func(t *testing.T) {
+			r := run(t, nil, "-H", tc.Arg, "--assert-ok", url("/ok"))
+			assertExit(t, r, exitBadFlagVal)
+			assertContains(t, r, tc.Diag)
+		})
+	}
+
+	// An empty -H is refused too, but only alongside another one. pflag reads a
+	// string array back through its own string form, in which a lone empty
+	// value serialises to "[]" and parses back as no values at all -- so a
+	// solitary -H '' never reaches this program. Harmless (it asks for no
+	// header and gets none) and worth pinning, because the pair below proves
+	// the validation itself is not what lets the single case through.
+	t.Run("an empty value alongside another", func(t *testing.T) {
+		r := run(t, nil, "-H", "X-Ok: 1", "-H", "", "--assert-ok", url("/ok"))
+		assertExit(t, r, exitBadFlagVal)
+		assertContains(t, r, "has no ':' separator")
+	})
+
+	t.Run("a solitary empty value is dropped before it arrives", func(t *testing.T) {
+		assertExit(t, run(t, nil, "-H", "", "--assert-ok", url("/ok")), exitOK)
+	})
+
+	// The message names the fix, because "no separator" alone leaves the
+	// reader guessing whether an empty value is even expressible.
+	t.Run("the error says how to send an empty value", func(t *testing.T) {
+		r := run(t, nil, "-H", "X-Foo", "--assert-ok", url("/ok"))
+		assertContains(t, r, `write "X-Foo:" to send the header with an empty value`)
+	})
+
+	t.Run("and that spelling works", func(t *testing.T) {
+		r := run(t, nil, "-H", "X-Foo:", "--assert-body", `"X-Foo":\[""\]`, url("/echo"))
+		assertExit(t, r, exitOK)
+	})
+
+	// Ordinary headers are untouched.
+	t.Run("a normal header still works", func(t *testing.T) {
+		r := run(t, nil, "-H", "X-Probe: 1", "--assert-body", `"X-Probe":\["1"\]`, url("/echo"))
+		assertExit(t, r, exitOK)
+	})
+
+	// The parser is shared with the header assertions, where a bare name is
+	// meaningful. Validating inside it would have broken these.
+	t.Run("--assert-header* still accept a bare name", func(t *testing.T) {
+		for _, f := range []string{"--assert-header", "--assert-header-eq"} {
+			r := run(t, nil, f, "Content-Type", url("/ok"))
+			assertExit(t, r, exitOK)
+		}
+		assertExit(t, run(t, nil, "--assert-header-missing", "X-Absent", url("/ok")), exitOK)
+	})
+}

e2e_known_issues_test.go

@@ -130,23 +130,6 @@ func TestKnownIssue31MaxTimeAcceptsNonPositive(t *testing.T) {
 	}
 }
 
-// TestKnownIssue33BareHeaderSendsEmptyValue: a -H value with no colon parses to
-// an empty value and is sent as an empty-valued header.
-//
-// curl treats `-H 'X-Foo'` as "remove this header" and requires `-H 'X-Foo;'` to
-// send an empty one, so the divergence is real -- but the header is NOT dropped,
-// which is what #33 originally claimed. The issue text has been corrected.
-func TestKnownIssue33BareHeaderSendsEmptyValue(t *testing.T) {
-	characterizes(t, 33, "-H without a colon sends an empty-valued header rather than being rejected")
-
-	r := run(t, nil, "-H", "BareHeader", "--assert-body-eq", "never-matches", url("/echo"))
-	assertExit(t, r, exitRequestFail)
-
-	// Canonicalised to Bareheader, present, with an empty value.
-	assertContains(t, r, "Bareheader")
-	assertContains(t, r, `\"Bareheader\":[\"\"]`)
-}
-
 // TestKnownIssue34StdoutAlwaysEmpty: every byte goes to stderr, so redirecting
 // stdout captures nothing.
 func TestKnownIssue34StdoutAlwaysEmpty(t *testing.T) {

main.go

@@ -236,7 +236,7 @@ Compression:
 
 			vs, _ := cmd.Flags().GetStringArray("header")
 			for _, v := range vs {
-				name, value := parseHeaderLine(v)
+				name, value := mustParseRequestHeader(v)
 				req.Header.Add(name, value)
 			}
 			if err := c.Do(req, parseAssertionFlags(cmd)...); err != nil {
@@ -260,7 +260,8 @@ Compression:
 	cmd.PersistentFlags().IntP("max-time", "m", 20,
 		"Maximum time in seconds that you allow each request to take")
 	cmd.Flags().StringP("request", "X", "GET", "Set method for HTTP request")
-	cmd.Flags().StringArrayP("header", "H", nil, "Set header for HTTP request")
+	cmd.Flags().StringArrayP("header", "H", nil,
+		"Set header for HTTP request, as <name: value>; a name alone is rejected")
 	cmd.Flags().StringP("data", "d", "",
 		"Sends the specified data in a POST request to the HTTP server")
 	cmd.Flags().BoolP("location", "L", false,
@@ -454,6 +455,33 @@ func parseLogLevel(s string) (LogLevel, bool) {
 	}
 }
 
+// mustParseRequestHeader parses a -H value, refusing the two forms that would
+// put a header on the wire the caller did not describe.
+//
+// parseHeaderLine is shared with --assert-header*, where a name on its own is
+// meaningful: it asserts the header is present. A request has no such reading.
+// A name with no colon was sent as a header with an empty value, which is the
+// opposite of what the same input means to curl -- there it removes an
+// internally-generated header -- so a user reaching for that idiom got the one
+// outcome they were trying to avoid, silently (#33).
+//
+// The validation lives here rather than in the parser for that reason: the
+// parser is right for one caller and wrong for the other.
+func mustParseRequestHeader(v string) (name, value string) {
+	if !strings.Contains(v, ":") {
+		dief(71, "Invalid value for --header flag: %q has no ':' separator; "+
+			"write %q to send the header with an empty value", v, v+":")
+	}
+
+	name, value = parseHeaderLine(v)
+	if name == "" {
+		dief(71, "Invalid value for --header flag: %q has no header name "+
+			"before the ':'", v)
+	}
+
+	return name, value
+}
+
 func mustParseHostMappings(vals []string) []hostMapping {
 	res, err := parseHostMappings(vals)
 	if err != nil {