search

moqui / moqui-framework

Use Moqui Framework to build enterprise applications based on Java. It includes tools for databases (relational, graph, document), local and web services, web and other UI with screens and forms, security, file/resource access, scripts, templates, l10n, caching, logging, search, rules, workflow, multi-instance, and integration.
http://www.moqui.org
Other
279 stars 200 forks source link

Update to session token condition #574

Closed aabiabdallah closed 1 year ago

aabiabdallah commented 1 year ago

The line of code removed introduces some inconsistent behavior to the session token requirement. It basically says that if a session token was just created then it is not to be validated (assuming because the caller still doesn't have it). This is not aligned with our claim that a session token is require for all non-GET requests, instead we get this behavior:

First Attempt with new Session:

  1. GET /rest/s1/test/publicApi1 -> 200
  2. POST /rest/s1/test/publicApi2 -> Session token required (in X-CSRF-Token) for URL ...

Second Attempt with new Session:

  1. POST /rest/s1/test/publicApi2 -> 200 !
jonesde commented 1 year ago

Thank you, agreed, better to allow a POST as a first request in a session and I don't think there are any potential issues with that. I don't recall having prevented that explicitly, just never really ran into that scenario... I bet other people have though. There have been a few odd reports about session token issues that I couldn't reproduce, and I bet the missing piece was that the first request in the session was a POST instead of a GET!

aabiabdallah commented 1 year ago

Hi David! Just making sure we're on the same page after reading your comment. Moqui (before this PR) allows the first request in a session to bypass X-CSRF-Token checks even if it is a POST, this PR changes this logic and enforces the token requirement on all POST requests irrespective of when they occur in the session.

jonesde commented 1 year ago

Oh, you removed the line... I looked at a different diff first for your commit to the branch and I think that was vs the master branch and reversed so it looked like it was added.

In that case, I'm not so sure about this change... it might be okay. Was there a reason you needed it? Because it constrains behavior I can't imagine something wasn't working that now is, so is there a security reason of some sort?

jonesde commented 1 year ago

Because it constrains previously supported behavior I reverted for now, it could break existing applications.

aabiabdallah commented 1 year ago

My initial intent was to avoid circumventing the CSRF token requirement for all post requests, but I read through the code a bit more and CSRF is enforced when the user is authenticated and a session exists on the server. So no having CSRF on public APIs shouldn't be an issue. I'll remove this branch.