aliases: Return error on invalid lines

Today, aliases parsing is too lax, silently ignoring most kinds of invalid
lines.

That behaviour can cause a lot of confusion when users think the aliases
are being parsed, and also cause problems when extending the syntax.

This patch fixes that problem by making aliases parsing return errors on
the invalid lines.

Unfortunately this will cause some previously-accepted files to now be
rejected, but it should be quite visible.

internal/aliases/aliases.go

@@ -399,7 +399,7 @@ func (v *Resolver) Reload() error {
 				continue
 			}
 			if err != nil {
-				return fmt.Errorf("error parsing %q: %v", path, err)
+				return err
 			}
 
 			// Add the aliases to the resolver, overriding any previous values.
@@ -425,7 +425,7 @@ func (v *Resolver) parseFile(domain, path string) (map[string][]Recipient, error
 
 	aliases, err := v.parseReader(domain, f)
 	if err != nil {
-		return nil, fmt.Errorf("reading %q: %v", path, err)
+		return nil, fmt.Errorf("%q %w", path, err)
 	}
 	return aliases, nil
 }
@@ -436,23 +436,23 @@ func (v *Resolver) parseReader(domain string, r io.Reader) (map[string][]Recipie
 	scanner := bufio.NewScanner(r)
 	for i := 1; scanner.Scan(); i++ {
 		line := strings.TrimSpace(scanner.Text())
-		if strings.HasPrefix(line, "#") {
+		if line == "" || strings.HasPrefix(line, "#") {
 			continue
 		}
 
 		sp := strings.SplitN(line, ":", 2)
 		if len(sp) != 2 {
-			continue
+			return nil, newParseError(i, "missing ':' in line")
 		}
 
 		addr, rawalias := strings.TrimSpace(sp[0]), strings.TrimSpace(sp[1])
 		if len(addr) == 0 || len(rawalias) == 0 {
-			continue
+			return nil, newParseError(i, "missing address or alias")
 		}
 
 		if strings.Contains(addr, "@") {
 			// It's invalid for lhs addresses to contain @ (for now).
-			continue
+			return nil, newParseError(i, "left-side: cannot contain @")
 		}
 
 		// We remove DropChars from the address, but leave the suffixes (if
@@ -462,41 +462,53 @@ func (v *Resolver) parseReader(domain string, r io.Reader) (map[string][]Recipie
 		addr = v.RemoveDropCharacters(addr)
 		addr, _ = normalize.Addr(addr)
 
-		rs := parseRHS(rawalias, domain)
+		rs, err := parseRHS(rawalias, domain)
+		if err != nil {
+			return nil, newParseError(i, "right-side: %w", err)
+		}
 		aliases[addr] = rs
 	}
 
 	return aliases, scanner.Err()
 }
 
-func parseRHS(rawalias, domain string) []Recipient {
+func parseRHS(rawalias, domain string) ([]Recipient, error) {
 	if len(rawalias) == 0 {
-		return nil
+		// Explicitly allow empty rawalias strings at this point: the file
+		// parsing will prevent this at the upper level, and when we parse the
+		// RHS for the hook invocation, an empty string is valid.
+		return nil, nil
 	}
 	if rawalias[0] == '|' {
 		cmd := strings.TrimSpace(rawalias[1:])
 		if cmd == "" {
 			// A pipe alias without a command is invalid.
-			return nil
+			return nil, fmt.Errorf("the pipe alias is missing a command")
 		}
-		return []Recipient{{cmd, PIPE}}
+		return []Recipient{{cmd, PIPE}}, nil
 	}
 
 	rs := []Recipient{}
 	for _, a := range strings.Split(rawalias, ",") {
 		a = strings.TrimSpace(a)
 		if a == "" {
+			// Ignore empty aliases. This is out of convenience, so we allow
+			// things like "a: b,".
 			continue
 		}
+
 		// Addresses with no domain get the current one added, so it's
 		// easier to share alias files.
 		if !strings.Contains(a, "@") {
 			a = a + "@" + domain
 		}
-		a, _ = normalize.Addr(a)
+		a, err := normalize.Addr(a)
+		if err != nil {
+			return nil, fmt.Errorf("normalizing address %q: %w", a, err)
+		}
 		rs = append(rs, Recipient{a, EMAIL})
 	}
-	return rs
+	return rs, nil
 }
 
 // removeAllAfter removes everything from s that comes after the separators,
@@ -559,9 +571,30 @@ func (v *Resolver) runResolveHook(tr *trace.Trace, addr string) ([]Recipient, er
 	// Same format as the right hand side of aliases file, see parseRHS.
 	domain := envelope.DomainOf(addr)
 	raw := strings.TrimSpace(out)
-	rs := parseRHS(raw, domain)
+	rs, err := parseRHS(raw, domain)
+	if err != nil {
+		hookResults.Add("resolve:parse_err", 1)
+		tr.Errorf("error parsing hook output: %v", err)
+		return nil, err
+	}
 
 	tr.Debugf("recipients: %v", rs)
 	hookResults.Add("resolve:success", 1)
 	return rs, nil
 }
+
+type parseError struct {
+	line int
+	err  error
+}
+
+func (e *parseError) Error() string {
+	return fmt.Sprintf("line %d: %v", e.line, e.err)
+}
+
+func newParseError(line int, f string, args ...interface{}) error {
+	return &parseError{
+		line: line,
+		err:  fmt.Errorf(f, args...),
+	}
+}

internal/aliases/aliases_test.go

@@ -486,11 +486,9 @@ func TestAddFile(t *testing.T) {
 		contents string
 		expected []Recipient
 	}{
-		{"\n", []Recipient{{"a@dom", EMAIL}}},
+		{"", []Recipient{{"a@dom", EMAIL}}},
+		{"\n\n", []Recipient{{"a@dom", EMAIL}}},
 		{" # Comment\n", []Recipient{{"a@dom", EMAIL}}},
-		{":\n", []Recipient{{"a@dom", EMAIL}}},
-		{"a: \n", []Recipient{{"a@dom", EMAIL}}},
-		{"a@dom: b@c \n", []Recipient{{"a@dom", EMAIL}}},
 
 		{"a: b\n", []Recipient{{"b@dom", EMAIL}}},
 		{"a:b\n", []Recipient{{"b@dom", EMAIL}}},
@@ -505,10 +503,6 @@ func TestAddFile(t *testing.T) {
 
 		{"a: c@d, e@f, g\n",
 			[]Recipient{{"c@d", EMAIL}, {"e@f", EMAIL}, {"g@dom", EMAIL}}},
-
-		// Invalid pipe aliases, should be ignored.
-		{"a:|\n", []Recipient{{"a@dom", EMAIL}}},
-		{"a:| \n", []Recipient{{"a@dom", EMAIL}}},
 	}
 
 	tr := trace.New("test", "TestAddFile")
@@ -521,7 +515,7 @@ func TestAddFile(t *testing.T) {
 		resolver := NewResolver(allUsersExist)
 		_, err := resolver.AddAliasesFile("dom", fname)
 		if err != nil {
-			t.Fatalf("error adding file: %v", err)
+			t.Fatalf("case %q, error adding file: %v", c.contents, err)
 		}
 
 		got, err := resolver.Resolve(tr, "a@dom")
@@ -533,6 +527,35 @@ func TestAddFile(t *testing.T) {
 			t.Errorf("case %q, got %v, expected %v", c.contents, got, c.expected)
 		}
 	}
+
+	// Error cases.
+	errcases := []struct {
+		contents string
+		expected string
+	}{
+		{":\n", "line 1: missing address or alias"},
+		{"a: \n", "line 1: missing address or alias"},
+		{"a:|\n", "right-side: the pipe alias is missing a command"},
+		{"a:| \n", "right-side: the pipe alias is missing a command"},
+		{"a@dom: b@c \n", "left-side: cannot contain @"},
+		{"a", "line 1: missing ':' in line"},
+		{"a: x y z\n", "disallowed rune encountered"},
+	}
+
+	for _, c := range errcases {
+		fname := mustWriteFile(t, c.contents)
+		defer os.Remove(fname)
+
+		resolver := NewResolver(allUsersExist)
+		_, err := resolver.AddAliasesFile("dom", fname)
+		if err == nil {
+			t.Errorf("case %q, expected error, got nil (aliases: %v)",
+				c.contents, resolver.aliases)
+		} else if !strings.Contains(err.Error(), c.expected) {
+			t.Errorf("case %q, got error %q, expected it to contain %q",
+				c.contents, err.Error(), c.expected)
+		}
+	}
 }
 
 const richFileContents = `
@@ -544,9 +567,6 @@ a: b
 c: d@e, f,
 x: | command
 
-# The following is invalid, should be ignored.
-a@dom: x@dom
-
 # Overrides.
 o1: a
 o1: b
@@ -651,12 +671,24 @@ func TestManyFiles(t *testing.T) {
 	if err := resolver.Reload(); err != nil {
 		t.Fatalf("failed to reload: %v", err)
 	}
+	check()
 
+	// Make one of the files invalid, then reload. Reload should return an
+	// error, and leave the resolver unchanged.
+	if err := os.WriteFile(files["d1"], []byte("invalid\n"), 0644); err != nil {
+		t.Fatalf("failed to write file: %v", err)
+	}
+	err := resolver.Reload()
+	if err == nil {
+		t.Fatalf("expected error, got nil")
+	} else if !strings.Contains(err.Error(), "line 1: missing ':' in line") {
+		t.Fatalf("expected error to contain 'missing :', got %v", err)
+	}
 	check()
 }
 
-func TestHookError(t *testing.T) {
+func TestHook(t *testing.T) {
-	tr := trace.New("TestHookError", "test")
+	tr := trace.New("TestHook", "test")
 	defer tr.Finish()
 
 	resolver := NewResolver(allUsersExist)
@@ -671,12 +703,49 @@ func TestHookError(t *testing.T) {
 		{"a@localA", []Recipient{{"c@d", EMAIL}}, nil},
 	}.check(t, resolver)
 
+	// Test that the empty hook is run correctly.
+	resolver.ResolveHook = "testdata/empty-hook.sh"
+	mustExist(t, resolver, "a@localA")
+	Cases{
+		{"a@localA", []Recipient{{"c@d", EMAIL}}, nil},
+	}.check(t, resolver)
+
+	// Test that a normal hook is run correctly.
+	resolver.ResolveHook = "testdata/normal-hook.sh"
+	mustExist(t, resolver, "a@localA")
+	Cases{
+		{"a@localA", []Recipient{
+			{"c@d", EMAIL}, // From the internal aliases.
+			{"p@q", EMAIL}, // From the hook.
+			{"x@y", EMAIL}, // From the hook.
+		}, nil},
+	}.check(t, resolver)
+
+	// Test that a non-existent hook is ignored.
+	resolver.ResolveHook = "testdata/doesnotexist"
+	mustExist(t, resolver, "a@localA")
+	Cases{
+		{"a@localA", []Recipient{{"c@d", EMAIL}}, nil},
+	}.check(t, resolver)
+
+	// Test a hook that returns an invalid alias.
+	resolver.ResolveHook = "testdata/invalid-hook.sh"
+	mustNotExist(t, resolver, "a@localA")
+
+	rcpts, err := resolver.Resolve(tr, "a@localA")
+	if len(rcpts) != 0 {
+		t.Errorf("expected no recipients, got %v", rcpts)
+	}
+	if !strings.Contains(err.Error(), "the pipe alias is missing a command") {
+		t.Errorf("expected 'the pipe alias is missing a command', got: %v", err)
+	}
+
 	// Now use a resolver that exits with an error.
 	resolver.ResolveHook = "testdata/erroring-hook.sh"
 
 	// Check that the hook is run and the error is propagated.
 	mustNotExist(t, resolver, "a@localA")
-	rcpts, err := resolver.Resolve(tr, "a@localA")
+	rcpts, err = resolver.Resolve(tr, "a@localA")
 	if len(rcpts) != 0 {
 		t.Errorf("expected no recipients, got %v", rcpts)
 	}

internal/aliases/testdata/empty-hook.sh

@@ -0,0 +1,4 @@
+#!/bin/sh
+
+# Hook that doesn't return any output.
+exit 0

internal/aliases/testdata/invalid-hook.sh

@@ -0,0 +1,4 @@
+#!/bin/sh
+
+# This is an invalid right-side line: the pipe is missing a command.
+echo "|"

internal/aliases/testdata/normal-hook.sh

@@ -0,0 +1,3 @@
+#!/bin/sh
+
+echo "p@q, x@y"

test/t-04-aliases/.gitignore

@@ -0,0 +1 @@
+config/hooks/alias-resolve