fix security audit review findings and CI tests

.github/workflows/frontend-ci.yml

@@ -101,6 +101,17 @@ jobs:
         run: |
           ./libredesk --install --idempotent-install --yes --config ./config.toml
           ./libredesk --upgrade --yes --config ./config.toml
+          # Fixtures simulate incoming messages through the contact writing permission.
+          docker exec -i '${{ job.services.db.id }}' psql -U libredesk -d libredesk -v ON_ERROR_STOP=1 <<'SQL'
+          WITH fixture_role AS (
+            INSERT INTO roles (name, description, permissions)
+            VALUES ('Cypress contact writer', 'Create incoming test messages', '{messages:write_as_contact}')
+            RETURNING id
+          )
+          INSERT INTO user_roles (user_id, role_id)
+          SELECT users.id, fixture_role.id FROM users CROSS JOIN fixture_role
+          WHERE users.email = 'System';
+          SQL
           ./libredesk --config ./config.toml &
           for i in $(seq 1 60); do
             if [ "$(curl --connect-timeout 1 --max-time 2 -s -o /dev/null -w '%{http_code}' http://localhost:9000/ || true)" = "200" ]; then
@@ -112,4 +123,4 @@ jobs:
           cd frontend
           specs=$(find cypress/e2e -type f -regextype posix-extended -regex '.*\.(cy|spec)\.(js|jsx|ts|tsx)$' | sort | awk -v s=${{ matrix.shard }} -v n=5 'NR % n == s % n' | paste -sd,)
           echo "shard ${{ matrix.shard }} runs: $specs"
-          pnpm exec cypress run --e2e --headless --spec "$specs"
+          pnpm exec cypress run --e2e --headless --spec "$specs"

cmd/auth.go

@@ -54,16 +54,15 @@ func handleOIDCLogin(r *fastglue.Request) error {
 		oidcNextSessKey: next,
 	}
 
-	if err = app.auth.CreateSessionWithValues(r, sessionValues); err != nil {
-		app.lo.Error("error saving state in session", "error", err)
-		return redirectLoginError(r, oidcErrLoginFailed, next)
-	}
-
 	authURL, err := app.auth.LoginURL(providerID, state)
 	if err != nil {
 		app.lo.Error("error getting oidc login url", "provider_id", providerID, "error", err)
 		return redirectLoginError(r, oidcErrLoginFailed, next)
 	}
+	if err = app.auth.SetSessionValues(r, sessionValues); err != nil {
+		app.lo.Error("error saving state in session", "error", err)
+		return redirectLoginError(r, oidcErrLoginFailed, next)
+	}
 	app.lo.Debug("redirecting to oidc provider for login", "provider_id", providerID)
 	return r.Redirect(authURL, fasthttp.StatusFound, nil, "")
 }

frontend/apps/main/src/composables/useLogout.js

@@ -1,14 +1,30 @@
 import api from '@main/api'
+import { useEmitter } from '@main/composables/useEmitter'
 import { usePushNotifications } from '@main/composables/usePushNotifications'
+import { EMITTER_EVENTS } from '@main/constants/emitterEvents.js'
 import { clearNewConversationDrafts } from '@main/features/conversation/useNewConversationDraft.js'
+import { handleHTTPError } from '@shared-ui/utils/http.js'
 
 const defaultBrowser = typeof window === 'undefined' ? {} : window
 
 export const createLogout = (pushNotifications, browser = defaultBrowser, apiClient = api) => async () => {
   await Promise.resolve(pushNotifications.disable()).catch(() => {})
+  await apiClient.logout()
   clearNewConversationDrafts(browser.localStorage)
-  await apiClient.logout().catch(() => {})
   browser.location.href = '/'
 }
 
-export const useLogout = () => createLogout(usePushNotifications())
+export const useLogout = () => {
+  const emitter = useEmitter()
+  const logout = createLogout(usePushNotifications())
+  return async () => {
+    try {
+      await logout()
+    } catch (error) {
+      emitter.emit(EMITTER_EVENTS.SHOW_TOAST, {
+        variant: 'destructive',
+        description: handleHTTPError(error).message
+      })
+    }
+  }
+}

