Hi, I don't know about the functional validity of this change (yet), but technically speaking you should never use java.net.URLDecoder for encoding or decoding strings.

Why do you need to decode the URL created by NodeURL.toString() ?

On Jul 30, 2013, at 6:23 PM, Nick Scavelli <nscavell(a)redhat.com <mailto:nscavell@redhat.com>> wrote: > TLDR: eXo.env.server.portalBaseURL and > eXo.env.server.portalURLTemplate have changed to include only the > resolved path. > > I've made a change (http://git.io/ogoWhw) which has only been applied > to master so it can resonate a bit in hopes there are no issues. I > have added a resolvedNodePath field to PortalRequestContext (even > though I think nodePath should be changed) to represent the resolved > path of the portal. Meaning if I go to URL /portal/classic/home/foo > the resolved path is just /home not /home/foo. > > The reason for this change was that a fix was applied to decode the > nodePath in order to avoid XSS in the URL since we use this value to > create the JavaScript variables eXo.env.server.portalBaseURL and > eXo.env.server.portalURLTemplate. However this caused an issue for > non-ascii node names since the route couldn't be found after > decoding. I think the resolvedNodePath is a better solution however > it does change the behavior of the JS variables I mentioned. > > If anyone sees a potential issue with this solution please discuss here. > > - Nick > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/gatein-dev

On Jul 30, 2013, at 8:55 PM, Nick Scavelli <nscavell(a)redhat.com <mailto:nscavell@redhat.com>> wrote: > > On 07/30/2013 12:58 PM, Julien Viet wrote: >> Hi, >> >> I don't know about the functional validity of this change (yet), but >> technically speaking you should never use java.net.URLDecoder for >> encoding or decoding strings. > Not sure what you mean here. I'm not encoding and decoding. I removed > the line that used EntityEncoder to encode the path in > PortalRequestHandler. This was added to fix XSS exploit, however it > caused issues. >> >> Why do you need to decode the URL created by NodeURL.toString() ? > Because it's an encoded value. For example a path of /home/ábá in > NodeURL would be /home/%C3%A1b%C3%A1. why not add a toWhateverFormatString() that does the job properly instead on NodeURL instead of having an URL -> encoded -> decoded ?

>> >> On Jul 30, 2013, at 6:23 PM, Nick Scavelli <nscavell(a)redhat.com >> <mailto:nscavell@redhat.com>> wrote: >> >>> TLDR: eXo.env.server.portalBaseURL and >>> eXo.env.server.portalURLTemplate have changed to include only the >>> resolved path. >>> >>> I've made a change (http://git.io/ogoWhw) which has only been >>> applied to master so it can resonate a bit in hopes there are no >>> issues. I have added a resolvedNodePath field to >>> PortalRequestContext (even though I think nodePath should be >>> changed) to represent the resolved path of the portal. Meaning if I >>> go to URL /portal/classic/home/foo the resolved path is just /home >>> not /home/foo. >>> >>> The reason for this change was that a fix was applied to decode the >>> nodePath in order to avoid XSS in the URL since we use this value >>> to create the JavaScript variables eXo.env.server.portalBaseURL and >>> eXo.env.server.portalURLTemplate. However this caused an issue for >>> non-ascii node names since the route couldn't be found after >>> decoding. I think the resolvedNodePath is a better solution however >>> it does change the behavior of the JS variables I mentioned. >>> >>> If anyone sees a potential issue with this solution please discuss >>> here. >>> >>> - Nick >>> _______________________________________________ >>> gatein-dev mailing list >>> gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> >>> https://lists.jboss.org/mailman/listinfo/gatein-dev >> >

TLDR: eXo.env.server.portalBaseURL and eXo.env.server.portalURLTemplatehave changed to include only the resolved path. I've made a change (http://git.io/ogoWhw) which has only been applied to master so it can resonate a bit in hopes there are no issues. I have added a resolvedNodePath field to PortalRequestContext (even though I think nodePath should be changed) to represent the resolved path of the portal. Meaning if I go to URL /portal/classic/home/foo the resolved path is just /home not /home/foo. The reason for this change was that a fix was applied to decode the nodePath in order to avoid XSS in the URL since we use this value to create the JavaScript variables eXo.env.server.portalBaseURL and eXo.env.server.portalURLTemplate. However this caused an issue for non-ascii node names since the route couldn't be found after decoding. I think the resolvedNodePath is a better solution however it does change the behavior of the JS variables I mentioned. If anyone sees a potential issue with this solution please discuss here. - Nick _______________________________________________ gatein-dev mailing list gatein-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/gatein-dev

On 07/31/2013 12:58 PM, Trong Tran wrote: > Hi Nick, > > Obviously this change makes the generation of URL inconsistently > between server and client side (with JS). In addition, we did support > a capability to allow the request URI containing extra information so > far, and Portal can find the best match navigation node from the URI > to show up the corresponding page ==> so we can not just cut it off Yes good point. I guess I question why we process invalid URL's as if they were correct. But as long as we support this, you are right the inconsistency between client and server URL generation must be in sync. > > In eXo Platform product side, it is also using these JS variables to > build the url, therefore there will be probably impacts to its behaviors. Unless you relied on invalid path information to be contained, I'm not sure. > > If this change is just a consequence of the XSS fix from > https://issues.jboss.org/browse/GTNPORTAL-2769. I do think that > GTNPORTAL-2769 would be handled in better way. Actually we did fix > the problem in EXOGTN without changing usage of node path (refer to > https://github.com/exoplatform/exogtn/commit/f22356d). Maybe we > should go in the way as EXOGTN does. > > WDYT ? I'll take a look and see. Thanks !

