Merge pull request #10 from discoverygarden/IT-692

IT-692: Stop false positives on substrings

Author: adamcbowman

botblocker.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"regexp"
 
 	"net/http"
 	"net/netip"
@@ -183,26 +184,35 @@ func (b *BotBlocker) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	log.Debugf("Checking request: CIDR: \"%v\" user agent: \"%s\"", req.RemoteAddr, req.UserAgent())
 	// Using an external plugin to avoid https://github.com/traefik/yaegi/issues/1697
 	timer := getTimer(startTime)
-	defer timer()
 
 	remoteAddrPort, err := netip.ParseAddrPort(req.RemoteAddr)
 	if err != nil {
+		timer()
 		http.Error(rw, "internal error", http.StatusInternalServerError)
 		return
 	}
 	if b.shouldBlockIp(remoteAddrPort.Addr()) {
-		log.Infof("blocked request with from IP %v", remoteAddrPort.Addr())
+		log.Infof("blocked request with from IP \"%v\"", remoteAddrPort.Addr())
+		timer()
 		http.Error(rw, "blocked", http.StatusForbidden)
 		return
 	}
 
 	agent := strings.ToLower(req.UserAgent())
-	if b.shouldBlockAgent(agent) {
-		log.Infof("blocked request with user agent %v because it contained %v", agent, agent)
+	blocked, badAgent, err := b.shouldBlockAgent(agent)
+	if err != nil {
+		timer()
+		http.Error(rw, "internal error", http.StatusInternalServerError)
+		return
+	}
+	if blocked {
+		log.Infof("blocked request with user agent \"%v\" because it contained \"%v\"", agent, badAgent)
+		timer()
 		http.Error(rw, "blocked", http.StatusForbidden)
 		return
 	}
 
+	timer()
 	b.next.ServeHTTP(rw, req)
 }
 
@@ -215,14 +225,23 @@ func (b *BotBlocker) shouldBlockIp(addr netip.Addr) bool {
 	return false
 }
 
-func (b *BotBlocker) shouldBlockAgent(userAgent string) bool {
+func (b *BotBlocker) shouldBlockAgent(userAgent string) (bool, string, error) {
 	userAgent = strings.ToLower(strings.TrimSpace(userAgent))
 	for _, badAgent := range b.userAgentBlockList {
+		// fast check with contains
 		if strings.Contains(userAgent, badAgent) {
-			return true
+			// verify with regex
+			pattern := fmt.Sprintf(`(?:\b)%s(?:\b)`, badAgent)
+			matched, err := regexp.Match(pattern, []byte(userAgent))
+			if err != nil {
+				return false, "", fmt.Errorf("failed to check user agent %s: %e", userAgent, err)
+			}
+			if matched {
+				return true, badAgent, nil
+			}
 		}
 	}
-	return false
+	return false, "", nil
 }
 
 func getTimer(startTime time.Time) func() {

botblocker_test.go

@@ -150,29 +150,62 @@ func TestShouldAllowIpCidr(t *testing.T) {
 }
 
 func TestShouldBlockUserAgent(t *testing.T) {
+	badAgent := "nintendobrowser"
 	botBlocker := BotBlocker{
 		userAgentBlockList: []string{
-			"nintendobrowser",
+			badAgent,
 		},
 	}
-	badUserAgent := "Mozilla/5.0 (Nintendo WiiU) AppleWebKit/536.30 (KHTML, like Gecko) NX/3.0.4.2.12 NintendoBrowser/4.3.1.11264.US"
+	requestAgent := "Mozilla/5.0 (Nintendo WiiU) AppleWebKit/536.30 (KHTML, like Gecko) NX/3.0.4.2.12 NintendoBrowser/4.3.1.11264.US"
 
-	blocked := botBlocker.shouldBlockAgent(badUserAgent)
+	blocked, blockedAgent, err := botBlocker.shouldBlockAgent(requestAgent)
+	if err != nil {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) returned a none nil error", requestAgent)
+	}
 	if !blocked {
-		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %t; want true", badUserAgent, blocked)
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %t; want true", requestAgent, blocked)
+	}
+	if blockedAgent != badAgent {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %s; want \"\"", requestAgent, blockedAgent)
 	}
 }
 
-func TestShouldAlowUserAgent(t *testing.T) {
+func TestShouldAllowUserAgent(t *testing.T) {
 	botBlocker := BotBlocker{
 		userAgentBlockList: []string{
 			"nintendobrowser",
 		},
 	}
 	userAgent := "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36"
 
-	blocked := botBlocker.shouldBlockAgent(userAgent)
+	blocked, badAgent, err := botBlocker.shouldBlockAgent(userAgent)
+	if err != nil {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) returned a non nil error", userAgent)
+	}
+	if blocked {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %t; want false", userAgent, blocked)
+	}
+	if badAgent != "" {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %s; want \"\"", userAgent, badAgent)
+	}
+}
+
+func TestShouldAllowUserAgentSubstring(t *testing.T) {
+	botBlocker := BotBlocker{
+		userAgentBlockList: []string{
+			"obot",
+		},
+	}
+	userAgent := "mozilla/5.0+(compatible; uptimerobot/2.0; http://www.uptimerobot.com/)"
+
+	blocked, badAgent, err := botBlocker.shouldBlockAgent(userAgent)
+	if err != nil {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) returned a non nil error", userAgent)
+	}
 	if blocked {
 		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %t; want false", userAgent, blocked)
 	}
+	if badAgent != "" {
+		t.Fatalf("botBlocker.shouldBlockAgent(%s) = %s; want \"\"", userAgent, badAgent)
+	}
 }