Skip to content

Commit c8c9c78

Browse files
dschomdschom
authored
Merge pull request #18419 from mozilla/FXA-11168
bug(customs): Fix misconfigured custom rules for recovery phone
2 parents b027920 + 98e1227 commit c8c9c78

7 files changed

Lines changed: 66 additions & 160 deletions

File tree

‎packages/fxa-auth-server/lib/routes/recovery-phone.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ class RecoveryPhoneHandler {
103103
throw AppError.invalidToken();
104104
}
105105

106-
await this.customs.check(request, email, 'recoveryPhoneSendCode');
106+
await this.customs.check(request, email, 'recoveryPhoneSendSigninCode');
107107

108108
const getFormattedMessage = async (code: string) => {
109109
const localizedMessage = await this.getLocalizedMessage(
@@ -170,7 +170,12 @@ class RecoveryPhoneHandler {
170170
if (!email) {
171171
throw AppError.invalidToken();
172172
}
173-
await this.customs.checkAuthenticated(request, uid, 'recoveryPhoneCreate');
173+
174+
await this.customs.checkAuthenticated(
175+
request,
176+
uid,
177+
'recoveryPhoneSendSetupCode'
178+
);
174179

175180
const getFormattedMessage = async (code: string) => {
176181
const localizedMessage = await this.getLocalizedMessage(
@@ -252,7 +257,7 @@ class RecoveryPhoneHandler {
252257
await this.customs.checkAuthenticated(
253258
request,
254259
uid,
255-
'recoveryPhoneConfirmCode'
260+
'verifyRecoveryPhoneTotpCode'
256261
);
257262

258263
let success = false;

‎packages/fxa-auth-server/test/local/routes/recovery-phone.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ describe('/recovery_phone', () => {
117117
assert.equal(mockCustoms.check.getCall(0).args[1], email);
118118
assert.equal(
119119
mockCustoms.check.getCall(0).args[2],
120-
'recoveryPhoneSendCode'
120+
'recoveryPhoneSendSigninCode'
121121
);
122122

123123
assert.calledOnceWithExactly(
@@ -219,7 +219,7 @@ describe('/recovery_phone', () => {
219219
assert.equal(mockCustoms.checkAuthenticated.getCall(0).args[1], uid);
220220
assert.equal(
221221
mockCustoms.checkAuthenticated.getCall(0).args[2],
222-
'recoveryPhoneCreate'
222+
'recoveryPhoneSendSetupCode'
223223
);
224224
assert.calledOnceWithExactly(
225225
mockStatsd.increment,

‎packages/fxa-auth-server/test/remote/recovery_phone_tests.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,14 +337,14 @@ describe(`#integration - recovery phone - customs checks`, function () {
337337
// One code gets sent here
338338
await client.recoveryPhoneCreate(phoneNumber);
339339

340-
// Send 8 more codes, for a total of 9 codes.
341-
for (let i = 0; i < 9; i++) {
340+
// Send 15 more codes, for a total of 16 codes.
341+
for (let i = 0; i < 15; i++) {
342342
try {
343343
await client.recoveryPhoneConfirmSetup('000001');
344344
} catch {}
345345
}
346346

347-
// The 10th code should get throttled.
347+
// The 16th code should get throttled.
348348
let error;
349349
try {
350350
await client.recoveryPhoneConfirmSetup('000001');
@@ -366,7 +366,7 @@ describe(`#integration - recovery phone - customs checks`, function () {
366366

367367
let error;
368368
try {
369-
for (let i = 0; i < 9; i++) {
369+
for (let i = 0; i < 7; i++) {
370370
await client.recoveryPhoneSendCode();
371371
}
372372
} catch (err) {

‎packages/fxa-customs-server/lib/actions.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ const CODE_VERIFYING_ACTION = {
2424
// changes at 60 seconds
2525
// limits by email
2626
verifyTotpCode: true,
27-
recoveryPhoneConfirmCode: true,
27+
verifyRecoveryPhoneTotpCode: true,
2828
};
2929

3030
// Actions that, if allowed, would allow an attacker
@@ -61,8 +61,8 @@ const EMAIL_SENDING_ACTION = {
6161
// very annoying to a user if abused.
6262
const SMS_SENDING_ACTION = {
6363
connectDeviceSms: true,
64-
recoveryPhoneSendCode: true,
65-
recoveryPhoneCreate: true,
64+
recoveryPhoneSendSigninCode: true,
65+
recoveryPhoneSendSetupCode: true,
6666
};
6767

6868
// Actions that may grant access to an account but

‎packages/fxa-customs-server/lib/config/config.js‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,33 @@ module.exports = function (fs, path, url, convict) {
448448
},
449449
},
450450
},
451+
recoveryPhoneTotpCodeRules: {
452+
actions: {
453+
doc: 'Array of actions that this rule should be applied to',
454+
default: ['verifyRecoveryPhoneTotpCode'],
455+
format: Array,
456+
},
457+
limits: {
458+
max: {
459+
doc: 'max actions during `period` that can occur before rate limit is applied',
460+
format: 'nat',
461+
default: 10,
462+
env: 'RECOVERY_PHONE_TOTP_CODE_RULE_MAX',
463+
},
464+
periodMs: {
465+
doc: 'period needed before rate limit is reset',
466+
format: 'duration',
467+
default: '15 minutes',
468+
env: 'RECOVERY_PHONE_TOTP_CODE_RULE_PERIOD_MS',
469+
},
470+
rateLimitIntervalMs: {
471+
doc: 'how long rate limit is applied',
472+
format: 'duration',
473+
default: '15 minutes',
474+
env: 'RECOVERY_PHONE_TOTP_CODE_RULE_LIMIT_INTERVAL_MS',
475+
},
476+
},
477+
},
451478
tokenCodeRules: {
452479
actions: {
453480
doc: 'Array of actions that this rule should be applied to',
@@ -475,61 +502,61 @@ module.exports = function (fs, path, url, convict) {
475502
},
476503
},
477504
},
478-
recoveryPhoneCreateCodeRules: {
505+
recoveryPhoneSendSetupCodeRules: {
479506
actions: {
480507
doc:
481508
'Array of actions that this rule should be applied to. A user should only be able to initiate ' +
482-
'a recovery phone setup X times a day.',
483-
default: ['recoveryPhoneCreate'],
509+
'a recovery phone setup X times a day by sending a setup code.',
510+
default: ['recoveryPhoneSendSetupCode'],
484511
format: Array,
485512
},
486513
limits: {
487514
max: {
488515
doc: 'max actions during `period` that can occur before rate limit is applied',
489516
format: 'nat',
490517
default: 9,
491-
env: 'RECOVERY_PHONE_SETUP_CODE_RULE_MAX',
518+
env: 'RECOVERY_PHONE_SEND_SETUP_CODE_RULE_MAX',
492519
},
493520
periodMs: {
494521
doc: 'period needed before rate limit is reset',
495522
format: 'duration',
496523
default: '1 day',
497-
env: 'RECOVERY_PHONE_SETUP_CODE_RULE_PERIOD_MS',
524+
env: 'RECOVERY_PHONE_SEND_SETUP_CODE_RULE_PERIOD_MS',
498525
},
499526
rateLimitIntervalMs: {
500527
doc: 'how long rate limit is applied',
501528
format: 'duration',
502-
default: '2 hours',
503-
env: 'RECOVERY_PHONE_SETUP_CODE_RULE_LIMIT_INTERVAL_MS',
529+
default: '24 hours',
530+
env: 'RECOVERY_PHONE_SEND_SETUP_CODE_RULE_LIMIT_INTERVAL_MS',
504531
},
505532
},
506533
},
507-
recoveryPhoneConfirmCodeRules: {
534+
recoveryPhoneSendSigninCodeRules: {
508535
actions: {
509536
doc:
510537
'Array of actions that this rule should be applied to. A user should only be able to initiate ' +
511-
'a recovery phone confirm X times a day.',
512-
default: ['recoveryPhoneConfirmCode'],
538+
'a recovery phone signin X times a day by sending an sms code.',
539+
default: ['recoveryPhoneSendSigninCode'],
513540
format: Array,
514541
},
515542
limits: {
516543
max: {
517544
doc: 'max actions during `period` that can occur before rate limit is applied',
518545
format: 'nat',
519546
default: 6,
520-
env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_MAX',
547+
env: 'RECOVERY_PHONE_SEND_SIGNIN_CODE_RULE_MAX',
521548
},
522549
periodMs: {
523550
doc: 'period needed before rate limit is reset',
524551
format: 'duration',
525552
default: '1 day',
526-
env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_PERIOD_MS',
553+
env: 'RECOVERY_PHONE_SEND_SIGNIN_CODE_RULE_PERIOD_MS',
527554
},
528555
rateLimitIntervalMs: {
529556
doc: 'how long rate limit is applied',
530557
format: 'duration',
531-
default: '2 hours',
532-
env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_LIMIT_INTERVAL_MS',
558+
default: '24 hours',
559+
env: 'RECOVERY_PHONE_SEND_SIGNIN_CODE_RULE_LIMIT_INTERVAL_MS',
533560
},
534561
},
535562
},

‎packages/fxa-customs-server/test/remote/too_many_sms.js‎

Lines changed: 0 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -372,132 +372,6 @@ test('clear everything', function (t) {
372372
});
373373
});
374374

375-
test('/check `recoveryPhoneSendCode` by email', function (t) {
376-
const action = 'recoveryPhoneSendCode';
377-
return (
378-
client
379-
.postAsync('/check', {
380-
ip: TEST_IP,
381-
email: '[email protected]',
382-
action,
383-
})
384-
.spread(function (req, res, obj) {
385-
t.equal(res.statusCode, 200, 'returns a 200');
386-
t.equal(obj.block, false, 'not rate limited');
387-
return client.postAsync('/check', {
388-
ip: TEST_IP,
389-
email: '[email protected]',
390-
action,
391-
});
392-
})
393-
.spread(function (req, res, obj) {
394-
t.equal(res.statusCode, 200, 'returns a 200');
395-
t.equal(obj.block, false, 'not rate limited');
396-
return client.postAsync('/check', {
397-
ip: TEST_IP,
398-
email: '[email protected]',
399-
action,
400-
});
401-
})
402-
.spread(function (req, res, obj) {
403-
t.equal(res.statusCode, 200, 'returns a 200');
404-
t.equal(obj.block, true, 'rate limited');
405-
t.equal(obj.retryAfter, 1, 'rate limit retry amount');
406-
407-
// Delay ~1s for rate limit to go away
408-
return Promise.delay(1010);
409-
})
410-
411-
// Reissue requests to verify that throttling is disabled
412-
.then(function () {
413-
return client.postAsync('/check', {
414-
ip: TEST_IP,
415-
email: '[email protected]',
416-
action,
417-
});
418-
})
419-
.spread(function (req, res, obj) {
420-
t.equal(res.statusCode, 200, 'returns a 200');
421-
t.equal(obj.block, false, 'not rate limited');
422-
t.end();
423-
})
424-
.catch(function (err) {
425-
t.fail(err);
426-
t.end();
427-
})
428-
);
429-
});
430-
431-
test('clear everything', function (t) {
432-
mcHelper.clearEverything(function (err) {
433-
t.notOk(err, 'no errors were returned');
434-
t.end();
435-
});
436-
});
437-
438-
test('/check `recoveryPhoneCreate` by email', function (t) {
439-
const action = 'recoveryPhoneCreate';
440-
return (
441-
client
442-
.postAsync('/check', {
443-
ip: TEST_IP,
444-
email: '[email protected]',
445-
action,
446-
})
447-
.spread(function (req, res, obj) {
448-
t.equal(res.statusCode, 200, 'returns a 200');
449-
t.equal(obj.block, false, 'not rate limited');
450-
return client.postAsync('/check', {
451-
ip: TEST_IP,
452-
email: '[email protected]',
453-
action,
454-
});
455-
})
456-
.spread(function (req, res, obj) {
457-
t.equal(res.statusCode, 200, 'returns a 200');
458-
t.equal(obj.block, false, 'not rate limited');
459-
return client.postAsync('/check', {
460-
ip: TEST_IP,
461-
email: '[email protected]',
462-
action,
463-
});
464-
})
465-
.spread(function (req, res, obj) {
466-
t.equal(res.statusCode, 200, 'returns a 200');
467-
t.equal(obj.block, true, 'rate limited');
468-
t.equal(obj.retryAfter, 1, 'rate limit retry amount');
469-
470-
// Delay ~1s for rate limit to go away
471-
return Promise.delay(1010);
472-
})
473-
474-
// Reissue requests to verify that throttling is disabled
475-
.then(function () {
476-
return client.postAsync('/check', {
477-
ip: TEST_IP,
478-
email: '[email protected]',
479-
action,
480-
});
481-
})
482-
.spread(function (req, res, obj) {
483-
t.equal(res.statusCode, 200, 'returns a 200');
484-
t.equal(obj.block, false, 'not rate limited');
485-
t.end();
486-
})
487-
.catch(function (err) {
488-
t.fail(err);
489-
t.end();
490-
})
491-
);
492-
});
493-
494-
test('clear everything', function (t) {
495-
mcHelper.clearEverything(function (err) {
496-
t.notOk(err, 'no errors were returned');
497-
t.end();
498-
});
499-
});
500-
501375
test('teardown', async function (t) {
502376
await testServer.stop();
503377
t.end();

‎packages/fxa-customs-server/test/remote/user_defined_rules.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ config.userDefinedRateLimitRules.tokenCodeRules.limits.max = 2;
3030
config.userDefinedRateLimitRules.tokenCodeRules.limits.periodMs = 1000;
3131
config.userDefinedRateLimitRules.tokenCodeRules.limits.rateLimitIntervalMs = 1000;
3232

33-
config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.max = 5;
34-
config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.periodMs = 5000;
35-
config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.rateLimitIntervalMs = 1000;
33+
config.userDefinedRateLimitRules.recoveryPhoneSendSetupCodeRules.limits.max = 5;
34+
config.userDefinedRateLimitRules.recoveryPhoneSendSetupCodeRules.limits.periodMs = 5000;
35+
config.userDefinedRateLimitRules.recoveryPhoneSendSetupCodeRules.limits.rateLimitIntervalMs = 1000;
3636

37-
config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.max = 5;
38-
config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.periodMs = 5000;
39-
config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.rateLimitIntervalMs = 1000;
37+
config.userDefinedRateLimitRules.recoveryPhoneSendSigninCodeRules.limits.max = 5;
38+
config.userDefinedRateLimitRules.recoveryPhoneSendSigninCodeRules.limits.periodMs = 5000;
39+
config.userDefinedRateLimitRules.recoveryPhoneSendSigninCodeRules.limits.rateLimitIntervalMs = 1000;
4040

4141
const ACTIONS = ['verifyTotpCode', 'verifyTokenCode'];
4242

@@ -150,8 +150,8 @@ ACTIONS.forEach((action) => {
150150
});
151151

152152
const recoveryPhoneActions = [
153-
'recoveryPhoneCreate',
154-
'recoveryPhoneConfirmCode',
153+
'recoveryPhoneSendSetupCode',
154+
'recoveryPhoneSendSigninCode',
155155
];
156156
recoveryPhoneActions.forEach((action) => {
157157
test(`clear everything for ${action}`, async (t) => {

0 commit comments

Comments
 (0)