git » chasquid » commit 0e29fe4

diff --git a/internal/aliases/aliases.go b/internal/aliases/aliases.go
index 72666d0..505e2bb 100644
--- a/internal/aliases/aliases.go
+++ b/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...),
+	}
+}
diff --git a/internal/aliases/aliases_test.go b/internal/aliases/aliases_test.go
index 3854487..8ec6f7a 100644
--- a/internal/aliases/aliases_test.go
+++ b/internal/aliases/aliases_test.go
@@ -486,16 +486,14 @@ 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}}},
 		{"a : b \n", []Recipient{{"b@dom", EMAIL}}},
-		{"a : b, \n", []Recipient{{"b@dom", EMAIL}}},
+		{"a:b,\n", []Recipient{{"b@dom", EMAIL}}},
 
 		{"a: |cmd\n", []Recipient{{"cmd", PIPE}}},
 		{"a:|cmd\n", []Recipient{{"cmd", PIPE}}},
@@ -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) {
-	tr := trace.New("TestHookError", "test")
+func TestHook(t *testing.T) {
+	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)
 	}
diff --git a/internal/aliases/testdata/empty-hook.sh b/internal/aliases/testdata/empty-hook.sh
new file mode 100755
index 0000000..e4c2fac
--- /dev/null
+++ b/internal/aliases/testdata/empty-hook.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+# Hook that doesn't return any output.
+exit 0
diff --git a/internal/aliases/testdata/invalid-hook.sh b/internal/aliases/testdata/invalid-hook.sh
new file mode 100755
index 0000000..8c1cc21
--- /dev/null
+++ b/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 "|"
diff --git a/internal/aliases/testdata/normal-hook.sh b/internal/aliases/testdata/normal-hook.sh
new file mode 100755
index 0000000..30879e3
--- /dev/null
+++ b/internal/aliases/testdata/normal-hook.sh
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+echo "p@q, x@y"
diff --git a/test/t-04-aliases/.gitignore b/test/t-04-aliases/.gitignore
new file mode 100644
index 0000000..3dfe10f
--- /dev/null
+++ b/test/t-04-aliases/.gitignore
@@ -0,0 +1 @@
+config/hooks/alias-resolve