(Bug) Postcard limit can be exceeded in some cases #169

[garatortiz]

• What is it? (leave one option)

  • Bug (Fix)

• What was the root cause of the problem originally / what feature was missing? There can be a race condition here if multiple requests are sent in short time period. Each requests will result in calling postcardLimiter.canSend which leads to calling postcardLimiter.get. If there are too many requests, they all get the same value from .get() and thus .canSend() is true for all of them, so they can exceed the limit.

• How does this pull request solve it (in broad terms)? Limit the request before calling the canSend method.

• Does it close any open issues? Closes #169

• Quick checklist

  • [x] npm run lint shows no errors
  • [x] docs were updated where necessary OR they don't need to be updated
  • [x] tests were updated where necessary OR they don't need to be updated

• Additional information

[coveralls]

Pull Request Test Coverage Report for Build 579

• 10 of 44 (22.73%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-1.6%) to 79.648%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
web-dapp/server-lib/session-stores/memory.js	2	6	33.33%
web-dapp/server-lib/session-stores/redis.js	4	18	22.22%
web-dapp/controllers/notifyRegTx.js	1	17	5.88%
<!--	Total:	10	44	22.73%	-->

Files with Coverage Reduction	New Missed Lines	%
web-dapp/server-lib/postcard_limiter.js	1	81.25%
<!--	Total:	1		-->

Totals
Change from base Build 575:	-1.6%
Covered Lines:	1029
Relevant Lines:	1272

---

💛 - Coveralls

[fvictorio]

For reference, this is the curl that I used, replacing XXX with the transaction id and YYY with the session key:

curl 'http://localhost:3000/api/notifyRegTx' -H 'Pragma: no-cache' -H 'Origin: http://localhost:3000' -H 'Accept-Encoding: gzip, deflate, br' -H 'Accept-Language: en-US,en;q=0.9,es;q=0.8' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36' -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' -H 'Accept: */*' -H 'Cache-Control: no-cache' -H 'X-Requested-With: XMLHttpRequest' -H 'Connection: keep-alive' -H 'Referer: http://localhost:3000/register' -H 'DNT: 1' --data 'wallet=0x7e7693f12bfd372042b754b729d1474572a2dd01&txId=XXX&sessionKey=YYY' --compressed &

(Of course, the wallet may be different, but that's the first one in the start-testrpc script.)

[phahulin]

I think there's one more important point - if there are multiple processes of the web-server behind a load balancer, they won't be sharing this semaphore -q-semaphore runs only in-memory, so each process will have its own version of the semaphore.

I wonder if we can use getAndLock and unlock (from https://github.com/poanetwork/poa-popa/pull/180/files#diff-398c3b29ace7ec50f98721e0bddb02e6R13) to make it work?

[phahulin]

Probably this module https://www.npmjs.com/package/petty-cache#semaphore can help. But for a local testing it should be configurable to run web-server without it.

[phahulin]

@matiasgaratortiz I moved pettyCache to session-store and added methods mutexLock, mutexUnlock. Also, moved MAX_POSTCARDS_PER_DAY from .env to server-config because it's not used by client-side react.

@fvictorio it seems to be working, could you please test again? You'll need redis locally, because pettyCache only works with redis. For memory session store, I just pass requests forward, since memory is used only for tests anyway.

[phahulin]

Added mutexes for memory-type sessionStore

[phahulin]

@fvictorio I also changed ./api to /api on RegisterAddressPage.test.js

[fvictorio]

I tested again with multiple concurrent requests and all postcards were sent, even when the limit was exceeded.

I also started two instances of the app with redis as db, and the postcard limit was enforced even when alternating between the two instances, so that worked.

[phahulin]

@fvictorio it must be fixed now, sorry.

[fvictorio]

Thanks @phahulin. Tested it with both memory and redis stores. Looks good.

[phahulin]

@fvictorio thank you!