diff --git a/server/internal/akismet/akismet.go b/server/internal/akismet/akismet.go index 4165c19..afcefad 100644 --- a/server/internal/akismet/akismet.go +++ b/server/internal/akismet/akismet.go @@ -2,8 +2,10 @@ package akismet import ( "context" + "errors" "fmt" "io" + "log/slog" "net/http" "net/url" "strings" @@ -16,6 +18,36 @@ const ( defaultTimeout = 3 * time.Second ) +// Akismet answers every comment check with HTTP 200 and explains rejections +// and account problems in these headers instead. +const ( + debugHelpHeader = "X-Akismet-Debug-Help" + alertCodeHeader = "X-Akismet-Alert-Code" + alertMsgHeader = "X-Akismet-Alert-Msg" +) + +// ConfigError reports a check Akismet refused to answer because of how the +// integration is configured — an invalid or deactivated key, or a blog URL the +// key does not cover. Retrying cannot fix it, so callers should treat it as an +// operator-visible fault rather than a transient failure. +type ConfigError struct { + Body string + DebugHelp string +} + +func (e *ConfigError) Error() string { + if e.DebugHelp != "" { + return fmt.Sprintf("akismet rejected the request as %q: %s", e.Body, e.DebugHelp) + } + return fmt.Sprintf("akismet rejected the request as %q; check AKISMET_API_KEY and AKISMET_BLOG_URL", e.Body) +} + +// IsConfigError reports whether err is an Akismet configuration rejection. +func IsConfigError(err error) bool { + var configErr *ConfigError + return errors.As(err, &configErr) +} + type Checker interface { CheckComment(ctx context.Context, comment Comment) (bool, error) } @@ -132,9 +164,19 @@ func (c *Client) CheckComment(ctx context.Context, comment Comment) (bool, error return false, fmt.Errorf("read akismet response: %w", err) } body := strings.TrimSpace(string(bodyBytes)) + debugHelp := strings.TrimSpace(resp.Header.Get(debugHelpHeader)) if resp.StatusCode != http.StatusOK { - return false, fmt.Errorf("akismet returned %s: %s", resp.Status, body) + return false, fmt.Errorf("akismet returned %s: %s%s", resp.Status, body, debugHelpSuffix(debugHelp)) + } + + // Alerts ride along with an otherwise good verdict to flag an account + // problem — a plan that does not cover this site, say — so they are worth + // surfacing even though the check itself succeeded. + if alertCode := strings.TrimSpace(resp.Header.Get(alertCodeHeader)); alertCode != "" { + slog.Warn("akismet account alert", + "alert_code", alertCode, + "alert_message", strings.TrimSpace(resp.Header.Get(alertMsgHeader))) } switch body { @@ -142,9 +184,18 @@ func (c *Client) CheckComment(ctx context.Context, comment Comment) (bool, error return true, nil case "false": return false, nil + case "invalid": + return false, &ConfigError{Body: body, DebugHelp: debugHelp} default: - return false, fmt.Errorf("akismet returned unexpected response %q", body) + return false, fmt.Errorf("akismet returned unexpected response %q%s", body, debugHelpSuffix(debugHelp)) + } +} + +func debugHelpSuffix(debugHelp string) string { + if debugHelp == "" { + return "" } + return " (" + debugHelp + ")" } func valueOrDefault(value, fallback string) string { diff --git a/server/internal/akismet/akismet_test.go b/server/internal/akismet/akismet_test.go index 2a61607..154d925 100644 --- a/server/internal/akismet/akismet_test.go +++ b/server/internal/akismet/akismet_test.go @@ -107,8 +107,9 @@ func TestCheckCommentMapsHam(t *testing.T) { func TestCheckCommentRejectsUnexpectedResponse(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Akismet-Debug-Help", "Something went sideways") w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("invalid")) + _, _ = w.Write([]byte("nonsense")) })) defer server.Close() @@ -128,6 +129,94 @@ func TestCheckCommentRejectsUnexpectedResponse(t *testing.T) { if !strings.Contains(err.Error(), "unexpected response") { t.Fatalf("expected unexpected response error, got %v", err) } + if !strings.Contains(err.Error(), "Something went sideways") { + t.Fatalf("expected debug help in error, got %v", err) + } + if IsConfigError(err) { + t.Fatalf("an unreadable response is not a configuration error, got %v", err) + } +} + +func TestCheckCommentReportsInvalidKeyAsConfigError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Akismet-Debug-Help", `Empty "api_key" value`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("invalid")) + })) + defer server.Close() + + client, err := NewClient(Config{ + APIKey: "test-key", + BlogURL: "https://example.org", + Endpoint: server.URL, + }) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + _, err = client.CheckComment(t.Context(), Comment{UserIP: "203.0.113.10", Content: "hello"}) + if err == nil { + t.Fatal("expected configuration error") + } + if !IsConfigError(err) { + t.Fatalf("expected a configuration error, got %v", err) + } + if !strings.Contains(err.Error(), `Empty "api_key" value`) { + t.Fatalf("expected Akismet debug help in error, got %v", err) + } +} + +func TestCheckCommentReturnsVerdictAlongsideAccountAlert(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Akismet-Alert-Code", "301") + w.Header().Set("X-Akismet-Alert-Msg", "Upgrade required for a commercial site") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("true")) + })) + defer server.Close() + + client, err := NewClient(Config{ + APIKey: "test-key", + BlogURL: "https://example.org", + Endpoint: server.URL, + }) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + isSpam, err := client.CheckComment(t.Context(), Comment{UserIP: "203.0.113.10", Content: "hello"}) + if err != nil { + t.Fatalf("an account alert must not fail the check: %v", err) + } + if !isSpam { + t.Fatal("expected spam verdict") + } +} + +func TestCheckCommentIncludesDebugHelpOnHTTPError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Akismet-Debug-Help", "Missing required field") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte("bad request")) + })) + defer server.Close() + + client, err := NewClient(Config{ + APIKey: "test-key", + BlogURL: "https://example.org", + Endpoint: server.URL, + }) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + _, err = client.CheckComment(t.Context(), Comment{UserIP: "203.0.113.10", Content: "hello"}) + if err == nil { + t.Fatal("expected HTTP status error") + } + if !strings.Contains(err.Error(), "Missing required field") { + t.Fatalf("expected debug help in error, got %v", err) + } } func TestNewClientRequiresFullBlogURL(t *testing.T) { diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 0a4264f..38c3f0d 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -1795,7 +1795,13 @@ func PostArticleComment(conn *sql.DB, spamChecker akismet.Checker) http.HandlerF }) if err != nil { status = "pending" - slog.Warn("akismet comment check failed; comment requires moderation", "article_slug", slug, "error", err) + if akismet.IsConfigError(err) { + // Nothing gets filtered until an operator fixes the key or + // blog URL, so this is a fault, not a hiccup. + slog.Error("akismet is misconfigured; spam filtering is not running", "article_slug", slug, "error", err) + } else { + slog.Warn("akismet comment check failed; comment requires moderation", "article_slug", slug, "error", err) + } } else if isSpam { status = "spam" }