fix #3896, If I know your session's objectId, I can delete it without any auth

[pungme]

• should fix If I know your session's objectId, I can delete it without any auth #3896
• As far as i understand, the problem is all the _Session object has Public Read & Write as ACL and the in the middleware it bypass if the sessionToken is empty. In our experiment, passing X-Parse-Session-Token: 'something' through the header it'll check and throw an error. but if not passing X-Parse-Session-Token: at all, it'll bypass the session check and rely on ACL instead. Which is fine for other classes but for _Session is a crucial exploit since it's Public Read & Write.
• In this PR, we check in the middleware.js, we tried to check the session in rest.js but it seems to break other test.
• This is my second PR. Please feel free to correct & suggest. Thanks!

[pungme]

I'm new here, so how can I debug if the test fail with Postgres? 🤔

[dplewis]

https://github.com/parse-community/parse-server/blob/master/CONTRIBUTING.md

[pungme]

@dplewis Thanks! I'll try that.

[dplewis]

@pungme no problem, I ran your test against postgres and an error was thrown

error getting auth for sessionToken TypeError: Cannot read property '$ne' of undefined

For

PG: find _Session { sessionToken: undefined } skip=undefined, limit=1, , keys=undefined

There might be a better way to do it but your test passed when I added this here https://github.com/parse-community/parse-server/blob/master/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js#L187

if (schema.className === '_Session' && fieldName === 'sessionToken' && !fieldValue) {
  throw new Parse.Error(
    Parse.Error.INVALID_SESSION_TOKEN,
    'invalid session token'
  );
}

[pungme]

@dplewis thanks a lot man, I'll work on it!

[flovilmart]

It should probably never reach the database adapter, most likely should be handled in the DatabaseController

[dplewis]

@pungme If you have time can you write a test for update session throws invalid session token if no session token I don't think there's one.

@flovilmart The test for update session doesn't have a rest key so it throws an unauthorized error. expect(error).toBe(null); is always true. Also it doesn't check if the session has been updated with foo:bar

https://github.com/parse-community/parse-server/blob/master/spec/ParseUser.spec.js#L2178

[pungme]

Hey,
Thanks guys! appreciate your help!

@dplewis sure man, I can add that.

@flovilmart do you mean i should move my checking (currently in middleware.js) to the DatabaseController ?

Cheers

[codecov]

Codecov Report

Merging #3925 into master will increase coverage by 0.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #3925      +/-   ##
==========================================
+ Coverage   90.43%   90.45%   +0.01%     
==========================================
  Files         114      114              
  Lines        7682     7686       +4     
==========================================
+ Hits         6947     6952       +5     
+ Misses        735      734       -1

Impacted Files	Coverage Δ
src/middlewares.js	97.26% <100%> (ø)	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	95.71% <100%> (+0.01%)	⬆️
src/rest.js	95.89% <50%> (-1.3%)	⬇️
src/RestWrite.js	93.1% <0%> (ø)	⬆️
src/Routers/SessionsRouter.js	90.24% <0%> (+4.87%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d79ba1...2e04c67. Read the comment docs.

[flovilmart]

@pungme the fix should probably be in rest.js, let me see if I can do something about it :)

[pungme]

@flovilmart just adjusted it maybe something like this?

[pungme]

@flovilmart oh just saw that you have another pull request, yes that's much better! Thanks!

[flovilmart]

No problem thanks for reporting and adding the failing tests, that was 👍 ! I'll close that one if you don't mind and use the other one.

[pungme]

@flovilmart not at all! go ahead :-)

[AndrewLane]

Thanks for the quick fix!