> > > > On 30 July 2013 23:23, Nick Scavelli <nscavell(a)redhat.com > <mailto:nscavell@redhat.com>> wrote: > > TLDR: eXo.env.server.portalBaseURL and > eXo.env.server.portalURLTemplate have changed to include only the > resolved path. > > I've made a change (http://git.io/ogoWhw) which has only been > applied to master so it can resonate a bit in hopes there are no > issues. I have added a resolvedNodePath field to > PortalRequestContext (even though I think nodePath should be > changed) to represent the resolved path of the portal. Meaning if > I go to URL /portal/classic/home/foo the resolved path is just > /home not /home/foo. > > The reason for this change was that a fix was applied to decode > the nodePath in order to avoid XSS in the URL since we use this > value to create the JavaScript variables > eXo.env.server.portalBaseURL and > eXo.env.server.portalURLTemplate. However this caused an issue > for non-ascii node names since the route couldn't be found after > decoding. I think the resolvedNodePath is a better solution > however it does change the behavior of the JS variables I mentioned. > > If anyone sees a potential issue with this solution please > discuss here. > > - Nick > > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/gatein-dev > > > > > -- > *Trong Tran* > /(+84) 983841909 | /trongtt(a)gmail.com <mailto:trongtt@gmail.com> > Twitter: http://twitter.com/trongtt// _______________________________________________ gatein-dev mailing list gatein-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/gatein-dev

From your PR, it looks like the PortalRequestContext#getResolvedNodePath() method is still being used in UIPortalApplication#getPortalURLTemplate(), and the method could be removed I guess. On 2 August 2013 01:24, Nick Scavelli <nscavell(a)redhat.com <mailto:nscavell@redhat.com>> wrote: On 07/31/2013 04:43 PM, Nick Scavelli wrote: > > On 07/31/2013 12:58 PM, Trong Tran wrote: >> Hi Nick, >> >> Obviously this change makes the generation of URL inconsistently >> between server and client side (with JS). In addition, we did >> support a capability to allow the request URI containing extra >> information so far, and Portal can find the best match >> navigation node from the URI to show up the corresponding page >> ==> so we can not just cut it off > Yes good point. I guess I question why we process invalid URL's > as if they were correct. But as long as we support this, you are > right the inconsistency between client and server URL generation > must be in sync. >> >> In eXo Platform product side, it is also using these JS >> variables to build the url, therefore there will be probably >> impacts to its behaviors. > Unless you relied on invalid path information to be contained, > I'm not sure. >> >> If this change is just a consequence of the XSS fix from >> https://issues.jboss.org/browse/GTNPORTAL-2769. I do think that >> GTNPORTAL-2769 would be handled in better way. Actually we did >> fix the problem in EXOGTN without changing usage of node path >> (refer to https://github.com/exoplatform/exogtn/commit/f22356d). >> Maybe we should go in the way as EXOGTN does. >> >> WDYT ? > I'll take a look and see. Thanks ! So I think this will work however I changed it to decode the actual JS variables. PR is here https://github.com/gatein/gatein-portal/pull/614 >> >> >> >> On 30 July 2013 23:23, Nick Scavelli <nscavell(a)redhat.com >> <mailto:nscavell@redhat.com>> wrote: >> >> TLDR: eXo.env.server.portalBaseURL and >> eXo.env.server.portalURLTemplate have changed to include >> only the resolved path. >> >> I've made a change (http://git.io/ogoWhw) which has only >> been applied to master so it can resonate a bit in hopes >> there are no issues. I have added a resolvedNodePath field >> to PortalRequestContext (even though I think nodePath should >> be changed) to represent the resolved path of the portal. >> Meaning if I go to URL /portal/classic/home/foo the resolved >> path is just /home not /home/foo. >> >> The reason for this change was that a fix was applied to >> decode the nodePath in order to avoid XSS in the URL since >> we use this value to create the JavaScript variables >> eXo.env.server.portalBaseURL and >> eXo.env.server.portalURLTemplate. However this caused an >> issue for non-ascii node names since the route couldn't be >> found after decoding. I think the resolvedNodePath is a >> better solution however it does change the behavior of the >> JS variables I mentioned. >> >> If anyone sees a potential issue with this solution please >> discuss here. >> >> - Nick >> >> _______________________________________________ >> gatein-dev mailing list >> gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> >> >> >> >> -- >> *Trong Tran* >> /(+84) 983841909 | /trongtt(a)gmail.com <mailto:trongtt@gmail.com> >> Twitter: http://twitter.com/trongtt// > > > > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/gatein-dev -- *Trong Tran* /(+84) 983841909 | /trongtt(a)gmail.com <mailto:trongtt@gmail.com> Twitter: http://twitter.com/trongtt//