javaserverfaces
  1. javaserverfaces
  2. JAVASERVERFACES-2247

includeViewParameters re-evaluates param/model values as EL expressions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.3
    • Fix Version/s: trunk, 2.0.7, 2.1.5, 2.1.6, 2.2.0-m01
    • Component/s: None
    • Labels:
      None
    • Environment:

      Mojarra 2.1.4

      Description

      Consider this testcase:

      Bean (request scoped):

      private String value; // +getter+setter
      
      public String submit() {
        String viewId = FacesContext.getCurrentInstance().getViewRoot().getViewId();
        return viewId + "?faces-redirect=true&includeViewParams=true";
      }
      

      View:

      <f:metadata>
        <f:viewParam name="value" value="#{bean.value}" />
      </f:metadata>
      <h:form>
        <h:inputText value="#{bean.value}" />
        <h:commandButton value="submit" action="#{bean.submit}" />
      </h:form>
      

      When you submit #

      {2+2}

      in the field, it will be evaluated as 4.
      When you submit #

      {bean.submit()}

      in the field, it will be evaluated as its outcome value.

      This is strongly undesireable and puts possible attack holes open. The root of the problem is that com.sun.faces.context.URLBuilder#addParameters(Map<String, List<String>> params) re-evaluates the param/model values as expressions.

      1. 20111122-i_mojarra_2247_war.patch
        23 kB
        Ed Burns
      2. 20111123-1538-i_mojarra_2247.patch
        39 kB
        Ed Burns
      3. 20111124-JAVASERVERFACES-2247-stacktrace.tiff
        67 kB
        Ed Burns
      4. changebundle.txt
        43 kB
        Ed Burns
      5. mojarra_2247_initialfix.patch
        10 kB
        arjan tijms

        Activity

        Hide
        arjan tijms added a comment -

        I misunderstood your recommendation. I thought you were suggesting we simply don't perform the EL evaluation at all. I can see now that you clearly stated this in your original comment on 23 November. I'll keep this open until I can fix it the way you suggest.

        The patch I provided was just a quick shot at the problem, but I think it does exactly this:

        1. EL evaluation is removed from UrlBuilder (and thus from encodeRedirectURL())
        2. For both NavigationHandlerImpl#handleNavigation and NavigationCase#getRedirectURL, the EL evaluation is done just prior to the call to ViewHandler#getRedirect (and thus before encodeRedirectURL())


        So, if that patch would make use of the newly added SharedUtils class to put the EL evaluation for an entire map, wouldn't that be a step in the right direction? (hopefully the excellent suggestion made by jhorstmann can be put in there as well).

        Show
        arjan tijms added a comment - I misunderstood your recommendation. I thought you were suggesting we simply don't perform the EL evaluation at all. I can see now that you clearly stated this in your original comment on 23 November. I'll keep this open until I can fix it the way you suggest. The patch I provided was just a quick shot at the problem, but I think it does exactly this: EL evaluation is removed from UrlBuilder (and thus from encodeRedirectURL()) For both NavigationHandlerImpl#handleNavigation and NavigationCase#getRedirectURL, the EL evaluation is done just prior to the call to ViewHandler#getRedirect (and thus before encodeRedirectURL()) So, if that patch would make use of the newly added SharedUtils class to put the EL evaluation for an entire map, wouldn't that be a step in the right direction? (hopefully the excellent suggestion made by jhorstmann can be put in there as well) .
        Hide
        Ed Burns added a comment -

        Committed to trunk.

        Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247

        SECTION: Modified Files
        ----------------------------
        M jsf-tools/template-src/SharedUtils.java

        • Make methods package private.
        • Absorb evaluateExpressions() from UrlBuilder.
        • Fix isExpression() per recommendation of jhorstmann.

        M jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java
        M jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java
        M jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java
        M jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java
        M jsf-api/src/main/java/javax/faces/application/NavigationCase.java

        • Use SharedUtils.isExpression() instead of ELUtils.isExpression()

        M jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java

        • Remove evaluateExpressions() and obseleted ivars.

        M jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java

        • Remove isExpression() and isMixedExpression().

        M jsf-ri/build.xml
        M jsf-ri/build-tests.xml
        M jsf-api/build.xml

        • Copy SharedUtils to more places.

        M jsf-api/src/main/java/javax/faces/LogStrings_ko.properties
        M jsf-api/src/main/java/javax/faces/LogStrings.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_fr.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_de.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_ja.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties
        M jsf-api/src/main/java/javax/faces/LogStrings_es.properties

        • No more need for extra message.

        M jsf-api/src/main/java/javax/faces/component/UIViewParameter.java

        • Revert change that prevents the value of a UIViewParameter from being
          an expression literal.

        A jsf-ri/test/com/sun/faces/application/packagehack
        A jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java

        • get around IllegalAccessException in tests.

        Sending jsf-api/build.xml
        Sending jsf-api/src/main/java/javax/faces/LogStrings.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties
        Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java
        Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
        Sending jsf-ri/build-tests.xml
        Sending jsf-ri/build.xml
        Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java
        Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java
        Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java
        Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java
        Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java
        Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java
        Adding jsf-ri/test/com/sun/faces/application/packagehack
        Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java
        Sending jsf-tools/template-src/SharedUtils.java
        Transmitting file data .......................
        Committed revision 9486.

        Show
        Ed Burns added a comment - Committed to trunk. Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247 SECTION: Modified Files ---------------------------- M jsf-tools/template-src/SharedUtils.java Make methods package private. Absorb evaluateExpressions() from UrlBuilder. Fix isExpression() per recommendation of jhorstmann. M jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java M jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java M jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java M jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java M jsf-api/src/main/java/javax/faces/application/NavigationCase.java Use SharedUtils.isExpression() instead of ELUtils.isExpression() M jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java Remove evaluateExpressions() and obseleted ivars. M jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java Remove isExpression() and isMixedExpression(). M jsf-ri/build.xml M jsf-ri/build-tests.xml M jsf-api/build.xml Copy SharedUtils to more places. M jsf-api/src/main/java/javax/faces/LogStrings_ko.properties M jsf-api/src/main/java/javax/faces/LogStrings.properties M jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties M jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties M jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties M jsf-api/src/main/java/javax/faces/LogStrings_fr.properties M jsf-api/src/main/java/javax/faces/LogStrings_de.properties M jsf-api/src/main/java/javax/faces/LogStrings_ja.properties M jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties M jsf-api/src/main/java/javax/faces/LogStrings_es.properties No more need for extra message. M jsf-api/src/main/java/javax/faces/component/UIViewParameter.java Revert change that prevents the value of a UIViewParameter from being an expression literal. A jsf-ri/test/com/sun/faces/application/packagehack A jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java get around IllegalAccessException in tests. Sending jsf-api/build.xml Sending jsf-api/src/main/java/javax/faces/LogStrings.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java Sending jsf-ri/build-tests.xml Sending jsf-ri/build.xml Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java Adding jsf-ri/test/com/sun/faces/application/packagehack Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java Sending jsf-tools/template-src/SharedUtils.java Transmitting file data ....................... Committed revision 9486.
        Hide
        Ed Burns added a comment -

        Committed to trunk.

        Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247

        SECTION: Modified Files
        ----------------------------
        M jsf-ri/build-pre-maven-rename.xml
        M jsf-api/build-pre-maven-rename.xml

        • I forgot to make the updates to the "copy.template.sources" target in
          these files.

        Sending jsf-api/build-pre-maven-rename.xml
        Sending jsf-ri/build-pre-maven-rename.xml
        Transmitting file data ..
        Committed revision 9487.

        Show
        Ed Burns added a comment - Committed to trunk. Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247 SECTION: Modified Files ---------------------------- M jsf-ri/build-pre-maven-rename.xml M jsf-api/build-pre-maven-rename.xml I forgot to make the updates to the "copy.template.sources" target in these files. Sending jsf-api/build-pre-maven-rename.xml Sending jsf-ri/build-pre-maven-rename.xml Transmitting file data .. Committed revision 9487.
        Hide
        Ed Burns added a comment -

        Committed to 2.1 branch.

        Sending jsf-api/build-pre-maven-rename.xml
        Sending jsf-api/build.xml
        Sending jsf-api/src/main/java/javax/faces/LogStrings.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties
        Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java
        Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
        Sending jsf-ri/build-pre-maven-rename.xml
        Sending jsf-ri/build-tests.xml
        Sending jsf-ri/build.xml
        Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java
        Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java
        Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java
        Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java
        Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java
        Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java
        Adding jsf-ri/test/com/sun/faces/application/packagehack
        Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java
        Sending jsf-tools/template-src/SharedUtils.java
        Transmitting file data ........................
        Committed revision 9488.

        Show
        Ed Burns added a comment - Committed to 2.1 branch. Sending jsf-api/build-pre-maven-rename.xml Sending jsf-api/build.xml Sending jsf-api/src/main/java/javax/faces/LogStrings.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_HK.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java Sending jsf-ri/build-pre-maven-rename.xml Sending jsf-ri/build-tests.xml Sending jsf-ri/build.xml Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java Adding jsf-ri/test/com/sun/faces/application/packagehack Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java Sending jsf-tools/template-src/SharedUtils.java Transmitting file data ........................ Committed revision 9488.
        Hide
        Ed Burns added a comment -

        Committed to 2.0 branch.

        Sending jsf-api/build-pre-maven-rename.xml
        Sending jsf-api/build.xml
        Sending jsf-api/src/main/java/javax/faces/LogStrings.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties
        Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties
        Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java
        Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
        Sending jsf-ri/build-pre-maven-rename.xml
        Sending jsf-ri/build-tests.xml
        Sending jsf-ri/build.xml
        Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java
        Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java
        Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java
        Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java
        Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java
        Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java
        Adding jsf-ri/test/com/sun/faces/application/packagehack
        Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java
        Sending jsf-tools/template-src/SharedUtils.java
        Transmitting file data .......................
        Committed revision 9489.

        Show
        Ed Burns added a comment - Committed to 2.0 branch. Sending jsf-api/build-pre-maven-rename.xml Sending jsf-api/build.xml Sending jsf-api/src/main/java/javax/faces/LogStrings.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_de.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_es.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_fr.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ja.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_ko.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_pt_BR.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_CN.properties Sending jsf-api/src/main/java/javax/faces/LogStrings_zh_TW.properties Sending jsf-api/src/main/java/javax/faces/application/NavigationCase.java Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java Sending jsf-ri/build-pre-maven-rename.xml Sending jsf-ri/build-tests.xml Sending jsf-ri/build.xml Sending jsf-ri/src/main/java/com/sun/faces/application/NavigationHandlerImpl.java Sending jsf-ri/src/main/java/com/sun/faces/config/processor/NavigationConfigProcessor.java Sending jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java Sending jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java Sending jsf-ri/src/main/java/com/sun/faces/mgbean/BeanBuilder.java Sending jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java Adding jsf-ri/test/com/sun/faces/application/packagehack Adding jsf-ri/test/com/sun/faces/application/packagehack/PackageHack.java Sending jsf-tools/template-src/SharedUtils.java Transmitting file data ....................... Committed revision 9489.

          People

          • Assignee:
            Ed Burns
            Reporter:
            balusc
          • Votes:
            6 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: