-
Notifications
You must be signed in to change notification settings - Fork 218
Description
Whenever I run the auth-server tests locally, I always get test failures in select_email_services.js; see the bottom of my comment for the full listing, in case they help someone else searching for this issue. @dannycoates mentioned that he sometimes sees these failing sporadically in CI as well.
I think I finally tracked it down to an issue with the sandboxed execution of email-matching regexes, which we do here:
fxa/packages/fxa-auth-server/lib/senders/select_email_services.js
Lines 196 to 199 in 62c4158
| const sandbox = new Sandbox({ timeout: 100 }); | |
| sandbox.run(`new RegExp("${regex}").test("${emailAddress}")`, output => { | |
| resolve(output.result === 'true'); | |
| }); |
Every call to sandbox.run spawns a new node process in order to execute the regex. I'm on windows, so spawning new processes is expensive and this basically always times out for me. I find it plausible that this would occasionally timeout in CI as well.
I tried to work around this by increasing the timeout, but it turns out the timeout argument we're providing above gets ignored. If I manually change the default timeout in the sandbox library's code, I can get the tests passing reliably (albeit slowly).
I can continue just ignoring these local failures, but spawning a new process every time we want to pick an email sender seems pretty expensive to me. I'll also note that the sandbox library hasn't been updated in 6 years, which isn't in itself a bad thing. It might be a concern if we started using it for processing more untrusted input besides regexes though, especially given long-standing sandbox escape issues.
Can we do something cheaper here to sanitize the config loaded from redis? I believe the thing we're trying to guard against is a catastrophic-backgracking regex DoS if an untrusted regex somehow makes its way into the live config in redis, and spawning a whole new process for each regex test seems like a very large general-purpose hammer for this very specific nail.
rfk@pomelo:fxa-auth-server(refresh-token-last-used-at-in-redis *)$ VERIFIER_VERSION=0 ./scripts/test-local.sh ./test/local/senders/select_email_services.js
keys file already exists
selectEmailServices:
redis.get returns sendgrid percentage-only match:
✓ selectEmailServices returns the correct data
redis.get returns sendgrid percentage-only mismatch:
✓ selectEmailServices returns the correct data
redis.get returns sendgrid regex-only match:
1) selectEmailServices returns the correct data
redis.get returns sendgrid regex-only mismatch:
✓ selectEmailServices returns the correct data (534ms)
redis.get returns sendgrid combined match:
2) selectEmailServices returns the correct data
redis.get returns sendgrid combined mismatch (percentage):
✓ selectEmailServices returns the correct data
redis.get returns sendgrid combined mismatch (regex):
✓ selectEmailServices returns the correct data (533ms)
redis.get returns socketlabs percentage-only match:
✓ selectEmailServices returns the correct data
redis.get returns socketlabs percentage-only mismatch:
✓ selectEmailServices returns the correct data
redis.get returns socketlabs regex-only match:
3) selectEmailServices returns the correct data
redis.get returns ses percentage-only match:
✓ selectEmailServices returns the correct data
redis.get returns ses percentage-only mismatch:
✓ selectEmailServices returns the correct data
redis.get returns ses regex-only match:
4) selectEmailServices returns the correct data
redis.get returns sendgrid and ses matches:
✓ selectEmailServices returns the correct data
redis.get returns sendgrid match and ses mismatch:
✓ selectEmailServices returns the correct data
redis.get returns sendgrid mismatch and ses match:
5) selectEmailServices returns the correct data
redis.get returns sendgrid and ses mismatches:
✓ selectEmailServices returns the correct data (523ms)
redis.get returns undefined:
✓ selectEmailServices returns the correct data
redis.get returns unsafe regex:
✓ selectEmailServices returns the correct data
redis.get returns quote-terminating regex:
✓ selectEmailServices returns the correct data
email address contains quote-terminator:
✓ selectEmailServices returns the correct data
redis.get fails:
✓ selectEmailServices returns fallback data
redis.get returns invalid JSON:
✓ selectEmailServices returns fallback data
redis.get returns sendgrid match:
6) selectEmailServices returns the correct data
redis.get returns sendgrid mismatch:
✓ selectEmailServices returns the correct data (523ms)
redis.get returns sendgrid and ses matches and a mismatch:
7) selectEmailServices returns the correct data
redis.get returns a sendgrid match and two ses matches:
8) selectEmailServices returns the correct data
redis.get returns three mismatches:
✓ selectEmailServices returns the correct data (3160ms)
selectEmailServices with mocked sandbox:
call selectEmailServices:
9) called the sandbox correctly
call sandbox result handler with match:
✓ resolved
✓ did not fail
call sandbox result handler with timeout:
✓ resolved
✓ did not fail
call selectEmailServices with mocked safe-regex, regex-only match and redos regex:
✓ email address was treated as mismatch (524ms)
selectEmailServices with real redis:
10) returned the correct results
25 passing (22s)
10 failing
┆Issue is synchronized with this Jira Task
┆Issue Number: FXA-1285