(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 (Bug) Postcard limit can be exceeded in some cases #169

• Quick checklist

  • npm run lint shows no errors
  • docs were updated where necessary OR they don't need to be updated
  • 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%

Files with Coverage Reduction	New Missed Lines	%
web-dapp/server-lib/postcard_limiter.js	1	81.25%

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

---

💛 - Coveralls

[fvictorio]

This doesn't seem to work. I made this test:

1. Comment the lines in RegisterAddressPage that post to /api/notifyRegTx
2. Limit the number of postcards per day to 3
3. Send the form 5 times. Use the resulting txIds and session keys and send 5 multiple concurrent posts to /api/notifyRegTx.

The five postcards are being sent, while only three of theme should be sent.

[fvictorio]

What does this do?

[garatortiz]

frontend was throwing a 404 error at that url

[fvictorio]

Everything worked for me when I uncommented it. Besides, did something change for this to be needed?

[phahulin]

It depends on which url you opened first. It serves as a root url, when you go to a different urls, they are not actually served by server, but intercepted by react and served on client-side. So if you open /register page in new incognito window, api requests would go to /register/api

[fvictorio]

Maybe the problem is the

        window.$.ajax({
            type: 'post',
            url: './api/prepareRegTx',

in RegisterAddressPage. The . seems problematic. Adding this endpoint to fix the issue seems extremely hacky to me.

[fvictorio]

I pushed a commit fixing this and removing the unnecessary extra routes. Tested both the registration and confirmation using new pages in incognito mode. Please check.

[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!