Skip to content

Add Go fuzz targets for NewDestination URL parsing and LoadConfig YAML #48

Description

@dolph

Two functions in this codebase are textbook fuzz targets:

  • NewDestination(Url) — accepts arbitrary user-supplied URL strings, branches on scheme, parses port via strconv.Atoi, calls net.LookupPort, special-cases nats/icmp. The current table tests cover ~10 happy-path schemes and 2 negative cases. There is no coverage of: empty hostnames, IDN/punycode, ports <0, >65535, leading zeros, scheme casing, userinfo with : or @ inside the username/password, URLs with fragments, URLs longer than int max, control characters, percent-encoded bytes that decode to scheme separators.
  • LoadConfig(path) — reads YAML into map[string]string. Malformed YAML currently log.Fatalfs (process-killing), which means a CI fuzz run would expose any non-error abort path. A clean-error refactor + a FuzzLoadConfig would simultaneously fix the abort behavior and lock it in.

Go 1.22 (per go.mod) ships native fuzzing. No external dependency, no new tooling, ~20 lines per target:

func FuzzNewDestination(f *testing.F) {
    seeds := []string{"http://host", "https://host:443/p", "tcp://host:1", "icmp://h", "mysql://u:p@h:3306/db"}
    for _, s := range seeds { f.Add(s) }
    f.Fuzz(func(t *testing.T, in string) {
        d, err := NewDestination(Url{Label: "x", Url: in})
        if err == nil {
            // Invariants that must hold on success:
            if d.Host == "" { t.Fatalf("empty host: %q", in) }
            if d.Port < -1 || d.Port > 65535 { t.Fatalf("port %d out of range: %q", in, d.Port) }
            // UrlString must not panic and must not leak password for set passwords
            _ = d.UrlString()
        }
    })
}

Why ship this now, separately from "add more tests"

  1. Fuzzing finds the cases humans don't write. The known parser bugs (ParseDestinations silently drops the first URL when loaded from a config file #5, Statsd YAML config is silently ignored and corrupts URL parsing #6, IPv6 is silently dropped; IPv6-only destinations report success without being checked #10, Statsd EscapeTag is incomplete: newline/CR enables wire-protocol injection #14) are exactly the shape of bugs fuzzing exposes.
  2. Once the corpus is seeded, regressions stay caught — CI can run a 10s smoke fuzz on every push without adding meaningful wall time.
  3. The investment is small. Two functions, two FuzzXxx blocks, one CI step. The repo has zero fuzz targets today.

Test plan

  • Add FuzzNewDestination with seeds covering each scheme in current tests.
  • Refactor LoadConfig to return (*Config, error) instead of log.Fatalf (prerequisite for fuzzing; required anyway for testability).
  • Add FuzzLoadConfig with seeds covering the documented statsd_* keys plus URL keys.
  • Add a go test -fuzz=Fuzz -fuzztime=30s job to CI gated on a label or schedule, separate from the standard go test -race job.

Refs #24 (umbrella: untested modules). Refs #23 (CI lacks coverage tracking — fuzzing is a complement, not a substitute, but the CI plumbing overlaps).

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions