Skip to content

Commit

Permalink
[BED-294] More descriptive error message when macros values are missi…
Browse files Browse the repository at this point in the history 
…ng (#18)

Error message now returns which macros are missing
  • Loading branch information
@csucu
csucu authored Jul 31, 2023
2 parents defbea5 + 730da5e commit 9b9eacf
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 32 deletions.
71 changes: 52 additions & 19 deletions macro.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,42 +19,43 @@ const (
)

type macro struct {
start int
pos int
prev int
length int
input string
output []string
state stateFn
exp bool
pctPos int
start int
pos int
prev int
length int
input string
missingMacros []string
output []string
state stateFn
exp bool
pctPos int
}

func newMacro(input string, exp bool) *macro {
return &macro{0, 0, 0, len(input), input, make([]string, 0), nil, exp, 0}
return &macro{0, 0, 0, len(input), input, make([]string, 0), make([]string, 0), nil, exp, 0}
}

type stateFn func(*macro, *parser) (stateFn, error)

// parseMacro evaluates whole input string and replaces keywords with appropriate
// values from
func parseMacro(p *parser, input string, exp bool) (string, error) {
// values from. It also returns any macros that were expected by not found
func parseMacro(p *parser, input string, exp bool) (string, []string, error) {
m := newMacro(input, exp)
var err error
for m.state = scanText; m.state != nil; {
m.state, err = m.state(m, p)
if err != nil {
// log error
return "", err
return "", nil, err
}

}
return strings.Join(m.output, ""), nil
return strings.Join(m.output, ""), m.missingMacros, nil
}

// parseMacroToken evaluates whole input string and replaces keywords with appropriate
// values from
func parseMacroToken(p *parser, t *token) (string, error) {
// values from. It also returns any macros that were expected by not found
func parseMacroToken(p *parser, t *token) (string, []string, error) {
return parseMacro(p, t.value, false)
}

Expand Down Expand Up @@ -238,6 +239,7 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
// var err error
var result string
var email *addrSpec
var missingMacro string

switch r {
case 's', 'S':
Expand All @@ -247,7 +249,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}

if result == "" {
missingMacro = "sender {s}"
}
case 'l', 'L':
email = parseAddrSpec(p.sender, p.sender)
curItem = item{email.local, negative, delimiter, false}
Expand All @@ -256,6 +260,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "local-part of <sender> {l}"
}

case 'o', 'O':
email = parseAddrSpec(p.sender, p.sender)
Expand All @@ -265,6 +272,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "domain of <sender> {o}"
}

case 'h', 'H':
curItem = item{removeRoot(p.heloDomain), negative, delimiter, false}
Expand All @@ -273,6 +283,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "heloDomain {h}"
}

case 'd', 'D':
curItem = item{removeRoot(p.domain), negative, delimiter, false}
Expand All @@ -281,14 +294,19 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}

if result == "" {
missingMacro = "domain {d}"
}
case 'i', 'I':
curItem = item{toDottedHex(p.ip, false), negative, delimiter, false}
m.moveon()
result, err = parseDelimiter(m, &curItem)
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "ip {i}"
}

case 'p', 'P':
// let's not use it for the moment, RFC doesn't recommend it.
Expand All @@ -311,7 +329,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}

if result == "" || result == "<nil>" {
missingMacro = "SMTP client IP {c}"
}
case 'r', 'R':
if !m.exp {
return errInvalidMacroSyntax(errors.New(`'r' macro letter allowed only in "exp" text`))
Expand All @@ -322,6 +342,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "receivingDomain {r}"
}

case 't', 'T':
if !m.exp {
Expand All @@ -333,6 +356,9 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
if err != nil {
return errInvalidMacroSyntax(err)
}
if result == "" {
missingMacro = "current timestamp {t}"
}
}

r, err = m.next()
Expand All @@ -345,6 +371,7 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
}

m.collect(result)
m.collectMissingMacros(missingMacro)
m.moveon()

m.moveon()
Expand All @@ -354,6 +381,12 @@ func scanMacro(m *macro, p *parser) (stateFn, error) {
func (m *macro) collect(result string) {
m.output = append(m.output, result)
}
func (m *macro) collectMissingMacros(macro string) {
if macro == "" {
return
}
m.missingMacros = append(m.missingMacros, macro)
}

func (m *macro) collectMacroBody() {
m.output = append(m.output, m.input[m.pctPos:m.pos])
Expand Down
53 changes: 48 additions & 5 deletions macro_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ package spf

import (
"fmt"
. "github.com/redsift/spf/v2/testing"
"net"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/miekg/dns"
. "github.com/redsift/spf/v2/testing"
)

const (
Expand Down Expand Up @@ -64,7 +65,7 @@ func TestMacroIteration(t *testing.T) {
continue
}
t.Run(fmt.Sprintf("%d_%s", no, test.domain), func(t *testing.T) {
got, err := parseMacroToken(
got, _, err := parseMacroToken(
newParser(WithResolver(testResolver)).with(stub, test.sender, test.domain, test.addr),
&token{mechanism: tExp, qualifier: qMinus, value: test.macro})
if err != nil {
Expand Down Expand Up @@ -134,7 +135,7 @@ func TestMacroExpansionRFCExamples(t *testing.T) {
for _, test := range testCases {

tkn.value = test.Input
result, err := parseMacroToken(parser, tkn)
result, _, err := parseMacroToken(parser, tkn)
if err != nil {
t.Errorf("Macro %s evaluation failed due to returned error: %v\n",
test.Input, err)
Expand Down Expand Up @@ -164,7 +165,7 @@ func TestMacroExpansion_partial(t *testing.T) {
for _, test := range testCases {

tkn.value = test.Input
result, err := parseMacroToken(parser, tkn)
result, _, err := parseMacroToken(parser, tkn)
if err != nil {
t.Errorf("Macro %s evaluation failed due to returned error: %v\n",
test.Input, err)
Expand Down Expand Up @@ -204,7 +205,7 @@ func TestParsingErrors(t *testing.T) {
for _, test := range testcases {

tkn.value = test.Input
result, err := parseMacroToken(parser, tkn)
result, _, err := parseMacroToken(parser, tkn)

if result != "" {
t.Errorf("For input '%s' expected empty result, got '%s' instead\n",
Expand Down Expand Up @@ -335,3 +336,45 @@ func TestMacro_Domains(t *testing.T) {
})
}
}

func TestMissingMacros(t *testing.T) {
testcases := []struct {
sender string
receivingFQDN string
helo string
ip net.IP

Input string
Output string
MissingMacros []string
}{
{sender, "example.com", "", ip4, "%{h}", "", []string{"heloDomain {h}"}},
{"", "example.com", "example.com", ip4, "%{s}", "", []string{"sender {s}"}},
{sender, "example.com", "example.com", net.IP{}, "%{c}", "", []string{"SMTP client IP {c}"}},
{"", "example.com", "", net.IP{}, "%{h}.%{s}.%{c}", "", []string{"heloDomain {h}", "sender {s}", "SMTP client IP {c}"}},
}

for _, test := range testcases {

tkn.value = test.Input

opts := []Option{WithResolver(testResolver)}
if test.helo != "" {
opts = append(opts, HeloDomain(test.helo))
}
if test.receivingFQDN != "" {
opts = append(opts, ReceivingFQDN(test.receivingFQDN))
}
parser := newParser(opts...).with(stub, test.sender, domain, test.ip)

_, missingMacros, err := parseMacro(parser, tkn.value, true)

if !cmp.Equal(test.MissingMacros, missingMacros) {
t.Errorf("String slices do not match. Diff: %v", cmp.Diff(test.MissingMacros, missingMacros))
}

if err != nil {
t.Errorf("For input '%s', expected empty err, got %v instead", test.Input, err)
}
}
}
24 changes: 16 additions & 8 deletions parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ func (p *parser) parseIP6(t *token) (bool, Result, error) {
func (p *parser) parseA(t *token) (bool, Result, time.Duration, error) {
fqdn, ip4Mask, ip6Mask, err := splitDomainDualCIDR(domainSpec(t.value, p.domain))
if err == nil {
fqdn, err = parseMacro(p, fqdn, false)
fqdn, _, err = parseMacro(p, fqdn, false)
}
if err == nil {
fqdn, err = truncateFQDN(fqdn)
Expand Down Expand Up @@ -495,7 +495,7 @@ func (p *parser) parseA(t *token) (bool, Result, time.Duration, error) {
func (p *parser) parseMX(t *token) (bool, Result, time.Duration, error) {
fqdn, ip4Mask, ip6Mask, err := splitDomainDualCIDR(domainSpec(t.value, p.domain))
if err == nil {
fqdn, err = parseMacro(p, fqdn, false)
fqdn, _, err = parseMacro(p, fqdn, false)
}
if err == nil {
fqdn, err = truncateFQDN(fqdn)
Expand Down Expand Up @@ -530,13 +530,17 @@ func (p *parser) parseMX(t *token) (bool, Result, time.Duration, error) {
}

func (p *parser) parseInclude(t *token) (bool, Result, error) {
domain, err := parseMacro(p, t.value, false)
domain, missingMacros, err := parseMacro(p, t.value, false)
if err == nil {
domain, err = truncateFQDN(domain)
}
if err == nil && !isDomainName(domain) {
err = newInvalidDomainError(domain)
}
if len(missingMacros) > 0 {
err = newMissingMacrosError(domain, missingMacros)
}

domain = NormalizeFQDN(domain)
p.fireDirective(t, domain)
if err != nil {
Expand Down Expand Up @@ -587,13 +591,17 @@ func (p *parser) parseInclude(t *token) (bool, Result, error) {
}

func (p *parser) parseExists(t *token) (bool, Result, time.Duration, error) {
resolvedDomain, err := parseMacroToken(p, t)
resolvedDomain, missingMacros, err := parseMacroToken(p, t)
if err == nil {
resolvedDomain, err = truncateFQDN(resolvedDomain)
}
if err == nil && !isDomainName(resolvedDomain) {
err = newInvalidDomainError(resolvedDomain)
}
if len(missingMacros) > 0 {
err = newMissingMacrosError(resolvedDomain, missingMacros)
}

resolvedDomain = NormalizeFQDN(resolvedDomain)
p.fireDirective(t, resolvedDomain)
if err != nil {
Expand All @@ -619,7 +627,7 @@ func (p *parser) parseExists(t *token) (bool, Result, time.Duration, error) {
// https://www.rfc-editor.org/rfc/rfc7208#section-5.5
func (p *parser) parsePtr(t *token) (bool, Result, error) {
fqdn := domainSpec(t.value, p.domain)
fqdn, err := parseMacro(p, fqdn, false)
fqdn, _, err := parseMacro(p, fqdn, false)
if err == nil {
fqdn, err = truncateFQDN(fqdn)
}
Expand Down Expand Up @@ -678,7 +686,7 @@ func (p *parser) handleRedirect(t *token) (Result, error) {
result Result
)

domain, err := parseMacro(p, t.value, false)
domain, _, err := parseMacro(p, t.value, false)
if err == nil {
domain, err = truncateFQDN(domain)
}
Expand Down Expand Up @@ -707,7 +715,7 @@ func (p *parser) handleRedirect(t *token) (Result, error) {
}

func (p *parser) handleExplanation(t *token) (string, error) {
domain, err := parseMacroToken(p, t)
domain, _, err := parseMacroToken(p, t)
if err != nil {
return "", SyntaxError{t, err}
}
Expand All @@ -733,7 +741,7 @@ func (p *parser) handleExplanation(t *token) (string, error) {
// not in the "unreserved" set, which is defined in [RFC3986].
// https://tools.ietf.org/html/rfc7208#section-7.3
// looks like we need to do it after truncating
exp, err := parseMacro(p, strings.Join(txts, ""), true)
exp, _, err := parseMacro(p, strings.Join(txts, ""), true)
if err != nil {
return "", SyntaxError{t, err}
}
Expand Down
8 changes: 8 additions & 0 deletions spf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package spf

import (
"errors"
"fmt"
"net"
"strconv"
"strings"
Expand Down Expand Up @@ -50,6 +51,13 @@ func newInvalidDomainError(domain string) error {
}
}

func newMissingMacrosError(domain string, macros []string) error {
return &DomainError{
Err: fmt.Sprintf("macros values missing: %s. ", strings.Join(macros, ", ")),
Domain: domain,
}
}

// IPMatcherFunc returns true if ip matches to implemented rules.
// If IPMatcherFunc returns any non nil error, the Resolver must stop
// any further processing and use the error as resulting error.
Expand Down

0 comments on commit 9b9eacf

Please sign in to comment.