Hi James, I still don't see any reason to keep checkSecureParameter in any form because it is related to ServiceEventHandler.
Protection against csrf is a good idea but it has no thing to do with Service. It should be done upstream in http request processing so every type of event (ServiceEventHandler, JavaEventHandler,…) could benefit from this protection. Samuel Quoting James Yong (2019-11-26 17:26:59) > Hi Jacques, all, > > Haven't look into the POC yet. Please see the following updates: > > 1. Not a good practice to allow state-changing request via GET method without > a token to check for CSRF. > > 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token > in url may also be exposed via if user posts screenshots. > > 3. CSRF is not a concern for GET request if we avoid point 1 & 2. > > 4. In OFBiz, the same request handler generally can process both GET and POST > requests. The checkSecureParameter in service events would check for no query > string and as a result, state-changing operation is restricted to a POST > method when service event was used. > > 5. Propose to revert the removal of checkSecureParameter, but add new code to > allow exceptions to query-string check in service events. Exception can be > made by setting a new attribute, allow-query-string-for-event = true | false > (default), in security tag within request-map tag. Also extend > checkSecureParameter to other event types. > > 6. Propose to add new attribute, csrfToken = true | false (default), to > security tag to support anti-csrf for post request: > > a) For forms: if true, will generate hidden token field value using CSRF > Guard library ( see [3] ) for every rendered form and store this token inside > session map variable. Support may be added for token name to be randomized. > > b) For login form: There is a need to prevent Login CSRF. Token will also > be created using pre-session. See [2] for Login CSRF. > > c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token > generated per user session for ajax call. Support may be added for token name > to be randomized for each session. > > 7. The service.http.parameters.require.encrypted property that was removed, > seems related to encrypting sensitive parameter values but wasn't > implemented. May look into this at a later time. > > 8. For implementations that aren't using OFBiz screens, a property may be > added to disable the check for CSRF token. > > Reference: > [1] > https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https. > [2] > https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html > [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no > > Regards, > James >
