Hi Jacques, Inline...
On 2019/11/06 18:28:26, Jacques Le Roux <jacques.le.r...@les7arts.com> wrote: > Hi James, > > Inline... > > Le 06/11/2019 à 17:53, James Yong a écrit : > > Hi Jacques, > > > > Understand the intent of checkSecureParameter function is to avoid > > sensitive information > > in the URL during POST method. > > Was ;) It does not exist anymore, in trunk at least, after OFBIZ-11260. [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below.. > > > > A proposal is made to provide an attribute (i.e. > > allow-query-string-for-service-event) to allow url parameters / query > > string for certain request. Shouldn't the value for this attribute be > > false, instead of true, when no value is specified for the attribute? > > Depends, if we want to have the same behaviour than in trunk after > OFBIZ-11260 then it should be false by default. Note that we "never" get back > from > changes, except in case of regression. [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection. > > > > > > The property service.http.parameters.require.encrypted is also not required > > for the proposed change. > > Yes, re-introducing will need to revert work done with OFBIZ-11260 > [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back. > > > > > As a side note, checkSecureParameter function restricts CSRF attack to a > > POST method but doesn't prevent CSRF entirely. Maybe can explore the > > feasibility of supporting CSRF token for OFBiz form in future.. > > Yes, we already do: https://issues.apache.org/jira/browse/OFBIZ-10427 [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed. > > Jacques > > > > > Regards, > > James > > > > > > > > On 2019/11/05 07:47:19, Jacques Le Roux <jacques.le.r...@les7arts.com> > > wrote: > >> Hi James, > >> > >> We had service.http.parameters.require.encrypted in url.properties. It's > >> was the same but an all or nothing. It's now removed. > >> > >> I'm not against your late proposition. It now means to revert changes from > >> OFBIZ-11260! > >> > >> But then it should be reversed. By default > >> allow-query-string-for-service-event would be true and if someone wants to > >> prevent a query string for a > >> particular event then false can be used. > >> > >> I'm not sure much people will care of that, not sure what others think... > >> > >> Jacques > >> > >> Le 05/11/2019 à 01:28, James Yong a écrit : > >>> Hi Jacques, Samuel, all, > >>> > >>> I think the security concerns raised are valid. > >>> > >>> However we can look into adding an attribute when the url parameter check > >>> isn’t required. > >>> For example, > >>> <request-map … > > > >>> <security https="true" auth=“true” > >>> allow-query-string-for-service-event=“true”/> > > >>> … > >>> > >>> Regards, > >>> James > >>> > >>> On 2019/10/31 14:20:11, Jacques Le Roux <jacques.le.r...@les7arts.com> > >>> wrote: > >>>> Hi Samuel, > >>>> > >>>> You can go ahead. I became entangled with non ending issues while > >>>> working on this and this change will not change anything about those > >>>> unrelated issues. > >>>> > >>>> Jacques > >>>> > >>>> Le 30/10/2019 à 17:01, Jacques Le Roux a écrit : > >>>>> Le 30/10/2019 à 15:34, Samuel a écrit : > >>>>>> Hi Jacques, > >>>>>> > >>>>>> On 27/10/2019 17:42, Jacques Le Roux wrote: > >>>>>> > >>>>>>> … So I have no problem removing this method... and closing > >>>>>>> OFBIZ-2330, maybe after "fixing" OFBIZ-9804... > >>>>>> I'm not sure to get your point with OFBIZ-9804, if we simply remove > >>>>>> `checkSecureParameter` we fix this issue, don't we ? > >>>>>> > >>>>>> Samuel > >>>>>> > >>>>> Yes, kinda. I prefer to have all calls to > >>>>> updateContactListPartyNoUserLogin similar. Please wait a bit before I > >>>>> close OFBIZ-9804... > >>>>> > >>>>> Jacques > >>>>> > >>>>> >
