fix: stop reporting expected errors to sentry

frontend/src/helpers/handleChunkLoadErrors.test.ts

@@ -0,0 +1,31 @@
+import {describe, it, expect, beforeEach} from 'vitest'
+
+import {canReloadForChunkLoadError, markChunkLoadErrorReload} from './handleChunkLoadErrors'
+
+describe('canReloadForChunkLoadError', () => {
+	beforeEach(() => {
+		sessionStorage.clear()
+	})
+
+	it('allows a reload when nothing was reloaded yet', () => {
+		expect(canReloadForChunkLoadError()).toBe(true)
+	})
+
+	it('blocks a reload right after one happened', () => {
+		markChunkLoadErrorReload(1_000)
+
+		expect(canReloadForChunkLoadError(2_000)).toBe(false)
+	})
+
+	it('allows a reload again after the cooldown passed', () => {
+		markChunkLoadErrorReload(1_000)
+
+		expect(canReloadForChunkLoadError(1_000 + 60_000)).toBe(true)
+	})
+
+	it('allows a reload when the stored timestamp is garbage', () => {
+		sessionStorage.setItem('chunkLoadErrorReloadedAt', 'not a number')
+
+		expect(canReloadForChunkLoadError()).toBe(true)
+	})
+})

frontend/src/helpers/handleChunkLoadErrors.ts

@@ -0,0 +1,45 @@
+const LAST_RELOAD_KEY = 'chunkLoadErrorReloadedAt'
+const RELOAD_COOLDOWN = 60 * 1000
+
+function readLastReload(): number {
+	try {
+		return Number(sessionStorage.getItem(LAST_RELOAD_KEY))
+	} catch {
+		return 0
+	}
+}
+
+/**
+ * Reloading only helps when the page is stale, so a second failure right after
+ * a reload means something else is broken — let that one through to Sentry
+ * instead of reloading forever.
+ */
+export function canReloadForChunkLoadError(now: number = Date.now()): boolean {
+	const lastReload = readLastReload()
+
+	if (!Number.isFinite(lastReload) || lastReload <= 0) {
+		return true
+	}
+
+	return now - lastReload >= RELOAD_COOLDOWN
+}
+
+export function markChunkLoadErrorReload(now: number = Date.now()) {
+	try {
+		sessionStorage.setItem(LAST_RELOAD_KEY, String(now))
+	} catch {
+		// A blocked sessionStorage only costs us the loop guard.
+	}
+}
+
+export function handleChunkLoadErrors() {
+	window.addEventListener('vite:preloadError', event => {
+		if (!canReloadForChunkLoadError()) {
+			return
+		}
+
+		event.preventDefault()
+		markChunkLoadErrorReload()
+		window.location.reload()
+	})
+}

frontend/src/helpers/sentryFilters.test.ts

@@ -0,0 +1,75 @@
+import {describe, it, expect} from 'vitest'
+import {AxiosError} from 'axios'
+
+import {shouldDropEvent} from './sentryFilters'
+
+// Object.assign instead of `new Error(msg, {cause})`: the vitest tsconfig
+// targets a lib without the two-argument Error constructor.
+function errorWithCause(message: string, cause: unknown): Error {
+	return Object.assign(new Error(message), {cause})
+}
+
+describe('shouldDropEvent', () => {
+	it('drops a plain AxiosError', () => {
+		expect(shouldDropEvent(new AxiosError('Request failed'))).toBe(true)
+	})
+
+	it('drops an error wrapping an AxiosError as cause', () => {
+		expect(shouldDropEvent(errorWithCause('Error renewing token: ', new AxiosError('Request failed')))).toBe(true)
+	})
+
+	it('drops an error with an AxiosError two levels deep', () => {
+		const inner = errorWithCause('inner', new AxiosError('Request failed'))
+
+		expect(shouldDropEvent(errorWithCause('outer', inner))).toBe(true)
+	})
+
+	it('drops an error-like object with code and message', () => {
+		expect(shouldDropEvent({code: 'ECONNABORTED', message: 'timeout'})).toBe(true)
+	})
+
+	it('keeps a plain error', () => {
+		expect(shouldDropEvent(new Error('something actually broke'))).toBe(false)
+	})
+
+	it('keeps a plain error wrapping another plain error', () => {
+		expect(shouldDropEvent(errorWithCause('outer', new Error('inner')))).toBe(false)
+	})
+
+	it('keeps undefined', () => {
+		expect(shouldDropEvent(undefined)).toBe(false)
+	})
+
+	it('does not loop on a cause cycle', () => {
+		const a = new Error('a')
+		const b = errorWithCause('b', a)
+		Object.assign(a, {cause: b})
+
+		expect(shouldDropEvent(a)).toBe(false)
+	})
+})
+
+describe('shouldDropEvent with chunk load errors', () => {
+	const messages = [
+		'Failed to fetch dynamically imported module: https://try.vikunja.io/assets/ProjectList-abc123.js',
+		'error loading dynamically imported module: https://try.vikunja.io/assets/ProjectList-abc123.js',
+		'Importing a module script failed.',
+		'Unable to preload CSS for /assets/ProjectList-abc123.css',
+	]
+
+	it.each(messages)('drops the exception %s', message => {
+		expect(shouldDropEvent(new Error(message))).toBe(true)
+	})
+
+	it.each(messages)('drops the event message %s', message => {
+		expect(shouldDropEvent(undefined, {message})).toBe(true)
+	})
+
+	it.each(messages)('drops the event exception value %s', message => {
+		expect(shouldDropEvent(undefined, {exception: {values: [{value: message}]}})).toBe(true)
+	})
+
+	it('keeps an unrelated event message', () => {
+		expect(shouldDropEvent(undefined, {message: 'something actually broke'})).toBe(false)
+	})
+})

