Fix 'isRequestOriginAllowed' returns mixed results when regex is used

[Tom-Brouwer]

Fixes #151

This pull-request contains the code update (Regex.test replaced by String.match function), as well as the update to the tests (The tests now use a global regex, and repeats the test two times to test consistency)

Test Output

> fastify-cors@6.0.2 test
> npm run lint && npm run unit && npm run typescript


> fastify-cors@6.0.2 lint
> npm run lint:standard && npm run lint:typescript


> fastify-cors@6.0.2 lint:standard
> standard


> fastify-cors@6.0.2 lint:typescript
> standard --parser @typescript-eslint/parser --plugin @typescript-eslint/eslint-plugin test/*.ts


> fastify-cors@6.0.2 unit
> tap test/*.test.js

 PASS  test/vary.test.js 1 OK 6.878ms
 PASS  test/preflight.test.js 51 OK 168.181ms
 PASS  test/cors.test.js 139 OK 236.792ms

                         
  🌈 SUMMARY RESULTS 🌈  
                         

Suites:   3 passed, 3 of 3 completed
Asserts:  191 passed, of 191
Time:     791.297ms
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                   
 index.js |     100 |      100 |     100 |     100 |                   
 vary.js  |     100 |      100 |     100 |     100 |                   
----------|---------|----------|---------|---------|-------------------

> fastify-cors@6.0.2 typescript
> tsd

Checklist

• [ x ] run npm run test and npm run benchmark -> Note: There is no 'benchmark' script in package.json
• [ x ] tests and/or benchmarks are included
• [ x ] documentation is changed or added
• [ x ] commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[coveralls]

Pull Request Test Coverage Report for Build 1278185507

• 0 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 1146637162:	0.0%
Covered Lines:	94
Relevant Lines:	94

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 1882066914

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 1881514815:	0.0%
Covered Lines:	95
Relevant Lines:	95

---

💛 - Coveralls

[zekth]

can't we just do?

Suggested change

	return Boolean(reqOrigin.match(allowedOrigin))
	allowedOrigin.lastIndex=0
	return allowedOrigin.test(reqOrigin)

[Tom-Brouwer]

@zekth I don't know, I'm not sure about the exact implementation of Node.js async/thread behaviour. E.g. if two requests are triggered simultaneously, can we ensure that the following does not occur?:

1. First request is running .test function
2. Second request sets 'lastIndex' to 0
3. First requests finds match and updates lastIndex again
4. Second requests then uses the updated lastIndex

If we are sure the above cannot happen, and these lines are always executed synchronously without interruption, we can also use your solution since it's less invasive.

Please let me know what you think, then I'll update the commit...

[zekth]

This cannot happen as we are not working with multithreading but asynchronization. As those operations happen in the same scope there would not be any collision regarding the change of the regex.

On the other hand this shows a problem that might appear, which is passing a reference of the regex in the plugin like:

const reg = /(example|other)\.com/gi
fastify.register(cors, { origin: reg })

In this case, if reg is mutated anywhere else in the app we'll have a collision. So in this case the only way to avoid this is to do something like

if (allowedOrigin instanceof RegExp) {
    const testReg = RegExp(allowedOrigin.source, allowedOrigin.flags)
    return testReg.test(reqOrigin)
}

[Tom-Brouwer]

You are correct. I just tested the performance implications of three variants:

const sourceRegex = new RegExp('example.com', 'gi');
const testString = 'https://www.example.com/';

function timeRegular() { // Takes about 33ms
  start = performance.now();
  for (let i=0; i < 1000000; i++) {
    sourceRegex.lastIndex = 0;
    sourceRegex.test(testString);
  }
  end = performance.now()
  return end - start;
}

function timeCopy() { // Takes ~ 160 ms
  start = performance.now();
  for (let i=0; i < 1000000; i++) {
    const testRegex = new RegExp(sourceRegex.source, sourceRegex.flags);
    testRegex.test(testString);
  }
  end = performance.now()
  return end - start;
}

function timeMatch() { // Takes ~100ms
  start = performance.now();
  for (let i=0; i < 1000000; i++) {
    testString.match(sourceRegex);
  }
  end = performance.now()
  return end - start;
}

The copying seems to add some overhead to the process (factor 5 roughly), but for a million checks it's only about 130ms extra.

I also checked here, and it seems like 'lastIndex' is the only editable attribute for a RegExp object.

If you think this additional overhead compares positively to being sure that 'lastIndex' is zero at the start, please let me know, I'll implement your last suggestion.

Personally I would go with your first suggestion (Which is also the fastest from the above tests), since I wouldn't even want to promote being able to count on the 'lastIndex' staying zero anywhere in the plugin, but that may be just my personal preference...

[zekth]

@climba03003 I would like to have your opinion on this.

[climba03003]

I will go for editing the lastIndex. If you think it may alter by someone in the middle of run. You can clone it once before passing down to the check. It should be in https://github.com/fastify/fastify-cors/pull/152/files#R101

[Fdawgs]

@Tom-Brouwer how's this going?

[Tom-Brouwer]

Hi @Fdawgs ,

Sorry, totally forgot about this. I've pushed an update in-line with the last comments (So implementing the 'lastIndex' solution). Please review.

Best regards,

Tom Brouwer

Test Output

> fastify-cors@6.0.2 test
> npm run lint && npm run unit && npm run typescript


> fastify-cors@6.0.2 lint
> npm run lint:standard && npm run lint:typescript


> fastify-cors@6.0.2 lint:standard
> standard


> fastify-cors@6.0.2 lint:typescript
> standard --parser @typescript-eslint/parser --plugin @typescript-eslint/eslint-plugin test/*.ts


> fastify-cors@6.0.2 unit
> tap test/*.test.js

 PASS  test/vary.test.js 1 OK 5.985ms
 PASS  test/preflight.test.js 51 OK 248.744ms
 PASS  test/cors.test.js 139 OK 262.177ms

                         
  🌈 SUMMARY RESULTS 🌈  
                         

Suites:   3 passed, 3 of 3 completed
Asserts:  191 passed, of 191
Time:     1s
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                   
 index.js |     100 |      100 |     100 |     100 |                   
 vary.js  |     100 |      100 |     100 |     100 |                   
----------|---------|----------|---------|---------|-------------------

> fastify-cors@6.0.2 typescript
> tsd

[climba03003]

LGTM.

[zekth]

✌️ lgtm