Excellent, thanks! On Mon, Aug 14, 2017 at 4:13 PM, Francesco Chicchiriccò <ilgro...@apache.org > wrote:
> Hi, > I have committed the changes discussed below as > > https://github.com/apache/syncope/commit/e99766a441260bb47db > ac13efe069682dd4a442d > https://github.com/apache/syncope/commit/f912d90c2aa23c055cf > b2e143e865f02025154bd > > Regards. > > On 11/08/2017 18:02, Colm O hEigeartaigh wrote: > >> On Fri, Aug 11, 2017 at 2:59 PM, Francesco Chicchiriccò < >> ilgro...@apache.org> wrote: >> >> Agree. Maybe it should just be changed to >>> >>> @PreAuthorize("isAuthenticated()") >>> >> +1. >> >> >> b) The urlContext not validated at all. For example, you can pass through >>> >>>> something like "../../root" which is added to the metadata, e.g. >>>> Location=" >>>> http://localhost:9080/syncope/../../root/assertion-consumer";. >>>> >>>> Should we implement some kind of validation rules on what is acceptable >>>> here? >>>> >>>> What do you have in mind here? Just forbid '../'? What could be the >>> issue(s) with the current implementation? >>> >> >> Well it makes me a bit uneasy that a user can essentially insert >> arbitrary >> URLs into a metadata document issued by Syncope. I have three suggestions: >> >> a) Restrict the acceptable values. Do we need to allow arbitrary contexts >> here...? >> b) Disallow ".." in the values >> c) Use Commons UrlValidator to make sure that the generated assertion >> consumer URL is a valid URL. >> >> Colm. >> > > -- > Francesco Chicchiriccò > > Tirasa - Open Source Excellence > http://www.tirasa.net/ > > Member at The Apache Software Foundation > Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail > http://home.apache.org/~ilgrosso/ > > -- Colm O hEigeartaigh Talend Community Coder http://coders.talend.com
