diff --git a/apps/portal/package.json b/apps/portal/package.json index c1f630e2ef..99ae1813a2 100644 --- a/apps/portal/package.json +++ b/apps/portal/package.json @@ -1,6 +1,6 @@ { "name": "@tryghost/portal", - "version": "2.38.0", + "version": "2.39.0", "license": "MIT", "repository": { "type": "git", diff --git a/apps/portal/src/App.js b/apps/portal/src/App.js index 66b920872d..2e3d80d493 100644 --- a/apps/portal/src/App.js +++ b/apps/portal/src/App.js @@ -211,7 +211,7 @@ export default class App extends React.Component { async fetchData() { const {site: apiSiteData, member} = await this.fetchApiData(); const {site: devSiteData, ...restDevData} = this.fetchDevData(); - const {site: linkSiteData, ...restLinkData} = this.fetchLinkData(apiSiteData); + const {site: linkSiteData, ...restLinkData} = this.fetchLinkData(apiSiteData, member); const {site: previewSiteData, ...restPreviewData} = this.fetchPreviewData(); const {site: notificationSiteData, ...restNotificationData} = this.fetchNotificationData(); let page = ''; @@ -420,18 +420,32 @@ export default class App extends React.Component { } /** Fetch state from Portal Links */ - fetchLinkData(site) { + fetchLinkData(site, member) { const qParams = new URLSearchParams(window.location.search); - if (qParams.get('uuid') && qParams.get('action') === 'unsubscribe') { - return { - showPopup: true, - page: 'unsubscribe', - pageData: { - uuid: qParams.get('uuid'), - newsletterUuid: qParams.get('newsletter'), - comments: qParams.get('comments') - } - }; + if (qParams.get('action') === 'unsubscribe') { + // if the user is unsubscribing from a newsletter with an old unsubscribe link that we can't validate, push them to newsletter mgmt where they have to log in + if (qParams.get('key') && qParams.get('uuid')) { + return { + showPopup: true, + page: 'unsubscribe', + pageData: { + uuid: qParams.get('uuid'), + key: qParams.get('key'), + newsletterUuid: qParams.get('newsletter'), + comments: qParams.get('comments') + } + }; + } else { // any malformed unsubscribe links should simply go to email prefs + return { + showPopup: true, + page: 'accountEmail', + pageData: { + newsletterUuid: qParams.get('newsletter'), + action: 'unsubscribe', + redirect: site.url + '#/portal/account/newsletters' + } + }; + } } if (hasRecommendations({site}) && qParams.get('action') === 'signup' && qParams.get('success') === 'true') { @@ -453,19 +467,31 @@ export default class App extends React.Component { const linkRegex = /^\/portal\/?(?:\/(\w+(?:\/\w+)*))?\/?$/; const feedbackRegex = /^\/feedback\/(\w+?)\/(\w+?)\/?$/; - if (path && feedbackRegex.test(path) && hashQuery.get('uuid')) { + if (path && feedbackRegex.test(path)) { const [, postId, scoreString] = path.match(feedbackRegex); const score = parseInt(scoreString); if (score === 1 || score === 0) { - return { - showPopup: true, - page: 'feedback', - pageData: { - uuid: hashQuery.get('uuid'), - postId, - score - } - }; + // if logged in, submit feedback + if (member || (hashQuery.get('uuid') && hashQuery.get('key'))) { + return { + showPopup: true, + page: 'feedback', + pageData: { + uuid: member ? null : hashQuery.get('uuid'), + key: member ? null : hashQuery.get('key'), + postId, + score + } + }; + } else { + return { + showPopup: true, + page: 'signin', + pageData: { + redirect: site.url + `#/feedback/${postId}/${score}/` + } + }; + } } } if (path && linkRegex.test(path)) { diff --git a/apps/portal/src/components/pages/AccountEmailPage.js b/apps/portal/src/components/pages/AccountEmailPage.js index ed31a676f4..f8466b0caa 100644 --- a/apps/portal/src/components/pages/AccountEmailPage.js +++ b/apps/portal/src/components/pages/AccountEmailPage.js @@ -1,22 +1,84 @@ import AppContext from '../../AppContext'; import {useContext, useEffect, useState} from 'react'; -import {isPaidMember} from '../../utils/helpers'; +import {isPaidMember, getSiteNewsletters} from '../../utils/helpers'; +import {SYNTAX_I18NEXT} from '@doist/react-interpolate'; import NewsletterManagement from '../common/NewsletterManagement'; +import Interpolate from '@doist/react-interpolate'; export default function AccountEmailPage() { - const {member, onAction, site, t} = useContext(AppContext); + const {member, onAction, site, t, pageData} = useContext(AppContext); + let newsletterUuid; + let action; + if (pageData) { + newsletterUuid = pageData.newsletterUuid; + action = pageData.action; + } + const [hasInteracted, setHasInteracted] = useState(true); + const siteNewsletters = getSiteNewsletters({site}); + // Redirect to signin page if member is not available useEffect(() => { if (!member) { onAction('switchPage', { - page: 'signin', - pageData: { - redirect: window.location.href // This includes the search/fragment of the URL (#/portal/account) which is missing from the default referer header - } + page: 'signin' }); } }, [member, onAction]); + // this results in an infinite loop, needs to run only once... + useEffect(() => { + // attempt auto-unsubscribe if we were redirected here from an unsubscribe link + if (newsletterUuid && action === 'unsubscribe') { + // Filter out the newsletter that matches the uuid + const remainingNewsletterSubscriptions = member?.newsletters.filter(n => n.uuid !== newsletterUuid); + setSubscribedNewsletters(remainingNewsletterSubscriptions); + setHasInteracted(false); // this shows the dialog + onAction('updateNewsletterPreference', {newsletters: remainingNewsletterSubscriptions}); + } + }, []); + + const HeaderNotification = () => { + if (pageData.comments && commentsEnabled) { + const hideClassName = hasInteracted ? 'gh-portal-hide' : ''; + return ( + <> +

+ {member?.email} + }} + /> +

+ + ); + } + const unsubscribedNewsletter = siteNewsletters?.find((d) => { + return d.uuid === pageData.newsletterUuid; + }); + + if (!unsubscribedNewsletter) { + return null; + } + + const hideClassName = hasInteracted ? 'gh-portal-hide' : ''; + return ( + <> +

+ {member?.email}, + newsletterName: {unsubscribedNewsletter?.name} + }} + /> +

+ + ); + }; + const defaultSubscribedNewsletters = [...(member?.newsletters || [])]; const [subscribedNewsletters, setSubscribedNewsletters] = useState(defaultSubscribedNewsletters); const {comments_enabled: commentsEnabled} = site; @@ -28,7 +90,7 @@ export default function AccountEmailPage() { return ( { setSubscribedNewsletters(updatedNewsletters); diff --git a/apps/portal/src/components/pages/AccountEmailPage.test.js b/apps/portal/src/components/pages/AccountEmailPage.test.js index 8bd6a2f082..87c5f8ea57 100644 --- a/apps/portal/src/components/pages/AccountEmailPage.test.js +++ b/apps/portal/src/components/pages/AccountEmailPage.test.js @@ -112,6 +112,6 @@ describe('Account Email Page', () => { newsletters: newsletterData }); const {mockOnActionFn} = setup({site: siteData, member: null}); - expect(mockOnActionFn).toHaveBeenCalledWith('switchPage', {page: 'signin', pageData: {redirect: window.location.href}}); + expect(mockOnActionFn).toHaveBeenCalledWith('switchPage', {page: 'signin'}); }); }); diff --git a/apps/portal/src/components/pages/FeedbackPage.js b/apps/portal/src/components/pages/FeedbackPage.js index 39e701cd41..1720ac52d4 100644 --- a/apps/portal/src/components/pages/FeedbackPage.js +++ b/apps/portal/src/components/pages/FeedbackPage.js @@ -258,9 +258,9 @@ const ConfirmDialog = ({onConfirm, loading, initialScore}) => { ); }; -async function sendFeedback({siteUrl, uuid, postId, score}) { - const ghostApi = setupGhostApi({siteUrl}); - await ghostApi.feedback.add({uuid, postId, score}); +async function sendFeedback({siteUrl, uuid, key, postId, score}, api) { + const ghostApi = api || setupGhostApi({siteUrl}); + await ghostApi.feedback.add({uuid, postId, key, score}); } const LoadingFeedbackView = ({action, score}) => { @@ -301,11 +301,10 @@ const ConfirmFeedback = ({positive}) => { }; export default function FeedbackPage() { - const {site, pageData, member, t} = useContext(AppContext); - const {uuid, postId, score: initialScore} = pageData; + const {site, pageData, member, t, api} = useContext(AppContext); + const {uuid, key, postId, score: initialScore} = pageData; const [score, setScore] = useState(initialScore); const positive = score === 1; - const isLoggedIn = !!member; const [confirmed, setConfirmed] = useState(isLoggedIn); @@ -315,7 +314,7 @@ export default function FeedbackPage() { const doSendFeedback = async (selectedScore) => { setLoading(true); try { - await sendFeedback({siteUrl: site.url, uuid, postId, score: selectedScore}); + await sendFeedback({siteUrl: site.url, uuid, key, postId, score: selectedScore}, api); setScore(selectedScore); } catch (e) { const text = HumanReadableError.getMessageFromError(e, t('There was a problem submitting your feedback. Please try again a little later.')); @@ -341,6 +340,5 @@ export default function FeedbackPage() { return ; } } - return (); } diff --git a/apps/portal/src/components/pages/FeedbackPage.test.js b/apps/portal/src/components/pages/FeedbackPage.test.js new file mode 100644 index 0000000000..729ed34301 --- /dev/null +++ b/apps/portal/src/components/pages/FeedbackPage.test.js @@ -0,0 +1,40 @@ +import {getMemberData, getSiteData} from '../../utils/fixtures-generator'; +import {render} from '../../utils/test-utils'; +import FeedbackPage from './FeedbackPage'; + +const setup = (overrides) => { + const {mockOnActionFn, ...utils} = render( + , + { + overrideContext: { + ...overrides + } + } + ); + return { + mockOnActionFn, + ...utils + }; +}; + +describe('FeedbackPage', () => { + const siteData = getSiteData(); + const posts = siteData.posts; + const member = getMemberData(); + + // we need the API to actually test the component, so the bulk of tests will be in the FeedbackFlow file + test('renders', () => { + // mock what the larger app would process and set + const pageData = { + uuid: member.uuid, + key: 'key', + postId: posts[0].id, + score: 1 + }; + const {getByTestId} = setup({pageData}); + + const loaderIcon = getByTestId('loaderIcon'); + + expect(loaderIcon).toBeInTheDocument(); + }); +}); diff --git a/apps/portal/src/components/pages/LoadingPage.js b/apps/portal/src/components/pages/LoadingPage.js index 40778ef3c8..eafa7e6e64 100644 --- a/apps/portal/src/components/pages/LoadingPage.js +++ b/apps/portal/src/components/pages/LoadingPage.js @@ -6,7 +6,7 @@ export default class LoadingPage extends React.Component { return (
- +
); diff --git a/apps/portal/src/components/pages/UnsubscribePage.js b/apps/portal/src/components/pages/UnsubscribePage.js index 04bb6f1b5e..8717509a21 100644 --- a/apps/portal/src/components/pages/UnsubscribePage.js +++ b/apps/portal/src/components/pages/UnsubscribePage.js @@ -32,9 +32,9 @@ function AccountHeader() { ); } -async function updateMemberNewsletters({api, memberUuid, newsletters, enableCommentNotifications}) { +async function updateMemberNewsletters({api, memberUuid, key, newsletters, enableCommentNotifications}) { try { - return await api.member.updateNewsletters({uuid: memberUuid, newsletters, enableCommentNotifications}); + return await api.member.updateNewsletters({uuid: memberUuid, key, newsletters, enableCommentNotifications}); } catch (e) { // ignore auto unsubscribe error } @@ -62,7 +62,7 @@ export default function UnsubscribePage() { // when we have a member logged in, we need to update the newsletters in the context onAction('updateNewsletterPreference', {newsletters}); } else { - await updateMemberNewsletters({api, memberUuid: pageData.uuid, newsletters}); + await updateMemberNewsletters({api, memberUuid: pageData.uuid, key: pageData.key, newsletters}); } setSubscribedNewsletters(newsletters); }; @@ -74,7 +74,7 @@ export default function UnsubscribePage() { await onAction('updateNewsletterPreference', {enableCommentNotifications: enabled}); updatedData = {...loggedInMember, enable_comment_notifications: enabled}; } else { - updatedData = await updateMemberNewsletters({api, memberUuid: pageData.uuid, enableCommentNotifications: enabled}); + updatedData = await updateMemberNewsletters({api, memberUuid: pageData.uuid, key: pageData.key, enableCommentNotifications: enabled}); } setMember(updatedData); }; @@ -102,7 +102,7 @@ export default function UnsubscribePage() { (async () => { let memberData; try { - memberData = await api.member.newsletters({uuid: pageData.uuid}); + memberData = await api.member.newsletters({uuid: pageData.uuid, key: pageData.key}); setMember(memberData ?? null); setLoading(false); } catch (e) { diff --git a/apps/portal/src/tests/EmailSubscriptionsFlow.test.js b/apps/portal/src/tests/EmailSubscriptionsFlow.test.js index e77fb33c27..90abd15c5b 100644 --- a/apps/portal/src/tests/EmailSubscriptionsFlow.test.js +++ b/apps/portal/src/tests/EmailSubscriptionsFlow.test.js @@ -1,5 +1,5 @@ import App from '../App.js'; -import {appRender, fireEvent, within} from '../utils/test-utils'; +import {appRender, fireEvent, within, waitFor} from '../utils/test-utils'; import {newsletters as Newsletters, site as FixtureSite, member as FixtureMember} from '../utils/test-fixtures'; import setupGhostApi from '../utils/api.js'; import userEvent from '@testing-library/user-event'; @@ -91,14 +91,17 @@ describe('Newsletter Subscriptions', () => { // unsure why fireEvent has no effect here await userEvent.click(manageSubscriptionsButton); - - const newsletter1 = within(popupIframeDocument).queryByText('Newsletter 1'); - const newsletter2 = within(popupIframeDocument).queryByText('Newsletter 2'); - const emailPreferences = within(popupIframeDocument).queryByText('Email preferences'); - - expect(newsletter1).toBeInTheDocument(); - expect(newsletter2).toBeInTheDocument(); - expect(emailPreferences).toBeInTheDocument(); + + await waitFor(() => { + const newsletter1 = within(popupIframeDocument).queryByText('Newsletter 1'); + const newsletter2 = within(popupIframeDocument).queryByText('Newsletter 2'); + const emailPreferences = within(popupIframeDocument).queryByText('Email preferences'); + + // within(popupIframeDocument).getByText('dslkfjsdlk'); + expect(newsletter1).toBeInTheDocument(); + expect(newsletter2).toBeInTheDocument(); + expect(emailPreferences).toBeInTheDocument(); + }); }); test('toggle subscribing to a newsletter', async () => { @@ -176,7 +179,7 @@ describe('Newsletter Subscriptions', () => { test('unsubscribe via email link while not logged in', async () => { // Mock window.location Object.defineProperty(window, 'location', { - value: new URL(`https://portal.localhost/?action=unsubscribe&uuid=${FixtureMember.subbedToNewsletter.uuid}&newsletter=${Newsletters[0].uuid}`), + value: new URL(`https://portal.localhost/?action=unsubscribe&uuid=${FixtureMember.subbedToNewsletter.uuid}&newsletter=${Newsletters[0].uuid}&key=hashedMemberUuid`), writable: true }); @@ -186,10 +189,16 @@ describe('Newsletter Subscriptions', () => { newsletters: Newsletters }, true); + // Verify the API was hit to collect subscribed newsletters expect(ghostApi.member.newsletters).toHaveBeenLastCalledWith( - {uuid: FixtureMember.subbedToNewsletter.uuid} + { + uuid: FixtureMember.subbedToNewsletter.uuid, + key: 'hashedMemberUuid' + } ); expect(popupFrame).toBeInTheDocument(); + + expect(within(popupIframeDocument).getByText(/will no longer receive/)).toBeInTheDocument(); // Verify the local state shows the newsletter as unsubscribed let newsletterToggles = within(popupIframeDocument).queryAllByTestId('checkmark-container'); let newsletter1Toggle = newsletterToggles[0]; @@ -204,7 +213,7 @@ describe('Newsletter Subscriptions', () => { test('unsubscribe via email link while logged in', async () => { // Mock window.location Object.defineProperty(window, 'location', { - value: new URL(`https://portal.localhost/?action=unsubscribe&uuid=${FixtureMember.subbedToNewsletter.uuid}&newsletter=${Newsletters[0].uuid}`), + value: new URL(`https://portal.localhost/?action=unsubscribe&uuid=${FixtureMember.subbedToNewsletter.uuid}&newsletter=${Newsletters[0].uuid}&key=hashedMemberUuid`), writable: true }); @@ -216,13 +225,18 @@ describe('Newsletter Subscriptions', () => { // Verify the API was hit to collect subscribed newsletters expect(ghostApi.member.newsletters).toHaveBeenLastCalledWith( - {uuid: FixtureMember.subbedToNewsletter.uuid} + { + uuid: FixtureMember.subbedToNewsletter.uuid, + key: 'hashedMemberUuid' + } ); // Verify the local state shows the newsletter as unsubscribed let newsletterToggles = within(popupIframeDocument).queryAllByTestId('checkmark-container'); let newsletter1Toggle = newsletterToggles[0]; let newsletter2Toggle = newsletterToggles[1]; + expect(within(popupIframeDocument).getByText(/will no longer receive/)).toBeInTheDocument(); + expect(newsletter1Toggle).toBeInTheDocument(); expect(newsletter2Toggle).toBeInTheDocument(); expect(newsletter1Toggle).not.toHaveClass('gh-portal-toggle-checked'); @@ -253,5 +267,26 @@ describe('Newsletter Subscriptions', () => { expect(newsletter1Toggle).not.toHaveClass('gh-portal-toggle-checked'); expect(newsletter2Toggle).toHaveClass('gh-portal-toggle-checked'); }); + + test('unsubscribe link without a key param', async () => { + // Mock window.location + Object.defineProperty(window, 'location', { + value: new URL(`https://portal.localhost/?action=unsubscribe&uuid=${FixtureMember.subbedToNewsletter.uuid}&newsletter=${Newsletters[0].uuid}`), + writable: true + }); + + const {ghostApi, popupFrame, popupIframeDocument} = await setup({ + site: FixtureSite.singleTier.onlyFreePlanWithoutStripe, + member: FixtureMember.subbedToNewsletter, + newsletters: Newsletters + }, true); + + // Verify the popup frame is not shown + expect(popupFrame).toBeInTheDocument(); + // Verify the API was hit to collect subscribed newsletters + expect(ghostApi.member.newsletters).not.toHaveBeenCalled(); + // expect sign in page + expect(within(popupIframeDocument).queryByText('Sign in')).toBeInTheDocument(); + }); }); }); diff --git a/apps/portal/src/tests/FeedbackFlow.test.js b/apps/portal/src/tests/FeedbackFlow.test.js new file mode 100644 index 0000000000..14d231d22a --- /dev/null +++ b/apps/portal/src/tests/FeedbackFlow.test.js @@ -0,0 +1,181 @@ +import App from '../App.js'; +import {appRender, fireEvent, waitFor, within} from '../utils/test-utils'; +import setupGhostApi from '../utils/api.js'; +import {getMemberData, getPostsData, getSiteData} from '../utils/fixtures-generator.js'; + +const siteData = getSiteData(); +const memberData = getMemberData(); +const posts = getPostsData(); +const postSlug = posts[0].slug; +const postId = posts[0].id; + +const setup = async (site = siteData, member = memberData, loggedOut = false, api = {}) => { + const ghostApi = setupGhostApi({siteUrl: site.url}); + ghostApi.init = api?.init || jest.fn(() => { + return Promise.resolve({ + site, + member: loggedOut ? null : member + }); + }); + ghostApi.feedback.add = api?.add || jest.fn(() => { + return Promise.resolve({ + feedback: [ + { + id: 1, + postId: 1, + memberId: member ? member.uuid : null, + score: 1 + } + ] + }); + }); + + const utils = appRender( + + ); + + // Note: this await is CRITICAL otherwise the iframe won't be loaded + const popupFrame = await utils.findByTitle(/portal-popup/i); + const popupIframeDocument = popupFrame.contentDocument; + + return { + ghostApi, + popupIframeDocument, + popupFrame, + ...utils + }; +}; + +describe('Feedback Submission Flow', () => { + describe('Valid feedback URL', () => { + describe('Logged in', () => { + test('Autosubmits feedback', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/?uuid=${memberData.uuid}&key=key`), + writable: true + }); + + const {ghostApi, popupFrame, popupIframeDocument} = await setup(); + + expect(popupFrame).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(1); + + within(popupIframeDocument).getByText('Thanks for the feedback!'); + within(popupIframeDocument).getByText('Your input helps shape what gets published.'); + }); + + test('Autosubmits feedback w/o uuid or key params', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/`), + writable: true + }); + const {ghostApi, popupFrame, popupIframeDocument} = await setup(); + + expect(popupFrame).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(1); + within(popupIframeDocument).getByText('Thanks for the feedback!'); + within(popupIframeDocument).getByText('Your input helps shape what gets published.'); + }); + }); + + describe('Logged out', () => { + test('Requires confirmation', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/?uuid=${memberData.uuid}&key=key`), + writable: true + }); + const {ghostApi, popupFrame, popupIframeDocument} = await setup(siteData, null, true); + + expect(popupFrame).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('Give feedback on this post')).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('More like this')).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText('Less like this')).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(0); + + const submitBtn = within(popupIframeDocument).getByText('Submit feedback'); + fireEvent.click(submitBtn); + + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(1); + + // the re-render loop is slow to get to the final state + await waitFor(() => { + within(popupIframeDocument).getByText('Thanks for the feedback!'); + within(popupIframeDocument).getByText('Your input helps shape what gets published.'); + }); + }); + + test('Requires login without key', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/?uuid=${memberData.uuid}`), + writable: true + }); + const {ghostApi, popupFrame, popupIframeDocument} = await setup(siteData, null, true); + + expect(popupFrame).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(0); + expect(within(popupIframeDocument).getByText(/Sign in/)).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/Sign up/)).toBeInTheDocument(); + }); + + test('Requires login without uuid or key', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/`), + writable: true + }); + const {ghostApi, popupFrame, popupIframeDocument} = await setup(siteData, null, true); + + expect(popupFrame).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(0); + expect(within(popupIframeDocument).getByText(/Sign in/)).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/Sign up/)).toBeInTheDocument(); + }); + }); + + test('Error on fail to submit', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/${postSlug}/#/feedback/${postId}/1/?uuid=${memberData.uuid}&key=key`), + writable: true + }); + const mockApi = { + add: jest.fn(() => { + return Promise.reject(new Error('Failed to submit feedback')); + }) + }; + const {ghostApi, popupFrame, popupIframeDocument} = await setup(siteData, memberData, false, mockApi); + + expect(popupFrame).toBeInTheDocument(); + expect(ghostApi.feedback.add).toHaveBeenCalledTimes(1); + expect(within(popupIframeDocument).getByText(/Sorry/)).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/There was a problem submitting your feedback/)).toBeInTheDocument(); + }); + }); + + describe('Invalid feedback URL', () => { + test('Redirects logged in members to account settings', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/postslughere/#/feedback/1/1/1/`), + writable: true + }); + const {popupFrame, popupIframeDocument} = await setup(); + + expect(popupFrame).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/Your account/)).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/Sign out/)).toBeInTheDocument(); + }); + + test('Redirects logged out users to sign up', async () => { + Object.defineProperty(window, 'location', { + value: new URL(`${siteData.url}/postslughere/#/feedback/1/1/1/`), + writable: true + }); + const {popupFrame, popupIframeDocument} = await setup(siteData, null, true); + + expect(popupFrame).toBeInTheDocument(); + // takes to sign up + await waitFor(() => { + expect(within(popupIframeDocument).getByText(/Name/)).toBeInTheDocument(); + expect(within(popupIframeDocument).getByText(/Email/)).toBeInTheDocument(); + }); + }); + }); +}); diff --git a/apps/portal/src/utils/api.js b/apps/portal/src/utils/api.js index 5ecc72430b..1fe06f485e 100644 --- a/apps/portal/src/utils/api.js +++ b/apps/portal/src/utils/api.js @@ -134,10 +134,11 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { }; api.feedback = { - async add({uuid, postId, score}) { + async add({uuid, key, postId, score}) { let url = endpointFor({type: 'members', resource: 'feedback'}); - url = url + `?uuid=${uuid}`; - + if (uuid && key) { // only necessary if not logged in, and both are required if so + url = url + `?uuid=${uuid}&key=${key}`; + } const body = { feedback: [ { @@ -303,9 +304,9 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { }); }, - async newsletters({uuid}) { + async newsletters({uuid, key}) { let url = endpointFor({type: 'members', resource: `member/newsletters`}); - url = url + `?uuid=${uuid}`; + url = url + `?uuid=${uuid}&key=${key}`; return makeRequest({ url, credentials: 'same-origin' @@ -317,9 +318,9 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}) { }); }, - async updateNewsletters({uuid, newsletters, enableCommentNotifications}) { + async updateNewsletters({uuid, newsletters, key, enableCommentNotifications}) { let url = endpointFor({type: 'members', resource: `member/newsletters`}); - url = url + `?uuid=${uuid}`; + url = url + `?uuid=${uuid}&key=${key}`; const body = { newsletters }; diff --git a/apps/portal/src/utils/fixtures-generator.js b/apps/portal/src/utils/fixtures-generator.js index 74adfd1369..d87f8fa530 100644 --- a/apps/portal/src/utils/fixtures-generator.js +++ b/apps/portal/src/utils/fixtures-generator.js @@ -41,6 +41,7 @@ export function getSiteData({ membersSupportAddress: members_support_address = 'support@example.com', editorDefaultEmailRecipients: editor_default_email_recipients = 'visibility', newsletters = [], + posts = getPostsData(), commentsEnabled, recommendations = [], recommendationsEnabled @@ -71,7 +72,8 @@ export function getSiteData({ newsletters, recommendations, recommendations_enabled: !!recommendationsEnabled, - editor_default_email_recipients + editor_default_email_recipients, + posts }; } @@ -175,6 +177,33 @@ export function getNewslettersData({numOfNewsletters = 3} = {}) { return newsletters.slice(0, numOfNewsletters); } +export function getPostsData({numOfPosts = 3} = {}) { + const posts = []; + for (let i = 0; i < numOfPosts; i++) { + posts.push(getPostData({ + title: `Post ${i + 1}`, + slug: `post-${i + 1}` + })); + } + return posts.slice(0, numOfPosts); +} + +export function getPostData({ + id = `post_${objectId()}`, + title = 'Post', + excerpt = 'Post excerpt', + slug = 'post', + featured = false +} = {}) { + return { + id, + title, + excerpt, + slug, + featured + }; +} + export function getProductsData({numOfProducts = 3} = {}) { const products = [ getProductData({ diff --git a/apps/portal/src/utils/fixtures.js b/apps/portal/src/utils/fixtures.js index 16ebb82739..baf43cd66c 100644 --- a/apps/portal/src/utils/fixtures.js +++ b/apps/portal/src/utils/fixtures.js @@ -121,6 +121,29 @@ export const site = getSiteData({ subscribe_on_signup: false, paid: false } + ], + posts: [ + { + id: 'post_66aacfe061c94e10eb6e4fc1', + title: 'Post 1', + excerpt: 'Post excerpt', + slug: 'post-1', + featured: false + }, + { + id: 'post_66aacfe04f14b8dbb56c5721', + title: 'Post 2', + excerpt: 'Post excerpt', + slug: 'post-2', + featured: false + }, + { + id: 'post_66aacfe03d609460819af18c', + title: 'Post 3', + excerpt: 'Post excerpt', + slug: 'post-3', + featured: false + } ] }); diff --git a/ghost/admin/package.json b/ghost/admin/package.json index e634a00c31..6e22ad5fe6 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.89.4", + "version": "5.89.5", "description": "Ember.js admin client for Ghost", "author": "Ghost Foundation", "homepage": "http://ghost.org", diff --git a/ghost/audience-feedback/lib/AudienceFeedbackService.js b/ghost/audience-feedback/lib/AudienceFeedbackService.js index cb262ee23a..f72cc0aa78 100644 --- a/ghost/audience-feedback/lib/AudienceFeedbackService.js +++ b/ghost/audience-feedback/lib/AudienceFeedbackService.js @@ -17,15 +17,16 @@ class AudienceFeedbackService { * @param {string} uuid * @param {string} postId * @param {0 | 1} score + * @param {string} key - hashed uuid value */ - buildLink(uuid, postId, score) { + buildLink(uuid, postId, score, key) { let postUrl = this.#urlService.getUrlByResourceId(postId, {absolute: true}); if (postUrl.match(/\/404\//)) { postUrl = this.#baseURL; } const url = new URL(postUrl); - url.hash = `#/feedback/${postId}/${score}/?uuid=${encodeURIComponent(uuid)}`; + url.hash = `#/feedback/${postId}/${score}/?uuid=${encodeURIComponent(uuid)}&key=${encodeURIComponent(key)}`; return url; } } diff --git a/ghost/audience-feedback/test/AudienceFeedbackService.test.js b/ghost/audience-feedback/test/AudienceFeedbackService.test.js index 8a85d54262..c744f510d6 100644 --- a/ghost/audience-feedback/test/AudienceFeedbackService.test.js +++ b/ghost/audience-feedback/test/AudienceFeedbackService.test.js @@ -10,7 +10,8 @@ describe('audienceFeedbackService', function () { uuid: '7b11de3c-dff9-4563-82ae-a281122d201d', postId: '634fc3901e0a291855d8b135', postTitle: 'somepost', - score: 1 + score: 1, + key: 'somekey' }; describe('build link', function () { @@ -23,8 +24,8 @@ describe('audienceFeedbackService', function () { baseURL: new URL('https://localhost:2368') } }); - const link = instance.buildLink(mockData.uuid, mockData.postId, mockData.score); - const expectedLink = `https://localhost:2368/${mockData.postTitle}/#/feedback/${mockData.postId}/${mockData.score}/?uuid=${mockData.uuid}`; + const link = instance.buildLink(mockData.uuid, mockData.postId, mockData.score, mockData.key); + const expectedLink = `https://localhost:2368/${mockData.postTitle}/#/feedback/${mockData.postId}/${mockData.score}/?uuid=${mockData.uuid}&key=somekey`; assert.equal(link.href, expectedLink); }); @@ -37,8 +38,8 @@ describe('audienceFeedbackService', function () { baseURL: new URL('https://localhost:2368') } }); - const link = instance.buildLink(mockData.uuid, mockData.postId, mockData.score); - const expectedLink = `https://localhost:2368/#/feedback/${mockData.postId}/${mockData.score}/?uuid=${mockData.uuid}`; + const link = instance.buildLink(mockData.uuid, mockData.postId, mockData.score, mockData.key); + const expectedLink = `https://localhost:2368/#/feedback/${mockData.postId}/${mockData.score}/?uuid=${mockData.uuid}&key=somekey`; assert.equal(link.href, expectedLink); }); }); diff --git a/ghost/core/core/frontend/services/routing/controllers/unsubscribe.js b/ghost/core/core/frontend/services/routing/controllers/unsubscribe.js index 7b730ade70..2fee46760a 100644 --- a/ghost/core/core/frontend/services/routing/controllers/unsubscribe.js +++ b/ghost/core/core/frontend/services/routing/controllers/unsubscribe.js @@ -3,6 +3,8 @@ const url = require('url'); const members = require('../../../../server/services/members'); const urlUtils = require('../../../../shared/url-utils'); const logging = require('@tryghost/logging'); +const settingsHelpers = require('../../../../server/services/settings-helpers'); +const crypto = require('crypto'); module.exports = async function unsubscribeController(req, res) { debug('unsubscribeController'); @@ -19,6 +21,17 @@ module.exports = async function unsubscribeController(req, res) { // Do an actual unsubscribe try { + if (!query.key) { + logging.warn('[List-Unsubscribe] Unsubscribe failed due to missing verification key for ' + query.uuid); + return res.status(400).end(); + } + const membersKey = settingsHelpers.getMembersValidationKey(); + const memberHmac = crypto.createHmac('sha256', membersKey).update(query.uuid).digest('hex'); + if (memberHmac !== query.key) { + logging.warn('[List-Unsubscribe] Unsubscribe failed due to invalid key for ' + query.uuid); + return res.status(400).end(); + } + const member = await members.api.members.get({uuid: query.uuid}, {withRelated: ['newsletters']}); if (member) { if (query.comments) { @@ -60,6 +73,9 @@ module.exports = async function unsubscribeController(req, res) { if (query.comments) { redirectUrl.searchParams.append('comments', query.comments); } + if (query.key) { + redirectUrl.searchParams.append('key', query.key); + } redirectUrl.searchParams.append('action', 'unsubscribe'); return res.redirect(302, redirectUrl.href); diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index e73bfd65f4..2c4278b0a9 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -15,10 +15,12 @@ const tpl = require('@tryghost/tpl'); const onHeaders = require('on-headers'); const tiersService = require('../tiers/service'); const config = require('../../../shared/config'); +const settingsHelpers = require('../settings-helpers'); const messages = { missingUuid: 'Missing uuid.', - invalidUuid: 'Invalid uuid.' + invalidUuid: 'Invalid uuid.', + invalidKey: 'Invalid key.' }; const getFreeTier = async function getFreeTier() { @@ -97,7 +99,7 @@ const loadMemberSession = async function loadMemberSession(req, res, next) { }; /** - * Require member authentication, and make it possible to authenticate via uuid. + * Require member authentication, and make it possible to authenticate via uuid + hashed key. * You can chain this after loadMemberSession to make it possible to authenticate via both the uuid and the session. */ const authMemberByUuid = async function authMemberByUuid(req, res, next) { @@ -114,6 +116,22 @@ const authMemberByUuid = async function authMemberByUuid(req, res, next) { }); } + const key = req.query.key; + if (!key) { + throw new errors.UnauthorizedError({ + message: tpl(messages.invalidKey) + }); + } + + // the request key is a hashed value from the member uuid and the members validation key so we can verify the source + // (only Ghost should be able to generate the key) + const memberHmac = crypto.createHmac('sha256', settingsHelpers.getMembersValidationKey()).update(uuid).digest('hex'); + if (memberHmac !== key) { + throw new errors.UnauthorizedError({ + message: tpl(messages.invalidKey) + }); + } + const member = await membersService.api.memberBREADService.read({uuid}); if (!member) { throw new errors.UnauthorizedError({ @@ -193,25 +211,14 @@ const deleteSuppression = async function deleteSuppression(req, res) { const getMemberNewsletters = async function getMemberNewsletters(req, res) { try { - const memberUuid = req.query.uuid; - - if (!memberUuid) { - res.writeHead(400); - return res.end('Invalid member uuid'); - } - - const memberData = await membersService.api.members.get({ - uuid: memberUuid - }, { - withRelated: ['newsletters'] - }); + const memberData = req.member; // validation assumed if (!memberData) { res.writeHead(404); return res.end('Email address not found.'); } - const data = _.pick(memberData.toJSON(), 'uuid', 'email', 'name', 'newsletters', 'enable_comment_notifications', 'status'); + const data = _.pick(memberData, 'uuid', 'email', 'name', 'newsletters', 'enable_comment_notifications', 'status'); if (data.newsletters) { data.newsletters = formatNewsletterResponse(data.newsletters); @@ -226,23 +233,15 @@ const getMemberNewsletters = async function getMemberNewsletters(req, res) { const updateMemberNewsletters = async function updateMemberNewsletters(req, res) { try { - const memberUuid = req.query.uuid; - if (!memberUuid) { - res.writeHead(400); - return res.end('Invalid member uuid'); - } - - const data = _.pick(req.body, 'newsletters', 'enable_comment_notifications'); - const memberData = await membersService.api.members.get({ - uuid: memberUuid - }); + const memberData = req.member; // validation assumed if (!memberData) { res.writeHead(404); return res.end('Email address not found.'); } + const data = _.pick(req.body, 'newsletters', 'enable_comment_notifications'); const options = { - id: memberData.get('id'), + id: memberData.id, withRelated: ['newsletters'] }; diff --git a/ghost/core/core/server/services/settings-helpers/SettingsHelpers.js b/ghost/core/core/server/services/settings-helpers/SettingsHelpers.js index bb59ba98c7..f11760976c 100644 --- a/ghost/core/core/server/services/settings-helpers/SettingsHelpers.js +++ b/ghost/core/core/server/services/settings-helpers/SettingsHelpers.js @@ -106,6 +106,16 @@ class SettingsHelpers { return domain; } + /** + * Retrieves the member validation key from the settings cache. The intent is for this key to be used where member + * auth is not required. For example, unsubscribe links in emails, which are required to be one-click unsubscribe. + * + * @returns {string} The member validation key. + */ + getMembersValidationKey() { + return this.settingsCache.get('members_email_auth_secret'); + } + getMembersSupportAddress() { let supportAddress = this.settingsCache.get('members_support_address'); diff --git a/ghost/core/core/server/web/members/app.js b/ghost/core/core/server/web/members/app.js index 9a3e8435b8..63552ec82f 100644 --- a/ghost/core/core/server/web/members/app.js +++ b/ghost/core/core/server/web/members/app.js @@ -40,9 +40,16 @@ module.exports = function setupMembersApp() { // Initializes members specific routes as well as assigns members specific data to the req/res objects // We don't want to add global bodyParser middleware as that interferes with stripe webhook requests on - `/webhooks`. - // Manage newsletter subscription via unsubscribe link - membersApp.get('/api/member/newsletters', middleware.getMemberNewsletters); - membersApp.put('/api/member/newsletters', bodyParser.json({limit: '50mb'}), middleware.updateMemberNewsletters); + // Manage newsletter subscription via unsubscribe link - these should be authenticated by uuid and hashed key + membersApp.get('/api/member/newsletters', + middleware.authMemberByUuid, + middleware.getMemberNewsletters + ); + membersApp.put('/api/member/newsletters', + bodyParser.json({limit: '50mb'}), + middleware.authMemberByUuid, + middleware.updateMemberNewsletters + ); // Get and update member data // Caching members content is an experimental feature diff --git a/ghost/core/core/shared/config/defaults.json b/ghost/core/core/shared/config/defaults.json index 006a350acc..cbbe2c3142 100644 --- a/ghost/core/core/shared/config/defaults.json +++ b/ghost/core/core/shared/config/defaults.json @@ -182,7 +182,7 @@ }, "portal": { "url": "https://cdn.jsdelivr.net/ghost/portal@~{version}/umd/portal.min.js", - "version": "2.38" + "version": "2.39" }, "sodoSearch": { "url": "https://cdn.jsdelivr.net/ghost/sodo-search@~{version}/umd/sodo-search.min.js", diff --git a/ghost/core/core/shared/config/env/config.development.json b/ghost/core/core/shared/config/env/config.development.json index d9551493f5..662af14c91 100644 --- a/ghost/core/core/shared/config/env/config.development.json +++ b/ghost/core/core/shared/config/env/config.development.json @@ -35,4 +35,4 @@ "maxAge": 0 } } -} +} \ No newline at end of file diff --git a/ghost/core/package.json b/ghost/core/package.json index f7664bf119..6090969018 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.89.4", + "version": "5.89.5", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org", diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap index ab36fdd179..9d567e3baf 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snap @@ -605,7 +605,7 @@ table.body h2 span { - + @@ -749,7 +749,7 @@ Another email card with a similar replacement, Jamie -Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=requested-newsletter-uuid] @@ -778,7 +778,7 @@ exports[`Email Preview API Read can read post email preview with email card and Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "24595", + "content-length": "24733", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -1302,7 +1302,7 @@ table.body h2 span {
Ghost © 2024 – UnsubscribeGhost © 2024 – Unsubscribe
- + @@ -1463,7 +1463,7 @@ Header Level 3 -Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=requested-newsletter-uuid] @@ -1492,7 +1492,7 @@ exports[`Email Preview API Read can read post email preview with fields 4: [head Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "29375", + "content-length": "29513", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -2042,7 +2042,7 @@ table.body h2 span {
Ghost © 2024 – UnsubscribeGhost © 2024 – Unsubscribe
- + @@ -2180,7 +2180,7 @@ Testing links [https://ghost.org/] in email excerpt and apostrophes ' -Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=requested-newsletter-uuid] @@ -2222,7 +2222,7 @@ exports[`Email Preview API Read has custom content transformations for email com Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "24349", + "content-length": "24487", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -3108,7 +3108,7 @@ table.body h2 span {
Ghost © 2024 – UnsubscribeGhost © 2024 – Unsubscribe
- + @@ -3253,7 +3253,7 @@ Testing links [https://ghost.org/] in email excerpt and apostrophes ' -Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=requested-newsletter-uuid] @@ -3295,7 +3295,7 @@ exports[`Email Preview API Read uses the newsletter provided through ?newsletter Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "25132", + "content-length": "25270", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -4207,7 +4207,7 @@ table.body h2 span {
Ghost © 2024 – UnsubscribeGhost © 2024 – Unsubscribe
- + @@ -4352,7 +4352,7 @@ Testing links [https://ghost.org/] in email excerpt and apostrophes ' -Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=requested-newsletter-uuid] +Ghost © 2024 – Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=requested-newsletter-uuid] @@ -4394,7 +4394,7 @@ exports[`Email Preview API Read uses the posts newsletter by default 4: [headers Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "25132", + "content-length": "25270", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap index c6f86209e5..1702fc2649 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/emails.test.js.snap @@ -790,12 +790,12 @@ Object { "failed_count": 0, "feedback_enabled": false, "from": "support@example.com", - "html": "

Hey Jamie, Hey Jamie,

Unsubscribe", + "html": "

Hey Jamie, Hey Jamie,

Unsubscribe", "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "newsletter_id": StringMatching /\\[a-f0-9\\]\\{24\\}/, "opened_count": 0, "plaintext": "Hey Jamie, Hey Jamie -Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&newsletter=preview]", +Unsubscribe [http://127.0.0.1:2369/unsubscribe/?uuid=example-uuid&key=803e513e2d8b88e759d8f433779659af335d7308b4cbac809600d563f6b49a76&newsletter=preview]", "post_id": "618ba1ffbe2896088840a6e3", "recipient_filter": "all", "reply_to": null, @@ -817,7 +817,7 @@ exports[`Emails API Does default replacements on the HTML body of an old email 2 Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "923", + "content-length": "1061", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/email-previews.test.js b/ghost/core/test/e2e-api/admin/email-previews.test.js index a3d66da487..1b5567d4ee 100644 --- a/ghost/core/test/e2e-api/admin/email-previews.test.js +++ b/ghost/core/test/e2e-api/admin/email-previews.test.js @@ -4,6 +4,7 @@ const assert = require('assert/strict'); const sinon = require('sinon'); const escapeRegExp = require('lodash/escapeRegExp'); const should = require('should'); +const settingsHelpers = require('../../../core/server/services/settings-helpers'); // @TODO: factor out these requires const ObjectId = require('bson-objectid').default; @@ -37,6 +38,7 @@ describe('Email Preview API', function () { beforeEach(function () { mockManager.mockMailgun(); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test-validation-key'); }); before(async function () { diff --git a/ghost/core/test/e2e-api/admin/emails.test.js b/ghost/core/test/e2e-api/admin/emails.test.js index 7f49929930..31b24b1edf 100644 --- a/ghost/core/test/e2e-api/admin/emails.test.js +++ b/ghost/core/test/e2e-api/admin/emails.test.js @@ -4,6 +4,7 @@ const assert = require('assert/strict'); const sinon = require('sinon'); const jobManager = require('../../../core/server/services/jobs/job-service'); const models = require('../../../core/server/models'); +const settingsHelpers = require('../../../core/server/services/settings-helpers'); const matchEmail = { id: anyObjectId, @@ -43,6 +44,7 @@ describe('Emails API', function () { beforeEach(function () { mockManager.mockEvents(); mockManager.mockMailgun(); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test-validation-key'); }); afterEach(function () { diff --git a/ghost/core/test/e2e-api/members/__snapshots__/feedback.test.js.snap b/ghost/core/test/e2e-api/members/__snapshots__/feedback.test.js.snap index d9487d896b..997655f492 100644 --- a/ghost/core/test/e2e-api/members/__snapshots__/feedback.test.js.snap +++ b/ghost/core/test/e2e-api/members/__snapshots__/feedback.test.js.snap @@ -26,6 +26,104 @@ Object { } `; +exports[`Members Feedback Authentication Allows authentication via uuid (+ key) 1: [body] 1`] = ` +Object { + "feedback": Array [ + Object { + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "memberId": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "postId": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "score": 1, + }, + ], +} +`; + +exports[`Members Feedback Authentication Allows authentication via uuid (+ key) 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "*", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "132", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "location": StringMatching /https\\?:\\\\/\\\\/\\.\\*\\?\\\\/feedback\\\\/\\[a-f0-9\\]\\{24\\}\\\\//, + "vary": "Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Members Feedback Authentication Thorws for invalid key 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "Invalid key.", + "property": null, + "type": "UnauthorizedError", + }, + ], +} +`; + +exports[`Members Feedback Authentication Throws for invalid uuid 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "Invalid key.", + "property": null, + "type": "UnauthorizedError", + }, + ], +} +`; + +exports[`Members Feedback Authentication Throws for missing key 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "Invalid key.", + "property": null, + "type": "UnauthorizedError", + }, + ], +} +`; + +exports[`Members Feedback Authentication Throws for nonexisting uuid 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": null, + "details": null, + "ghostErrorCode": null, + "help": null, + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "Invalid key.", + "property": null, + "type": "UnauthorizedError", + }, + ], +} +`; + exports[`Members Feedback Can add feedback 1: [body] 1`] = ` Object { "feedback": Array [ diff --git a/ghost/core/test/e2e-api/members/feedback.test.js b/ghost/core/test/e2e-api/members/feedback.test.js index 490db7954e..f98908325c 100644 --- a/ghost/core/test/e2e-api/members/feedback.test.js +++ b/ghost/core/test/e2e-api/members/feedback.test.js @@ -3,9 +3,13 @@ const {agentProvider, mockManager, fixtureManager, matchers, configUtils} = requ const {anyEtag, anyObjectId, anyLocationFor, anyErrorId} = matchers; const models = require('../../../core/server/models'); const sinon = require('sinon'); +const settingsHelpers = require('../../../core/server/services/settings-helpers'); +const crypto = require('crypto'); + +const membersValidationKeyMock = 'abc123dontstealme'; describe('Members Feedback', function () { - let membersAgent, membersAgent2, memberUuid; + let membersAgent, membersAgent2, memberUuid, memberHmac; let clock; before(async function () { @@ -14,9 +18,11 @@ describe('Members Feedback', function () { await fixtureManager.init('posts', 'members'); memberUuid = fixtureManager.get('members', 0).uuid; + memberHmac = crypto.createHmac('sha256', membersValidationKeyMock).update(memberUuid).digest('hex'); }); beforeEach(function () { + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns(membersValidationKeyMock); mockManager.mockMail(); }); @@ -55,21 +61,45 @@ describe('Members Feedback', function () { ] }); }); - }); - describe('Validation', function () { - const postId = fixtureManager.get('posts', 0).id; + it('Allows authentication via uuid (+ key)', async function () { + const postId = fixtureManager.get('posts', 0).id; - it('Throws for invalid score', async function () { + await membersAgent + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) + .body({ + feedback: [{ + score: 1, + post_id: postId + }] + }) + .expectStatus(201) + .matchHeaderSnapshot({ + etag: anyEtag, + location: anyLocationFor('feedback') + }) + .matchBodySnapshot({ + feedback: [ + { + id: anyObjectId, + memberId: anyObjectId, + postId: anyObjectId + } + ] + }); + }); + + it('Throws for missing key', async function () { + const postId = fixtureManager.get('posts', 0).id; await membersAgent .post(`/api/feedback/?uuid=${memberUuid}`) .body({ feedback: [{ - score: 2, + score: 1, post_id: postId }] }) - .expectStatus(422) + .expectStatus(401) .matchBodySnapshot({ errors: [ { @@ -79,16 +109,17 @@ describe('Members Feedback', function () { }); }); - it('Throws for invalid score type', async function () { + it('Thorws for invalid key', async function () { + const postId = fixtureManager.get('posts', 0).id; await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=1234`) .body({ feedback: [{ - score: 'text', + score: 1, post_id: postId }] }) - .expectStatus(422) + .expectStatus(401) .matchBodySnapshot({ errors: [ { @@ -99,6 +130,7 @@ describe('Members Feedback', function () { }); it('Throws for invalid uuid', async function () { + const postId = fixtureManager.get('posts', 0).id; await membersAgent .post(`/api/feedback/?uuid=1234`) .body({ @@ -118,6 +150,7 @@ describe('Members Feedback', function () { }); it('Throws for nonexisting uuid', async function () { + const postId = fixtureManager.get('posts', 0).id; const uuid = '00000000-0000-0000-0000-000000000000'; await membersAgent .post(`/api/feedback/?uuid=${uuid}`) @@ -136,10 +169,52 @@ describe('Members Feedback', function () { ] }); }); + }); + + describe('Validation', function () { + const postId = fixtureManager.get('posts', 0).id; + + it('Throws for invalid score', async function () { + await membersAgent + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) + .body({ + feedback: [{ + score: 2, + post_id: postId + }] + }) + .expectStatus(422) + .matchBodySnapshot({ + errors: [ + { + id: anyErrorId + } + ] + }); + }); + + it('Throws for invalid score type', async function () { + await membersAgent + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) + .body({ + feedback: [{ + score: 'text', + post_id: postId + }] + }) + .expectStatus(422) + .matchBodySnapshot({ + errors: [ + { + id: anyErrorId + } + ] + }); + }); it('Throws for nonexisting post', async function () { await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) .body({ feedback: [{ score: 1, @@ -161,7 +236,7 @@ describe('Members Feedback', function () { const postId = fixtureManager.get('posts', 0).id; await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) .body({ feedback: [{ score: 1, @@ -189,7 +264,7 @@ describe('Members Feedback', function () { const postId = fixtureManager.get('posts', 1).id; const {body} = await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) .body({ feedback: [{ score: 0, @@ -220,7 +295,7 @@ describe('Members Feedback', function () { clock.tick(10 * 60 * 1000); const {body: body2} = await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) .body({ feedback: [{ score: 1, @@ -252,7 +327,7 @@ describe('Members Feedback', function () { // Do the same change again, and the model shouldn't change const {body: body3} = await membersAgent - .post(`/api/feedback/?uuid=${memberUuid}`) + .post(`/api/feedback/?uuid=${memberUuid}&key=${memberHmac}`) .body({ feedback: [{ score: 1, diff --git a/ghost/core/test/e2e-api/members/middleware.test.js b/ghost/core/test/e2e-api/members/middleware.test.js index 7ce1e96bfb..88a0384933 100644 --- a/ghost/core/test/e2e-api/members/middleware.test.js +++ b/ghost/core/test/e2e-api/members/middleware.test.js @@ -3,6 +3,8 @@ const {agentProvider, mockManager, fixtureManager, matchers, configUtils} = requ const {anyEtag, anyObjectId, anyUuid, anyISODateTime, stringMatching} = matchers; const models = require('../../../core/server/models'); const should = require('should'); +const sinon = require('sinon'); +const settingsHelpers = require('../../../core/server/services/settings-helpers'); let membersAgent; @@ -60,8 +62,11 @@ describe('Comments API', function () { let member = await models.Member.findOne({id: fixtureManager.get('members', 0).id}, {require: true}); member.get('enable_comment_notifications').should.eql(true, 'This test requires the initial value to be true'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const hmac = crypto.createHmac('sha256', 'test').update(member.get('uuid')).digest('hex'); + await membersAgent - .put(`/api/member/newsletters/?uuid=${member.get('uuid')}`) + .put(`/api/member/newsletters/?uuid=${member.get('uuid')}&key=${hmac}`) .body({ enable_comment_notifications: false }) @@ -179,9 +184,12 @@ describe('Comments API', function () { member = await models.Member.findOne({id: member.id}, {require: true}); member.get('enable_comment_notifications').should.eql(false); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const hmac = crypto.createHmac('sha256', 'test').update(member.get('uuid')).digest('hex'); + // Via updateMemberNewsletters await membersAgent - .put(`/api/member/newsletters/?uuid=${member.get('uuid')}`) + .put(`/api/member/newsletters/?uuid=${member.get('uuid')}&key=${hmac}`) .body({ enable_comment_notifications: true }) diff --git a/ghost/core/test/e2e-frontend/members.test.js b/ghost/core/test/e2e-frontend/members.test.js index 80ed4e0c29..92cdb17716 100644 --- a/ghost/core/test/e2e-frontend/members.test.js +++ b/ghost/core/test/e2e-frontend/members.test.js @@ -6,12 +6,14 @@ const moment = require('moment'); const testUtils = require('../utils'); const configUtils = require('../utils/configUtils'); const settingsCache = require('../../core/shared/settings-cache'); +const settingsHelpers = require('../../core/server/services/settings-helpers'); const DomainEvents = require('@tryghost/domain-events'); const {MemberPageViewEvent} = require('@tryghost/member-events'); const models = require('../../core/server/models'); const {fixtureManager} = require('../utils/e2e-framework'); const DataGenerator = require('../utils/fixtures/data-generator'); const members = require('../../core/server/services/members'); +const crypto = require('crypto'); function assertContentIsPresent(res) { res.text.should.containEql('

markdown

'); @@ -168,88 +170,114 @@ describe('Front-end members behavior', function () { .expect(400); }); - it('should error for fetching member newsletters with missing uuid', async function () { - await request.get('/members/api/member/newsletters') - .expect(400); - }); + describe('Newsletters', function () { + afterEach(function () { + sinon.restore(); + }); - it('should error for fetching member newsletters with invalid uuid', async function () { - await request.get('/members/api/member/newsletters?uuid=abc') - .expect(404); - }); + it('should error for fetching member newsletters with missing uuid', async function () { + await request.get('/members/api/member/newsletters') + .expect(401); + }); - it('should error for updating member newsletters with missing uuid', async function () { - await request.put('/members/api/member/newsletters') - .expect(400); - }); + it('should error for fetching member newsletters with invalid uuid', async function () { + await request.get('/members/api/member/newsletters?uuid=abc') + .expect(401); + }); - it('should error for updating member newsletters with invalid uuid', async function () { - await request.put('/members/api/member/newsletters?uuid=abc') - .expect(404); - }); + it('should error for updating member newsletters with missing uuid', async function () { + await request.put('/members/api/member/newsletters') + .expect(401); + }); - it('should fetch and update member newsletters with valid uuid', async function () { - const memberUUID = DataGenerator.Content.members[0].uuid; + it('should error for updating member newsletters with invalid uuid', async function () { + await request.put('/members/api/member/newsletters?uuid=abc') + .expect(401); + }); - // Can fetch newsletter subscriptions - const getRes = await request.get(`/members/api/member/newsletters?uuid=${memberUUID}`) - .expect(200); - const getJsonResponse = getRes.body; + it('should error for updating member newsletters with no key', async function () { + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + await request.get('/members/api/member/newsletters?uuid=abc') + .expect(401); + }); - should.exist(getJsonResponse); - getJsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); - getJsonResponse.should.not.have.property('id'); - getJsonResponse.newsletters.should.have.length(1); + it('should 401 for GET member newsletters with a mismatched hmac key', async function () { + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + await request.get('/members/api/member/newsletters?uuid=abc&key=blah') + .expect(401); + }); - // NOTE: these should be snapshots not code - Object.keys(getJsonResponse.newsletters[0]).should.have.length(5); - getJsonResponse.newsletters[0].should.have.properties([ - 'id', - 'uuid', - 'name', - 'description', - 'sort_order' - ]); + it('should 401 for PUT member newsletters with a mismatched hmac key', async function () { + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + await request.put('/members/api/member/newsletters?uuid=abc&key=blah') + .expect(401); + }); - // Can update newsletter subscription - const originalNewsletters = getJsonResponse.newsletters; - const originalNewsletterName = originalNewsletters[0].name; - originalNewsletters[0].name = 'cannot change me'; + it('should fetch and update member newsletters with valid uuid', async function () { + const memberUUID = DataGenerator.Content.members[0].uuid; - const res = await request.put(`/members/api/member/newsletters?uuid=${memberUUID}`) - .send({ - newsletters: [] - }) - .expect(200); - const jsonResponse = res.body; + // Can fetch newsletter subscriptions + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256', 'test').update(memberUUID).digest('hex'); + const getRes = await request.get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) + .expect(200); + const getJsonResponse = getRes.body; - should.exist(jsonResponse); - jsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); - jsonResponse.should.not.have.property('id'); - jsonResponse.newsletters.should.have.length(0); + should.exist(getJsonResponse); + getJsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); + getJsonResponse.should.not.have.property('id'); + getJsonResponse.newsletters.should.have.length(1); - const resRestored = await request.put(`/members/api/member/newsletters?uuid=${memberUUID}`) - .send({ - newsletters: originalNewsletters - }) - .expect(200); + // NOTE: these should be snapshots not code + Object.keys(getJsonResponse.newsletters[0]).should.have.length(5); + getJsonResponse.newsletters[0].should.have.properties([ + 'id', + 'uuid', + 'name', + 'description', + 'sort_order' + ]); - const restoreJsonResponse = resRestored.body; - should.exist(restoreJsonResponse); - restoreJsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); - restoreJsonResponse.should.not.have.property('id'); - restoreJsonResponse.newsletters.should.have.length(1); - // @NOTE: this seems like too much exposed information, needs a review - Object.keys(restoreJsonResponse.newsletters[0]).should.have.length(5); - restoreJsonResponse.newsletters[0].should.have.properties([ - 'id', - 'uuid', - 'name', - 'description', - 'sort_order' - ]); + // Can update newsletter subscription + const originalNewsletters = getJsonResponse.newsletters; + const originalNewsletterName = originalNewsletters[0].name; + originalNewsletters[0].name = 'cannot change me'; - should.equal(restoreJsonResponse.newsletters[0].name, originalNewsletterName); + const res = await request.put(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) + .send({ + newsletters: [] + }) + .expect(200); + const jsonResponse = res.body; + + should.exist(jsonResponse); + jsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); + jsonResponse.should.not.have.property('id'); + jsonResponse.newsletters.should.have.length(0); + + const resRestored = await request.put(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) + .send({ + newsletters: originalNewsletters + }) + .expect(200); + + const restoreJsonResponse = resRestored.body; + should.exist(restoreJsonResponse); + restoreJsonResponse.should.have.properties(['email', 'uuid', 'status', 'name', 'newsletters']); + restoreJsonResponse.should.not.have.property('id'); + restoreJsonResponse.newsletters.should.have.length(1); + // @NOTE: this seems like too much exposed information, needs a review + Object.keys(restoreJsonResponse.newsletters[0]).should.have.length(5); + restoreJsonResponse.newsletters[0].should.have.properties([ + 'id', + 'uuid', + 'name', + 'description', + 'sort_order' + ]); + + should.equal(restoreJsonResponse.newsletters[0].name, originalNewsletterName); + }); }); it('should serve theme 404 on members endpoint', async function () { @@ -266,6 +294,10 @@ describe('Front-end members behavior', function () { }); describe('Unsubscribe', function () { + afterEach(function () { + sinon.restore(); + }); + it('should redirect with uuid and action param', async function () { await request.get('/unsubscribe/?uuid=XXX') .expect(302) @@ -278,12 +310,18 @@ describe('Front-end members behavior', function () { .expect('Location', 'http://127.0.0.1:2369/?uuid=XXX&newsletter=YYY&action=unsubscribe'); }); + it('should pass through an optional key param', async function () { + await request.get('/unsubscribe/?uuid=XXX&key=YYY') + .expect(302) + .expect('Location', 'http://127.0.0.1:2369/?uuid=XXX&key=YYY&action=unsubscribe'); + }); + it('should reject when missing a uuid', async function () { await request.get('/unsubscribe/') .expect(400); }); - it('should do an actual unsubscribe on POST', async function () { + it('should return unauthorized with a bad key', async function () { const newsletterId = fixtureManager.get('newsletters', 0).id; const member = await createMember({ email: 'unsubscribe-member-test@example.com', @@ -293,19 +331,40 @@ describe('Front-end members behavior', function () { }); const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('therealkey'); + const memberHmac = crypto.createHmac('sha256','thefalsekey').update(memberUUID).digest('hex'); + + // auth via uuid+key should fail + await request + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) + .expect(401); + }); + + it('should do an actual unsubscribe on POST', async function () { + const newsletterId = fixtureManager.get('newsletters', 0).id; + const member = await createMember({ + email: 'unsubscribe-member-test-another@example.com', + newsletters: [ + {id: newsletterId} + ] + }); + + const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256','test').update(memberUUID).digest('hex'); // Can fetch newsletter subscriptions let getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); let getJsonResponse = getRes.body; assert.equal(getJsonResponse.newsletters.length, 1); - await request.post(`/unsubscribe/?uuid=${memberUUID}`) + await request.post(`/unsubscribe/?uuid=${memberUUID}&key=${memberHmac}`) .expect(201); getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); getJsonResponse = getRes.body; @@ -326,19 +385,21 @@ describe('Front-end members behavior', function () { }); const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256','test').update(memberUUID).digest('hex'); // Can fetch newsletter subscriptions let getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); let getJsonResponse = getRes.body; assert.equal(getJsonResponse.newsletters.length, 2); - await request.post(`/unsubscribe/?uuid=${memberUUID}&newsletter=${newsletter2Uuid}`) + await request.post(`/unsubscribe/?uuid=${memberUUID}&newsletter=${newsletter2Uuid}&key=${memberHmac}`) .expect(201); getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); getJsonResponse = getRes.body; @@ -357,15 +418,17 @@ describe('Front-end members behavior', function () { }); const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256','test').update(memberUUID).digest('hex'); // Can fetch newsletter subscriptions let getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); let getJsonResponse = getRes.body; assert.equal(getJsonResponse.newsletters.length, 1); - await request.post(`/unsubscribe/?uuid=${memberUUID}&comments=1`) + await request.post(`/unsubscribe/?uuid=${memberUUID}&key=${memberHmac}&comments=1`) .expect(201); const updatedMember = await members.api.members.get({id: member.id}, {withRelated: ['newsletters']}); @@ -383,21 +446,23 @@ describe('Front-end members behavior', function () { }); const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256','test').update(memberUUID).digest('hex'); // Can fetch newsletter subscriptions let getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); let getJsonResponse = getRes.body; assert.equal(getJsonResponse.newsletters.length, 1); - await request.post(`/unsubscribe/?uuid=${memberUUID}`) + await request.post(`/unsubscribe/?uuid=${memberUUID}&key=${memberHmac}`) .type('form') .send({'List-Unsubscribe': 'One-Click'}) .expect(201); getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); getJsonResponse = getRes.body; @@ -414,20 +479,22 @@ describe('Front-end members behavior', function () { }); const memberUUID = member.get('uuid'); + sinon.stub(settingsHelpers, 'getMembersValidationKey').returns('test'); + const memberHmac = crypto.createHmac('sha256','test').update(memberUUID).digest('hex'); // Can fetch newsletter subscriptions let getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); let getJsonResponse = getRes.body; assert.equal(getJsonResponse.newsletters.length, 1); - await request.post(`/unsubscribe/?uuid=${memberUUID}`) + await request.post(`/unsubscribe/?uuid=${memberUUID}&key=${memberHmac}`) .field('List-Unsubscribe', 'One-Click') .expect(201); getRes = await request - .get(`/members/api/member/newsletters?uuid=${memberUUID}`) + .get(`/members/api/member/newsletters?uuid=${memberUUID}&key=${memberHmac}`) .expect(200); getJsonResponse = getRes.body; diff --git a/ghost/core/test/unit/server/services/members/middleware.test.js b/ghost/core/test/unit/server/services/members/middleware.test.js index 94d0242984..e1e6570816 100644 --- a/ghost/core/test/unit/server/services/members/middleware.test.js +++ b/ghost/core/test/unit/server/services/members/middleware.test.js @@ -211,7 +211,8 @@ describe('Members Service Middleware', function () { sinon.restore(); }); - it('returns 400 if no member uuid is part of the request', async function () { + // auth happens prior to this middleware + it('returns 404 if no member uuid is part of the request', async function () { req.query = {}; // Call the middleware @@ -219,11 +220,12 @@ describe('Members Service Middleware', function () { // Check behavior res.writeHead.calledOnce.should.be.true(); - res.writeHead.firstCall.args[0].should.eql(400); + res.writeHead.firstCall.args[0].should.eql(404); res.end.calledOnce.should.be.true(); - res.end.firstCall.args[0].should.eql('Invalid member uuid'); + res.end.firstCall.args[0].should.eql('Email address not found.'); }); + // auth happens prior to this middleware it('returns 404 if member uuid is not found', async function () { req.query = {uuid: 'test'}; sinon.stub(membersService, 'api').get(() => { @@ -246,8 +248,8 @@ describe('Members Service Middleware', function () { it('attempts to update newsletters', async function () { res.json = sinon.stub(); - req.query = {uuid: 'test'}; - const memberData = { + // member data appended if authed via uuid+key or session + req.member = { id: 'test', email: 'test@email.com', name: 'Test Name', @@ -258,10 +260,9 @@ describe('Members Service Middleware', function () { sinon.stub(membersService, 'api').get(() => { return { members: { - get: sinon.stub().resolves({id: 'test', email: 'test@email.com', get: () => 'test'}), update: sinon.stub().resolves({ - ...memberData, - toJSON: () => JSON.stringify(memberData) + ...req.member, + toJSON: () => JSON.stringify(req.member) }) } }; @@ -273,7 +274,22 @@ describe('Members Service Middleware', function () { it('returns 400 on error', async function () { // use a malformed request to trigger an error - req = {}; + // member data appended if authed via uuid+key or session + req.member = { + id: undefined, + email: 'test@email.com', + name: 'Test Name', + newsletters: [], + enable_comment_notifications: false, + status: 'free' + }; + sinon.stub(membersService, 'api').get(() => { + return { + members: { + update: sinon.stub().rejects(new Error('Test Error')) + } + }; + }); await membersMiddleware.updateMemberNewsletters(req, res); // Check behavior diff --git a/ghost/email-service/lib/EmailRenderer.js b/ghost/email-service/lib/EmailRenderer.js index b36f80ad59..474b3d7c20 100644 --- a/ghost/email-service/lib/EmailRenderer.js +++ b/ghost/email-service/lib/EmailRenderer.js @@ -11,6 +11,7 @@ const tpl = require('@tryghost/tpl'); const cheerio = require('cheerio'); const {EmailAddressParser} = require('@tryghost/email-addresses'); const {registerHelpers} = require('./helpers/register-helpers'); +const crypto = require('crypto'); const messages = { subscriptionStatus: { @@ -117,7 +118,7 @@ class EmailRenderer { /** * @param {object} dependencies * @param {object} dependencies.settingsCache - * @param {{getNoReplyAddress(): string, getMembersSupportAddress(): string}} dependencies.settingsHelpers + * @param {{getNoReplyAddress(): string, getMembersSupportAddress(): string, getMembersValidationKey(): string}} dependencies.settingsHelpers * @param {object} dependencies.renderers * @param {{render(object, options): string}} dependencies.renderers.lexical * @param {{render(object, options): string}} dependencies.renderers.mobiledoc @@ -501,9 +502,14 @@ class EmailRenderer { createUnsubscribeUrl(uuid, options = {}) { const siteUrl = this.#urlUtils.urlFor('home', true); const unsubscribeUrl = new URL(siteUrl); + const key = this.#settingsHelpers.getMembersValidationKey(); unsubscribeUrl.pathname = `${unsubscribeUrl.pathname}/unsubscribe/`.replace('//', '/'); if (uuid) { + // hash key with member uuid for verification (and to not leak uuid) - it's possible to update member email prefs without logging in + // @ts-ignore + const hmac = crypto.createHmac('sha256', key).update(`${uuid}`).digest('hex'); unsubscribeUrl.searchParams.set('uuid', uuid); + unsubscribeUrl.searchParams.set('key', hmac); } else { unsubscribeUrl.searchParams.set('preview', '1'); } @@ -639,6 +645,12 @@ class EmailRenderer { return member.uuid; } }, + { + id: 'key', + getValue: (member) => { + return crypto.createHmac('sha256', this.#settingsHelpers.getMembersValidationKey()).update(member.uuid).digest('hex'); + } + }, { id: 'first_name', getValue: (member) => { @@ -962,13 +974,15 @@ class EmailRenderer { const positiveLink = this.#audienceFeedbackService.buildLink( '--uuid--', post.id, - 1 - ).href.replace('--uuid--', '%%{uuid}%%'); + 1, + '--key--' + ).href.replace('--uuid--', '%%{uuid}%%').replace('--key--', '%%{key}%%'); const negativeLink = this.#audienceFeedbackService.buildLink( '--uuid--', post.id, - 0 - ).href.replace('--uuid--', '%%{uuid}%%'); + 0, + '--key--' + ).href.replace('--uuid--', '%%{uuid}%%').replace('--key--', '%%{key}%%'); const commentUrl = new URL(postUrl); commentUrl.hash = '#ghost-comments'; diff --git a/ghost/email-service/test/email-renderer.test.js b/ghost/email-service/test/email-renderer.test.js index 7fa4f2f612..0dedfae005 100644 --- a/ghost/email-service/test/email-renderer.test.js +++ b/ghost/email-service/test/email-renderer.test.js @@ -6,6 +6,7 @@ const linkReplacer = require('@tryghost/link-replacer'); const sinon = require('sinon'); const logging = require('@tryghost/logging'); const {HtmlValidate} = require('html-validate'); +const crypto = require('crypto'); async function validateHtml(html) { const htmlvalidate = new HtmlValidate({ @@ -60,6 +61,10 @@ async function validateHtml(html) { assert.equal(report.valid, true, 'Expected valid HTML without warnings, got errors:\n' + parsedErrors.join('\n\n')); } +const getMembersValidationKey = () => { + return 'members-key'; +}; + describe('Email renderer', function () { let logStub; @@ -92,7 +97,8 @@ describe('Email renderer', function () { return 'UTC'; } } - } + }, + settingsHelpers: {getMembersValidationKey} }); newsletter = createModel({ uuid: 'newsletteruuid' @@ -113,7 +119,8 @@ describe('Email renderer', function () { assert.equal(replacements.length, 1); assert.equal(replacements[0].token.toString(), '/%%\\{list_unsubscribe\\}%%/g'); assert.equal(replacements[0].id, 'list_unsubscribe'); - assert.equal(replacements[0].getValue(member), `http://example.com/subdirectory/unsubscribe/?uuid=myuuid&newsletter=newsletteruuid`); + const memberHmac = crypto.createHmac('sha256', getMembersValidationKey()).update(member.uuid).digest('hex'); + assert.equal(replacements[0].getValue(member), `http://example.com/subdirectory/unsubscribe/?uuid=${member.uuid}&key=${memberHmac}&newsletter=newsletteruuid`); }); it('returns a replacement if it is used', function () { @@ -149,7 +156,8 @@ describe('Email renderer', function () { assert.equal(replacements.length, 2); assert.equal(replacements[0].token.toString(), '/%%\\{unsubscribe_url\\}%%/g'); assert.equal(replacements[0].id, 'unsubscribe_url'); - assert.equal(replacements[0].getValue(member), `http://example.com/subdirectory/unsubscribe/?uuid=myuuid&newsletter=newsletteruuid`); + const memberHmac = crypto.createHmac('sha256', getMembersValidationKey()).update(member.uuid).digest('hex'); + assert.equal(replacements[0].getValue(member), `http://example.com/subdirectory/unsubscribe/?uuid=${member.uuid}&key=${memberHmac}&newsletter=newsletteruuid`); }); it('returns correct name', function () { @@ -310,6 +318,20 @@ describe('Email renderer', function () { // In case of empty name assert.equal(replacements[2].getValue({name: ''}), ''); }); + + it('handles members uuid and key', function () { + const html = '%%{uuid}%% %%{key}%%'; + const replacements = emailRenderer.buildReplacementDefinitions({html, newsletterUuid: newsletter.get('uuid')}); + assert.equal(replacements.length, 3); + assert.equal(replacements[0].token.toString(), '/%%\\{uuid\\}%%/g'); + assert.equal(replacements[0].id, 'uuid'); + assert.equal(replacements[0].getValue(member), 'myuuid'); + + assert.equal(replacements[1].token.toString(), '/%%\\{key\\}%%/g'); + assert.equal(replacements[1].id, 'key'); + const memberHmac = crypto.createHmac('sha256', getMembersValidationKey()).update(member.uuid).digest('hex'); + assert.equal(replacements[1].getValue(member), memberHmac); + }); }); describe('isMemberTrialing', function () { @@ -958,8 +980,8 @@ describe('Email renderer', function () { }); emailRenderer = new EmailRenderer({ audienceFeedbackService: { - buildLink: (_uuid, _postId, score) => { - return new URL('http://feedback-link.com/?score=' + encodeURIComponent(score) + '&uuid=' + encodeURIComponent(_uuid)); + buildLink: (_uuid, _postId, score, key) => { + return new URL('http://feedback-link.com/?score=' + encodeURIComponent(score) + '&uuid=' + encodeURIComponent(_uuid) + '&key=' + encodeURIComponent(key)); } }, urlUtils: { @@ -1058,7 +1080,7 @@ describe('Email renderer', function () { ); }); - it('returns feedback buttons and unsubcribe links', async function () { + it('returns feedback buttons and unsubscribe links', async function () { const post = createModel(basePost); const newsletter = createModel({ header_image: null, @@ -1084,14 +1106,16 @@ describe('Email renderer', function () { // Unsubscribe button included response.plaintext.should.containEql('Unsubscribe [%%{unsubscribe_url}%%]'); response.html.should.containEql('Unsubscribe'); - response.replacements.length.should.eql(3); + response.replacements.length.should.eql(4); response.replacements.should.match([ { id: 'uuid' }, { - id: 'unsubscribe_url', - token: /%%\{unsubscribe_url\}%%/g + id: 'key' + }, + { + id: 'unsubscribe_url' }, { id: 'list_unsubscribe' @@ -1440,21 +1464,23 @@ describe('Email renderer', function () { `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fencoded-link.com%2F%3Fcode%3Dtest%26source_tracking%3Dsite`, `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fexample.com%2F%3Fref%3D123%26source_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, '#', - `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%`, `%%{unsubscribe_url}%%`, `https://ghost.org/?via=pbg-newsletter&source_tracking=site` ]); // Check uuid in replacements - response.replacements.length.should.eql(3); + response.replacements.length.should.eql(4); response.replacements[0].id.should.eql('uuid'); response.replacements[0].token.should.eql(/%%\{uuid\}%%/g); - response.replacements[1].id.should.eql('unsubscribe_url'); - response.replacements[1].token.should.eql(/%%\{unsubscribe_url\}%%/g); - response.replacements[2].id.should.eql('list_unsubscribe'); + response.replacements[1].id.should.eql('key'); + response.replacements[1].token.should.eql(/%%\{key\}%%/g); + response.replacements[2].id.should.eql('unsubscribe_url'); + response.replacements[2].token.should.eql(/%%\{unsubscribe_url\}%%/g); + response.replacements[3].id.should.eql('list_unsubscribe'); }); it('replaces all relative links if click tracking is disabled', async function () { @@ -1495,10 +1521,10 @@ describe('Email renderer', function () { 'http://example.com/', 'http://example.com/#relative-test', '#', - 'http://feedback-link.com/?score=1&uuid=%%{uuid}%%', - 'http://feedback-link.com/?score=0&uuid=%%{uuid}%%', - 'http://feedback-link.com/?score=1&uuid=%%{uuid}%%', - 'http://feedback-link.com/?score=0&uuid=%%{uuid}%%', + 'http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%', + 'http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%', + 'http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%', + 'http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%', '%%{unsubscribe_url}%%', 'https://ghost.org/?via=pbg-newsletter' ]); @@ -1552,21 +1578,23 @@ describe('Email renderer', function () { `http://tracked-link.com/?m=%%{uuid}%%&url=http%3A%2F%2Fexample.com%2F%3Fsource_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fexternal-domain.com%2F%3Fref%3D123%26source_tracking%3Dsite`, `http://tracked-link.com/?m=%%{uuid}%%&url=https%3A%2F%2Fexample.com%2F%3Fref%3D123%26source_tracking%3DTest%2BNewsletter%26post_tracking%3Dadded`, - `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=1&uuid=%%{uuid}%%`, - `http://feedback-link.com/?score=0&uuid=%%{uuid}%%`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=1&uuid=%%{uuid}%%&key=%%{key}%%`, + `http://feedback-link.com/?score=0&uuid=%%{uuid}%%&key=%%{key}%%`, `%%{unsubscribe_url}%%`, `https://ghost.org/?via=pbg-newsletter&source_tracking=site` ]); // Check uuid in replacements - response.replacements.length.should.eql(3); + response.replacements.length.should.eql(4); response.replacements[0].id.should.eql('uuid'); response.replacements[0].token.should.eql(/%%\{uuid\}%%/g); - response.replacements[1].id.should.eql('unsubscribe_url'); - response.replacements[1].token.should.eql(/%%\{unsubscribe_url\}%%/g); - response.replacements[2].id.should.eql('list_unsubscribe'); + response.replacements[1].id.should.eql('key'); + response.replacements[1].token.should.eql(/%%\{key\}%%/g); + response.replacements[2].id.should.eql('unsubscribe_url'); + response.replacements[2].token.should.eql(/%%\{unsubscribe_url\}%%/g); + response.replacements[3].id.should.eql('list_unsubscribe'); }); it('removes data-gh-segment and renders paywall', async function () { @@ -1643,14 +1671,16 @@ describe('Email renderer', function () { response.html.should.containEql('Unsubscribe'); response.html.should.containEql('http://example.com'); - response.replacements.length.should.eql(3); + response.replacements.length.should.eql(4); response.replacements.should.match([ { id: 'uuid' }, { - id: 'unsubscribe_url', - token: /%%\{unsubscribe_url\}%%/g + id: 'key' + }, + { + id: 'unsubscribe_url' }, { id: 'list_unsubscribe' @@ -2263,42 +2293,38 @@ describe('Email renderer', function () { }); describe('createUnsubscribeUrl', function () { - it('includes member uuid and newsletter id', async function () { - const emailRenderer = new EmailRenderer({ + let emailRenderer; + + beforeEach(function () { + emailRenderer = new EmailRenderer({ urlUtils: { urlFor() { return 'http://example.com/subdirectory'; } + }, + settingsHelpers: { + getMembersValidationKey } }); + }); + + it('includes member uuid and newsletter id', async function () { const response = await emailRenderer.createUnsubscribeUrl('memberuuid', { newsletterUuid: 'newsletteruuid' }); - assert.equal(response, `http://example.com/subdirectory/unsubscribe/?uuid=memberuuid&newsletter=newsletteruuid`); + const memberHmac = crypto.createHmac('sha256', getMembersValidationKey()).update('memberuuid').digest('hex'); + assert.equal(response, `http://example.com/subdirectory/unsubscribe/?uuid=memberuuid&key=${memberHmac}&newsletter=newsletteruuid`); }); it('includes comments', async function () { - const emailRenderer = new EmailRenderer({ - urlUtils: { - urlFor() { - return 'http://example.com/subdirectory'; - } - } - }); const response = await emailRenderer.createUnsubscribeUrl('memberuuid', { comments: true }); - assert.equal(response, `http://example.com/subdirectory/unsubscribe/?uuid=memberuuid&comments=1`); + const memberHmac = crypto.createHmac('sha256', getMembersValidationKey()).update('memberuuid').digest('hex'); + assert.equal(response, `http://example.com/subdirectory/unsubscribe/?uuid=memberuuid&key=${memberHmac}&comments=1`); }); it('works for previews', async function () { - const emailRenderer = new EmailRenderer({ - urlUtils: { - urlFor() { - return 'http://example.com/subdirectory'; - } - } - }); const response = await emailRenderer.createUnsubscribeUrl(); assert.equal(response, `http://example.com/subdirectory/unsubscribe/?preview=1`); });
Ghost © 2024 – UnsubscribeGhost © 2024 – Unsubscribe