diff --git a/README.md b/README.md index 1d9b2d6..dbf03a8 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,7 @@ http-assert [flags] |------|-------|-------------| | `--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] | `--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 | diff --git a/e2e_assert_test.go b/e2e_assert_test.go index 45246cf..509fce9 100644 --- a/e2e_assert_test.go +++ b/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) + }) +} diff --git a/e2e_known_issues_test.go b/e2e_known_issues_test.go index 5a9773a..f75e253 100644 --- a/e2e_known_issues_test.go +++ b/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) { diff --git a/main.go b/main.go index 230ddce..3c63b64 100644 --- a/main.go +++ b/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 ; 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 {