frontend/src/helpers/sentryFilters.ts

@@ -0,0 +1,62 @@
+import {AxiosError} from 'axios'
+
+// Failed requests are surfaced to the user through the UI already, and an
+// expired session (401 on token refresh) is expected rather than a bug.
+// Errors wrapping one of them via `cause` count too.
+const MAX_CAUSE_DEPTH = 10
+
+// Thrown when a user has an old index.html open and a deploy changed the asset
+// hashes. handleChunkLoadErrors() reloads the page instead.
+const CHUNK_LOAD_ERROR_PATTERNS = [
+	/failed to fetch dynamically imported module/i,
+	/error loading dynamically imported module/i,
+	/importing a module script failed/i,
+	/unable to preload css/i,
+]
+
+type SentryEventLike = {
+	message?: string
+	exception?: {
+		values?: {value?: string}[]
+	}
+}
+
+function isRequestError(e: unknown): boolean {
+	if (e instanceof AxiosError) {
+		return true
+	}
+
+	if (typeof e !== 'object' || e === null) {
+		return false
+	}
+
+	return typeof (e as {code?: unknown}).code !== 'undefined'
+		&& typeof (e as {message?: unknown}).message !== 'undefined'
+}
+
+export function isChunkLoadError(message: unknown): boolean {
+	return typeof message === 'string'
+		&& CHUNK_LOAD_ERROR_PATTERNS.some(pattern => pattern.test(message))
+}
+
+export function shouldDropEvent(originalException: unknown, event?: SentryEventLike): boolean {
+	if (isChunkLoadError(event?.message)) {
+		return true
+	}
+
+	if (event?.exception?.values?.some(value => isChunkLoadError(value?.value))) {
+		return true
+	}
+
+	let current = originalException
+
+	for (let depth = 0; depth < MAX_CAUSE_DEPTH && current; depth++) {
+		if (isRequestError(current) || isChunkLoadError((current as {message?: unknown}).message)) {
+			return true
+		}
+
+		current = (current as {cause?: unknown}).cause
+	}
+
+	return false
+}

frontend/src/main.ts

@@ -54,8 +54,10 @@ import Modal from '@/components/misc/Modal.vue'
 import Card from '@/components/misc/Card.vue'
 
 import {setupKeyboardModality} from '@/helpers/keyboardModality'
+import {handleChunkLoadErrors} from '@/helpers/handleChunkLoadErrors'
 
 setupKeyboardModality()
+handleChunkLoadErrors()
 
 // We're loading the language before creating the app so that it won't fail to load when the user's 
 // language file is not yet loaded.

frontend/src/sentry.ts

@@ -1,6 +1,6 @@
 import type {App} from 'vue'
 import type {Router} from 'vue-router'
