Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-02-10 23:36:14 -05:00
Code Issues Activity

Updated support email address verification for DMARC changes (#19147)

Browse source 
fixes GRO-71

- Current flow: unchanged
- New managed flow: verification required
- New managed flow with custom sending domain: only verification
required for different domains
- Self hosters (feature flag): no verification required
This commit is contained in:
Simon Backx 2023-11-28 15:06:58 +01:00 • committed by GitHub
parent be6916f066
commit 3687feca07
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 3545 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
ghost/core/core/server/services/newsletters/NewslettersService.js
View file

@ -335,11 +335,11 @@ class NewslettersService {
fromEmail = `no-reply@${toDomain}`;
}
if (this.emailAddressService.useNewEmailAddresses) {
if (this.emailAddressService.service.useNewEmailAddresses) {
// Gone with the old logic: always use the default email address here
// We don't need to validate the FROM address, only the to address
// Also because we are not only validating FROM addresses, but also possible REPLY-TO addresses, which we won't send FROM
fromEmail = this.emailAddressService.defaultFromAddress;
fromEmail = this.emailAddressService.service.defaultFromAddress;
}
const {ghostMailer} = this;

29
ghost/core/core/server/services/settings/SettingsBREADService.js
View file

@ -1,6 +1,6 @@
const _ = require('lodash');
const tpl = require('@tryghost/tpl');
const {NotFoundError, NoPermissionError, BadRequestError, IncorrectUsageError} = require('@tryghost/errors');
const {NotFoundError, NoPermissionError, BadRequestError, IncorrectUsageError, ValidationError} = require('@tryghost/errors');
const {obfuscatedSetting, isSecretSetting, hideValueIfSecret} = require('./settings-utils');
const logging = require('@tryghost/logging');
const MagicLink = require('@tryghost/magic-link');
@ -9,7 +9,8 @@ const verifyEmailTemplate = require('./emails/verify-email');
const EMAIL_KEYS = ['members_support_address'];
const messages = {
problemFindingSetting: 'Problem finding setting: {key}',
accessCoreSettingFromExtReq: 'Attempted to access core setting from external request'
accessCoreSettingFromExtReq: 'Attempted to access core setting from external request',
invalidEmail: 'Invalid email address'
};
class SettingsBREADService {
@ -22,11 +23,13 @@ class SettingsBREADService {
* @param {Object} options.singleUseTokenProvider
* @param {Object} options.urlUtils
* @param {Object} options.labsService - labs service instance
* @param {{service: Object}} options.emailAddressService
*/
constructor({SettingsModel, settingsCache, labsService, mail, singleUseTokenProvider, urlUtils}) {
constructor({SettingsModel, settingsCache, labsService, mail, singleUseTokenProvider, urlUtils, emailAddressService}) {
this.SettingsModel = SettingsModel;
this.settingsCache = settingsCache;
this.labs = labsService;
this.emailAddressService = emailAddressService;
/* email verification setup */
@ -323,7 +326,18 @@ class SettingsBREADService {
const hasChanged = getSetting(setting).value !== email;
if (await this.requiresEmailVerification({email, hasChanged})) {
emailsToVerify.push({email, key});
const validated = this.emailAddressService.service.validate(email, 'replyTo');
if (!validated.allowed) {
throw new ValidationError({
message: messages.invalidEmail
});
}
if (validated.verificationEmailRequired) {
emailsToVerify.push({email, key});
} else {
filteredSettings.push(setting);
}
} else {
filteredSettings.push(setting);
}
@ -375,6 +389,13 @@ class SettingsBREADService {
fromEmail = `no-reply@${toDomain}`;
}
if (this.emailAddressService.service.useNewEmailAddresses) {
// Gone with the old logic: always use the default email address here
// We don't need to validate the FROM address, only the to address
// Also because we are not only validating FROM addresses, but also possible REPLY-TO addresses, which we won't send FROM
fromEmail = this.emailAddressService.service.defaultFromAddress;
}
const {ghostMailer} = this;
this.magicLinkService.transporter = {

4
ghost/core/core/server/services/settings/settings-service.js
View file

@ -15,6 +15,7 @@ const urlUtils = require('../../../shared/url-utils');
const ObjectId = require('bson-objectid').default;
const settingsHelpers = require('../settings-helpers');
const emailAddressService = require('../email-address');
const MAGIC_LINK_TOKEN_VALIDITY = 24 * 60 * 60 * 1000;
const MAGIC_LINK_TOKEN_VALIDITY_AFTER_USAGE = 10 * 60 * 1000;
@ -35,7 +36,8 @@ const getSettingsBREADServiceInstance = () => {
validityPeriodAfterUsage: MAGIC_LINK_TOKEN_VALIDITY_AFTER_USAGE,
maxUsageCount: MAGIC_LINK_TOKEN_MAX_USAGE_COUNT
}),
urlUtils
urlUtils,
emailAddressService: emailAddressService
});
};

4
ghost/core/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap
View file

@ -3036,7 +3036,7 @@ Object {
"forceTextContent": true,
"from": "\\"Ghost\\" <noreply@sendingdomain.com>",
"generateTextFromHTML": false,
"replyTo": "noreply@acme.com",
"replyTo": null,
"subject": "Verify email address",
"to": "hello@acme.com",
}
@ -3913,7 +3913,7 @@ Object {
"forceTextContent": true,
"from": "\\"Ghost\\" <default@email.com>",
"generateTextFromHTML": false,
"replyTo": "noreply@acme.com",
"replyTo": null,
"subject": "Verify email address",
"to": "hello@acme.com",
}

3306
ghost/core/test/e2e-api/admin/__snapshots__/settings.test.js.snap
View file

File diff suppressed because it is too large Load diff

199
ghost/core/test/e2e-api/admin/settings.test.js
View file

@ -3,10 +3,10 @@ const sinon = require('sinon');
const logging = require('@tryghost/logging');
const SingleUseTokenProvider = require('../../../core/server/services/members/SingleUseTokenProvider');
const settingsCache = require('../../../core/shared/settings-cache');
const {agentProvider, fixtureManager, mockManager, matchers} = require('../../utils/e2e-framework');
const {agentProvider, fixtureManager, mockManager, matchers, configUtils} = require('../../utils/e2e-framework');
const {stringMatching, anyEtag, anyUuid, anyContentLength, anyContentVersion} = matchers;
const models = require('../../../core/server/models');
const {mockLabsDisabled} = require('../../utils/e2e-framework-mock-manager');
const {mockLabsDisabled, mockLabsEnabled} = require('../../utils/e2e-framework-mock-manager');
const {anyErrorId} = matchers;
const CURRENT_SETTINGS_COUNT = 86;
@ -255,7 +255,7 @@ describe('Settings API', function () {
mockManager.assert.sentEmailCount(0);
});
it('editing members_support_address triggers email verification flow', async function () {
it('[LEGACY] editing members_support_address triggers email verification flow', async function () {
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'support@example.com'}]
@ -498,4 +498,197 @@ describe('Settings API', function () {
});
});
});
describe('Managed email without custom sending domain', function () {
this.beforeEach(function () {
configUtils.set('hostSettings:managedEmail:enabled', true);
configUtils.set('hostSettings:managedEmail:sendingDomain', null);
configUtils.set('mail:from', 'default@email.com');
});
it('editing members_support_address triggers email verification flow', async function () {
const currentSetting = settingsCache.get('members_support_address');
assert(currentSetting !== 'othersupport@example.com', 'This test requires a changed email address');
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'othersupport@example.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
})
.expect(({body}) => {
const membersSupportAddress = body.settings.find(setting => setting.key === 'members_support_address');
assert.equal(membersSupportAddress.value, currentSetting);
assert.deepEqual(body.meta, {
sent_email_verification: ['members_support_address']
});
});
mockManager.assert.sentEmailCount(1);
mockManager.assert.sentEmail({
subject: 'Verify email address',
to: 'othersupport@example.com'
});
});
it('editing members_support_address equaling default does not trigger verification flow', async function () {
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'default@email.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
});
mockManager.assert.sentEmailCount(0);
});
});
describe('Managed email with custom sending domain', function () {
this.beforeEach(function () {
configUtils.set('hostSettings:managedEmail:enabled', true);
configUtils.set('hostSettings:managedEmail:sendingDomain', 'sendingdomain.com');
configUtils.set('mail:from', 'default@email.com');
});
it('editing members_support_address without matching domain triggers email verification flow', async function () {
const currentSetting = settingsCache.get('members_support_address');
assert(currentSetting !== 'othersupport@example.com', 'This test requires a changed email address');
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'othersupport@example.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
})
.expect(({body}) => {
const membersSupportAddress = body.settings.find(setting => setting.key === 'members_support_address');
assert.equal(membersSupportAddress.value, currentSetting);
assert.deepEqual(body.meta, {
sent_email_verification: ['members_support_address']
});
});
mockManager.assert.sentEmailCount(1);
mockManager.assert.sentEmail({
subject: 'Verify email address',
to: 'othersupport@example.com'
});
});
it('editing members_support_address with matching domain does not trigger email verification flow', async function () {
const currentSetting = settingsCache.get('members_support_address');
assert(currentSetting !== 'support@sendingdomain.com', 'This test requires a changed email address');
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'support@sendingdomain.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
});
mockManager.assert.sentEmailCount(0);
});
it('editing members_support_address equaling default does not trigger verification flow', async function () {
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'default@email.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
});
mockManager.assert.sentEmailCount(0);
});
});
describe('Self hoster without managed email', function () {
this.beforeEach(function () {
configUtils.set('hostSettings:managedEmail:enabled', false);
configUtils.set('hostSettings:managedEmail:sendingDomain', '');
mockLabsEnabled('newEmailAddresses');
});
it('editing members_support_address does not trigger email verification flow', async function () {
const currentSetting = settingsCache.get('members_support_address');
assert(currentSetting !== 'support@customdomain.com', 'This test requires a changed email address');
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'support@customdomain.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
});
mockManager.assert.sentEmailCount(0);
});
it('editing members_support_address equaling default does not trigger verification flow', async function () {
await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'default@email.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag,
// Special rule for this test, as the labs setting changes a lot
'content-length': anyContentLength,
'content-version': anyContentVersion
});
mockManager.assert.sentEmailCount(0);
});
});
});

10
ghost/core/test/unit/server/services/settings/settings-bread-service.test.js
View file

@ -187,6 +187,16 @@ describe('UNIT > Settings BREAD Service:', function () {
isSet() {
return false;
}
},
emailAddressService: {
service: {
validate() {
return {
allowed: true,
verificationEmailRequired: true
};
}
}
}
});

2
ghost/email-addresses/src/EmailAddressService.ts
View file

@ -112,7 +112,7 @@ export class EmailAddressService {
}
// Invalid configuration: don't allow to send from this sending domain
logging.error(`[EmailAddresses] Invalid configuration: cannot send emails from ${preferred.from} when sending domain is ${this.sendingDomain}`);
logging.error(`[EmailAddresses] Invalid configuration: cannot send emails from ${preferred.from.address} when sending domain is ${this.sendingDomain}`);
}
// Only allow to send from the configured from address