frontend/apps/main/src/composables/useLogout.test.js

@@ -24,4 +24,19 @@ describe('createLogout', () => {
     expect(apiClient.logout).toHaveBeenCalled()
     expect(browser.location.href).toBe('/')
   })
+
+  it('preserves the page and drafts when logout fails', async () => {
+    const pushNotifications = { disable: vi.fn().mockResolvedValue() }
+    const error = new Error('offline')
+    const apiClient = { logout: vi.fn().mockRejectedValue(error) }
+    const browser = {
+      location: { href: '/inboxes/assigned' },
+      localStorage: { removeItem: vi.fn() }
+    }
+
+    await expect(createLogout(pushNotifications, browser, apiClient)()).rejects.toBe(error)
+
+    expect(browser.location.href).toBe('/inboxes/assigned')
+    expect(browser.localStorage.removeItem).not.toHaveBeenCalled()
+  })
 })

frontend/cypress/e2e/api/conversations.cy.js

@@ -64,6 +64,28 @@ describe('API: conversations', () => {
 
   beforeEach(() => cy.login())
 
+  it('requires permission to create a message as a contact', () => {
+    cy.api('GET', '/api/v1/roles').then(({ body }) => {
+      const role = body.data.find((entry) => entry.name === 'Cypress contact writer')
+      cy.api('PUT', `/api/v1/roles/${role.id}`, {
+        ...role,
+        permissions: ['conversations:write']
+      })
+      cy.api('POST', '/api/v1/conversations', {
+        inbox_id: inboxId,
+        contact_email: contactEmail,
+        first_name: 'Api',
+        content: 'hi',
+        initiator: 'contact'
+      }, { failOnStatusCode: false }).then((response) => {
+        cy.api('PUT', `/api/v1/roles/${role.id}`, role).then(() => {
+          expect(response.status).to.eq(403)
+          expect(response.body.error_type).to.eq('PermissionException')
+        })
+      })
+    })
+  })
+
   it('rejects a create with no inbox_id', () => {
     cy.api('POST', '/api/v1/conversations', {
       contact_email: contactEmail, first_name: 'Api', content: 'hi', initiator: 'contact'
@@ -171,6 +193,24 @@ describe('API: conversations', () => {
     })
   })
 
+  it('expands staff variables while preserving contact text', () => {
+    for (const entry of [
+      { sender_type: 'agent', private: false, expected: '<p>Hi Api</p>' },
+      { sender_type: 'agent', private: true, expected: '<p>Hi Api</p>' },
+      { sender_type: 'contact', private: false, expected: '<p>Hi {{ .Contact.FirstName }}</p>' }
+    ]) {
+      cy.api('POST', `/api/v1/conversations/${uuid}/messages`, {
+        message: '<p>Hi {{ .Contact.FirstName }}</p>',
+        sender_type: entry.sender_type,
+        private: entry.private,
+        to: [contactEmail]
+      }).then(({ status, body }) => {
+        expect(status).to.eq(200)
+        expect(body.data.content).to.eq(entry.expected)
+      })
+    }
+  })
+
   it('reads the conversation back by uuid', () => {
     cy.api('GET', `/api/v1/conversations/${uuid}`).then(({ status, body }) => {
       expect(status).to.eq(200)

frontend/cypress/e2e/testLogin.cy.js

@@ -158,3 +158,59 @@ describe('Login Component', () => {
         cy.wait('@slowLogin')
     })
 })
+
+describe('Logout', () => {
+    beforeEach(() => {
+        cy.viewport(1440, 900)
+        cy.login()
+        cy.visit('/inboxes/assigned')
+    })
+
+    const clickLogout = () => {
+        cy.contains('button', 'System').click()
+        cy.contains('[role="menuitem"]', /log\s*out/i).click()
+    }
+
+    it('keeps the session active when logout fails', () => {
+        cy.intercept('POST', '**/api/v1/auth/logout', {
+            statusCode: 500,
+            body: { message: 'Logout failed. Try again.' }
+        }).as('logoutFailure')
+        clickLogout()
+        cy.wait('@logoutFailure')
+        cy.contains('Logout failed. Try again.').should('be.visible')
+        cy.location('pathname').should('eq', '/inboxes/assigned')
+        cy.request('/api/v1/agents/me').its('status').should('eq', 200)
+    })
+
+    it('requires a CSRF token and ignores the old GET logout route', () => {
+        cy.request({
+            method: 'POST',
+            url: '/api/v1/auth/logout',
+            failOnStatusCode: false
+        }).its('status').should('eq', 403)
+        cy.request({ url: '/logout', failOnStatusCode: false })
+        cy.request('/api/v1/agents/me').its('status').should('eq', 200)
+    })
+
+    it('preserves the login when an SSO provider is unavailable', () => {
+        cy.getCookie('libredesk_session').then((cookie) => {
+            cy.request({ url: '/api/v1/oidc/999999/login', followRedirect: false })
+                .its('status').should('eq', 302)
+            cy.getCookie('libredesk_session').its('value').should('eq', cookie.value)
+        })
+        cy.request('/api/v1/agents/me').its('status').should('eq', 200)
+    })
+
+    it('destroys the session before returning to the login page', () => {
+        cy.intercept('POST', '**/api/v1/auth/logout').as('logout')
+        clickLogout()
+        cy.wait('@logout').its('response.statusCode').should('eq', 200)
+        cy.location('pathname').should('eq', '/')
+        cy.get('#email').should('be.visible')
+        cy.request({
+            url: '/api/v1/agents/me',
+            failOnStatusCode: false
+        }).its('status').should('eq', 401)
+    })
+})

internal/auth/auth.go

@@ -312,12 +312,18 @@ func (a *Auth) SaveSession(user amodels.User, r *fastglue.Request) error {
 	return nil
 }
 
-// CreateSessionWithValues starts a new session holding the passed values.
-func (a *Auth) CreateSessionWithValues(r *fastglue.Request, values map[string]interface{}) error {
+// SetSessionValues preserves an existing session and creates one only when the browser has no valid session.
+func (a *Auth) SetSessionValues(r *fastglue.Request, values map[string]any) error {
 	a.mu.RLock()
 	defer a.mu.RUnlock()
 
-	sess, err := a.sess.NewSession(r, r)
+	sess, err := a.sess.Acquire(r.RequestCtx, r, r)
+	if err == nil {
+		_, err = sess.GetMulti("id")
+	}
+	if errors.Is(err, simplesessions.ErrInvalidSession) {
+		sess, err = a.sess.NewSession(r, r)
+	}
 	if err != nil {
 		a.logger.Error("error creating session", "error", err)
 		return err

internal/auth/auth_test.go

@@ -316,6 +316,59 @@ func TestSessionValidationUsesOneRedisRead(t *testing.T) {
 	}
 }
 
+func TestSessionValuesPreserveLoginAndReplaceInvalidCookies(t *testing.T) {
+	for _, cookieType := range []string{"missing", "unknown", "expired", "authenticated"} {
+		t.Run(cookieType, func(t *testing.T) {
+			a, _, mr := sessionAuthForTest(t, time.Hour)
+			cookie := ""
+			switch cookieType {
+			case "unknown":
+				cookie = "unknown-session"
+			case "expired", "authenticated":
+				cookie = saveTestSession(t, a, 1)
+				if cookieType == "expired" {
+					mr.FastForward(2 * time.Hour)
+				}
+			}
+			r := sessionRequest(cookie)
+			if err := a.SetSessionValues(r, map[string]any{"oidc_state": "state"}); err != nil {
+				t.Fatal(err)
+			}
+			var responseCookie fasthttp.Cookie
+			responseCookie.SetKey("libredesk_session")
+			hasCookie := r.RequestCtx.Response.Header.Cookie(&responseCookie)
+			if cookieType == "authenticated" {
+				if hasCookie {
+					t.Fatal("SSO replaced the authenticated session cookie")
+				}
+				requireSession(t, a, cookie, 1)
+			} else {
+				if !hasCookie || len(responseCookie.Value()) != 64 || string(responseCookie.Value()) == cookie {
+					t.Fatal("SSO did not issue a new random session cookie")
+				}
+				cookie = string(responseCookie.Value())
+			}
+			state, err := a.GetSessionValue(sessionRequest(cookie), "oidc_state")
+			if err != nil || state != "state" {
+				t.Fatalf("SSO state = %v, error = %v", state, err)
+			}
+		})
+	}
+}
+
+func TestSessionValidationDoesNotCreateAnonymousSessions(t *testing.T) {
+	a, _, mr := sessionAuthForTest(t, time.Hour)
+	for _, cookie := range []string{"", "unknown-session"} {
+		r := sessionRequest(cookie)
+		if _, err := a.ValidateSession(r); !errors.Is(err, simplesessions.ErrInvalidSession) {
+			t.Fatalf("invalid session accepted: %v", err)
+		}
+		if len(mr.Keys()) != 0 || len(r.RequestCtx.Response.Header.Peek("Set-Cookie")) != 0 {
+			t.Fatal("invalid session validation created an anonymous session")
+		}
+	}
+}
+
 func sessionAuthForTest(t *testing.T, lifetime time.Duration) (*Auth, *redis.Client, *miniredis.Miniredis) {
 	t.Helper()
 	mr := miniredis.RunT(t)

internal/conversation/whatsapp_test.go

@@ -107,12 +107,16 @@ func TestQueueWhatsAppReplyVariables(t *testing.T) {
 		{"rendered text at limit", strings.Repeat("x", whatsAppMaxTextLength-len("Customer")) + "{{ .Contact.FirstName }}", map[string]any{}, strings.Repeat("x", whatsAppMaxTextLength-len("Customer")) + "Customer"},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
+			content := tc.content
+			if extractInt(tc.meta, "whatsapp_template_id") <= 0 {
+				content = m.RenderStaffTemplate(conv.UUID, senderID, content)
+			}
 			message, err := m.QueueReply(nil, /** media **/
-				inboxID, senderID, contactID, conv.UUID, tc.content,
+				inboxID, senderID, contactID, conv.UUID, content,
 				nil, /** to **/
 				nil, /** cc **/
 				nil, /** bcc **/
-				"", /** sendFrom **/
+				"",  /** sendFrom **/
 				tc.meta)
 			if err != nil {
 				t.Fatal(err)
@@ -134,17 +138,35 @@ func TestQueueWhatsAppReplyVariables(t *testing.T) {
 		})
 	}
 
+	t.Run("literal content is never expanded by the queue", func(t *testing.T) {
+		const content = "{{ .Contact.FirstName }} {{ range 3 }}A{{ end }}"
+		message, err := m.QueueReply(nil, /** media **/
+			inboxID, senderID, contactID, conv.UUID, content,
+			nil, /** to **/
+			nil, /** cc **/
+			nil, /** bcc **/
+			"",  /** sendFrom **/
+			map[string]any{})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if message.Content != content || message.TextContent != content {
+			t.Fatalf("literal message changed: %+v", message)
+		}
+	})
+
 	t.Run("expanded content exceeds limit", func(t *testing.T) {
 		if _, err := db.Exec(`UPDATE users SET first_name = $1 WHERE id = $2`, strings.Repeat("a", 64), contactID); err != nil {
 			t.Fatal(err)
 		}
 		content := strings.Repeat("x", whatsAppMaxTextLength-32) + "{{ .Contact.FirstName }}"
+		content = m.RenderStaffTemplate(conv.UUID, senderID, content)
 		if _, err := m.QueueReply(nil, /** media **/
 			inboxID, senderID, contactID, conv.UUID, content,
 			nil, /** to **/
 			nil, /** cc **/
 			nil, /** bcc **/
-			"", /** sendFrom **/
+			"",  /** sendFrom **/
 			map[string]any{}); err == nil || !strings.Contains(err.Error(), "4096") {
 			t.Fatalf("expected rendered content to exceed the limit, got %v", err)
 		}