Fix insert

[jotoeri]

@susnux I think you're right, that the insert is open currently, since the combination of PublicPage and CORS is not good.
However, i think we can remove the PublicPage condition in server, since that seems to me has been the fix when the CSRF-token was not allowed on CORS routes yet. As a quick-fix, i'd just execute the CORS-Routine here for now. We can just revert the commit, once server got fixed accordingly.

[codecov]

Codecov Report

Merging #1465 (7c05603) into main (1d315eb) will decrease coverage by 0.36%.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1465      +/-   ##
============================================
- Coverage     35.83%   35.47%   -0.36%     
- Complexity      520      532      +12     
============================================
  Files            51       52       +1     
  Lines          2202     2224      +22     
============================================
  Hits            789      789              
- Misses         1413     1435      +22     

[susnux]

Looks like a valid solution, I thought about something similar: Adding a custom Middleware for our API controller with a similar validation function. This might be cleaner?

[jotoeri]

Yeah, changed it to a middleware now. It's a slightly modified procedure to mainly use the public interfaces, but basically the same as in the CORSMiddleware.

[susnux]

If I am correct, this will check every user and even throw if anonymous submit as the user is not set.
In this case we could simply drop @PublicPage.

If we want to support anonymous submission we should change this to:

if ($this->reflector->hasAnnotation('PublicCORSFix') && $this->userSession->isLoggedIn())

[jotoeri]

Public Pages pass the CSRF Check. Thus it works to submit anonymously.

[susnux]

So request->passesCSRFCheck() returns true for anonymous requests without csrf token?
Because if not, anonymous requests would be blocked by line 79.
That's why I suggested the change to only check logged in users, so that anonymous responses are still possible.

[jotoeri]

Nah, csrfcheck result should of course be independent of the user/anonymous. But our own pages pass the csrf check. Didn't test now, but even with the embedding we should still have a valid csrf token.

Indeed, what does not work are anonymous requests from a foreign source. But do we really want to open that? Or shouldn't all external requests be authenticated? Feels a bit like a hole on its own to open that... 😕

[susnux]

I would not change the behavior here, as currently anonymous submissions from extern are allowed, so this would break such usage cases. Not sure on how high the threat of CSRF of anonymous submissions is, you could submit with the IP of someone else or send spam with the IP of someone else. (But having a breaking change is the only real reason here for me not disallowing anonymous submissions without session)

While writing this I noticed this is the only endpoint working anonymously, so it does not make much sense having this available from external pages, as you can not even request a public share from external without authentication. (only no admin required and cors is set so the user must be authenticated).

[susnux]

Tested and works as expected 👍