From 7305ea73094d20aaa28e440c35ee897c010cc02e Mon Sep 17 00:00:00 2001 From: Abhinav Raut Date: Sun, 25 Oct 2026 11:41:21 +0530 Subject: [PATCH] fix security audit review findings and CI tests --- .github/workflows/frontend-ci.yml | 13 ++++- cmd/auth.go | 9 ++- .../apps/main/src/composables/useLogout.js | 20 ++++++- .../main/src/composables/useLogout.test.js | 15 +++++ frontend/cypress/e2e/api/conversations.cy.js | 40 +++++++++++++ frontend/cypress/e2e/testLogin.cy.js | 56 +++++++++++++++++++ internal/auth/auth.go | 12 +++- internal/auth/auth_test.go | 53 ++++++++++++++++++ internal/conversation/whatsapp_test.go | 28 +++++++++- 9 files changed, 232 insertions(+), 14 deletions(-) diff --git a/.github/workflows/frontend-ci.yml b/.github/workflows/frontend-ci.yml index 2be0ae37..c427a101 100644 --- a/.github/workflows/frontend-ci.yml +++ b/.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" \ No newline at end of file + pnpm exec cypress run --e2e --headless --spec "$specs" diff --git a/cmd/auth.go b/cmd/auth.go index cec8d643..6808dff8 100644 --- a/cmd/auth.go +++ b/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, "") } diff --git a/frontend/apps/main/src/composables/useLogout.js b/frontend/apps/main/src/composables/useLogout.js index dbd58b22..1d246955 100644 --- a/frontend/apps/main/src/composables/useLogout.js +++ b/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 + }) + } + } +} diff --git a/frontend/apps/main/src/composables/useLogout.test.js b/frontend/apps/main/src/composables/useLogout.test.js index fcec2384..9bf6efb8 100644 --- a/frontend/apps/main/src/composables/useLogout.test.js +++ b/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() + }) }) diff --git a/frontend/cypress/e2e/api/conversations.cy.js b/frontend/cypress/e2e/api/conversations.cy.js index 48d36788..e189a5f6 100644 --- a/frontend/cypress/e2e/api/conversations.cy.js +++ b/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: '

Hi Api

' }, + { sender_type: 'agent', private: true, expected: '

Hi Api

' }, + { sender_type: 'contact', private: false, expected: '

Hi {{ .Contact.FirstName }}

' } + ]) { + cy.api('POST', `/api/v1/conversations/${uuid}/messages`, { + message: '

Hi {{ .Contact.FirstName }}

', + 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) diff --git a/frontend/cypress/e2e/testLogin.cy.js b/frontend/cypress/e2e/testLogin.cy.js index eedd1899..83dfd20a 100644 --- a/frontend/cypress/e2e/testLogin.cy.js +++ b/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) + }) +}) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 8e58479d..98aff4dc 100644 --- a/internal/auth/auth.go +++ b/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 diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index bddab2ac..790c1a4b 100644 --- a/internal/auth/auth_test.go +++ b/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) diff --git a/internal/conversation/whatsapp_test.go b/internal/conversation/whatsapp_test.go index f053f561..6255893a 100644 --- a/internal/conversation/whatsapp_test.go +++ b/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) }