-import {AxiosError} from 'axios'
+import {shouldDropEvent} from './helpers/sentryFilters'
 import {VERSION} from './version.json'
 
 export default async function setupSentry(app: App, router: Router) {
@@ -40,10 +40,7 @@ export default async function setupSentry(app: App, router: Router) {
 
 
 		beforeSend(event, hint) {
-
-			if ((typeof hint.originalException?.code !== 'undefined' && 
-				typeof hint.originalException?.message !== 'undefined')
-			|| hint.originalException instanceof AxiosError) {
+			if (shouldDropEvent(hint.originalException, event)) {
 				return null
 			}
 

pkg/events/events.go

@@ -55,10 +55,20 @@ type Event interface {
 	Name() string
 }
 
+// MetadataSkipErrorReporting marks a message whose failure is a user
+// configuration problem (an unreachable webhook target, say) rather than a bug,
+// so parking it in the poison queue must not page us. The poison middleware
+// republishes the same message, so metadata a handler sets survives.
+const MetadataSkipErrorReporting = "skip_error_reporting"
+
 type messageHandleFailedError struct {
 	Metadata message.Metadata
 }
 
+func shouldReportPoisonedMessage(meta message.Metadata) bool {
+	return meta.Get(MetadataSkipErrorReporting) != "true"
+}
+
 func (m *messageHandleFailedError) Error() string {
 	return fmt.Sprintf("Failed to handle message: %v", m.Metadata)
 }
@@ -97,7 +107,7 @@ func InitEvents() (err error) {
 		// The payload is deliberately not logged: events can carry credentials and user data.
 		log.Errorf("Error while handling message %s, %s", msg.UUID, meta)
 
-		if config.SentryEnabled.GetBool() {
+		if config.SentryEnabled.GetBool() && shouldReportPoisonedMessage(msg.Metadata) {
 			sentry.CaptureException(&messageHandleFailedError{
 				Metadata: msg.Metadata,
 			})

pkg/events/events_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/ThreeDotsLabs/watermill/message"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -91,3 +92,18 @@ func TestDispatchPendingNoEvents(t *testing.T) {
 	// Verify no events were dispatched
 	assert.Equal(t, 0, CountDispatchedEvents("test.event"))
 }
+
+func TestShouldReportPoisonedMessage(t *testing.T) {
+	t.Run("no metadata", func(t *testing.T) {
+		assert.True(t, shouldReportPoisonedMessage(message.Metadata{}))
+	})
+	t.Run("unrelated metadata", func(t *testing.T) {
+		assert.True(t, shouldReportPoisonedMessage(message.Metadata{"reason_poisoned": "boom"}))
+	})
+	t.Run("flag set", func(t *testing.T) {
+		assert.False(t, shouldReportPoisonedMessage(message.Metadata{MetadataSkipErrorReporting: "true"}))
+	})
+	t.Run("flag set to something else", func(t *testing.T) {
+		assert.True(t, shouldReportPoisonedMessage(message.Metadata{MetadataSkipErrorReporting: "false"}))
+	})
+}

pkg/models/listeners.go

@@ -1226,7 +1226,14 @@ func (wdl *WebhookDeliveryListener) Handle(msg *message.Message) error {
 		return nil
 	}
 
-	return webhook.sendWebhookPayload(evt.Payload)
+	if err := webhook.sendWebhookPayload(evt.Payload); err != nil {
+		// A target that is down or rejects the payload is the user's to fix, so
+		// don't report it — but still retry and eventually poison the message.
+		msg.Metadata.Set(events.MetadataSkipErrorReporting, "true")
+		return err
+	}
+
+	return nil
 }
 
 func getIDAsInt64(id interface{}) int64 {

pkg/models/listeners_test.go

@@ -21,6 +21,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"sync"
@@ -34,6 +36,7 @@ import (
 	"code.vikunja.io/api/pkg/license"
 	"code.vikunja.io/api/pkg/user"
 
+	"github.com/ThreeDotsLabs/watermill/message"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"xorm.io/xorm"
@@ -714,3 +717,46 @@ func TestAuditUserDataExportRequested(t *testing.T) {
 	assert.Equal(t, audit.UserTarget(42), entry.Target)
 	assert.Equal(t, audit.OutcomeSuccess, entry.Outcome)
 }
+
+func TestWebhookDeliveryListenerSkipsErrorReporting(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer ts.Close()
+
+	// httptest binds to loopback, which the SSRF-safe client blocks by default.
+	previousAllowNonRoutable := config.OutgoingRequestsAllowNonRoutableIPs.GetBool()
+	config.OutgoingRequestsAllowNonRoutableIPs.Set(true)
+	previousClient := webhookClient
+	webhookClient = nil
+	t.Cleanup(func() {
+		config.OutgoingRequestsAllowNonRoutableIPs.Set(previousAllowNonRoutable)
+		webhookClient = previousClient
+	})
+
+	s := db.NewSession()
+	webhook := &Webhook{
+		TargetURL:   ts.URL,
+		Events:      []string{"task.updated"},
+		ProjectID:   1,
+		CreatedByID: 1,
+	}
+	_, err := s.Insert(webhook)
+	require.NoError(t, err)
+	require.NoError(t, s.Commit())
+	_ = s.Close()
+
+	payload, err := json.Marshal(&WebhookDeliveryEvent{
+		WebhookID: webhook.ID,
+		Payload:   &WebhookPayload{EventName: "task.updated"},
+	})
+	require.NoError(t, err)
+
+	msg := message.NewMessage("test", payload)
+	err = (&WebhookDeliveryListener{}).Handle(msg)
+
+	require.Error(t, err)
+	assert.Equal(t, "true", msg.Metadata.Get(events.MetadataSkipErrorReporting))
+}