Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES-2247
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Ed Burns
Reporter: balusc
Votes: 6
Watchers: 7
Operations

If you were logged in you would be able to see more operations.
javaserverfaces

includeViewParameters re-evaluates param/model values as EL expressions

Created: 17/Nov/11 12:23 PM   Updated: 26/Jun/12 02:50 PM   Resolved: 02/Dec/11 07:28 PM
Component/s: None
Affects Version/s: 2.1.3
Fix Version/s: trunk, 2.0.7, 2.1.5, 2.1.6, 2.2.0-m01

Time Tracking:
Not Specified

File Attachments: 1. Text File 20111122-i_mojarra_2247_war.patch (23 kB) 22/Nov/11 11:01 PM - Ed Burns
2. Text File 20111123-1538-i_mojarra_2247.patch (39 kB) 23/Nov/11 08:41 PM - Ed Burns
3. File 20111124-JAVASERVERFACES-2247-stacktrace.tiff (67 kB) 24/Nov/11 04:00 PM - Ed Burns
4. Text File changebundle.txt (43 kB) 23/Nov/11 08:48 PM - Ed Burns
5. Text File mojarra_2247_initialfix.patch (10 kB) 22/Nov/11 11:48 PM - arjan tijms

Environment:

Mojarra 2.1.4


Tags:
Participants: arjan tijms, aschwart, balusc, Ed Burns, frederickkaempfer, Jakob Korherr, jhorstmann, lu4242, rogerk and ssilvert


 Description  « Hide

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.



balusc added a comment - 17/Nov/11 12:45 PM

The "workaround" is by the way to use a normal GET form instead and do the action job in a preRenderView listener:

<f:metadata>
  <f:viewParam name="value" value="#{bean.value}" />
  <f:event type="preRenderView" listener="#{bean.submit}" />
</f:metadata>
<form>
  <h:inputText id="value" value="#{bean.value}" />
  <input type="submit" value="submit" />
</form>

But this removes the possible benefit of using <f:ajax> in the form to do for example the validations.


frederickkaempfer added a comment - 18/Nov/11 09:21 AM

You don't even need an input text field. For example if you use an URL like this:

http://.../webpage.ext?value=%23{myBean.methodThatShouldBeSecure()}

The EL expression will get evaluated if the next navigation case uses includeViewParams. In that case I suspect your workaround will not work. Don't use includeViewParams before this is fixed.


balusc added a comment - 18/Nov/11 02:54 PM

Indeed, the `includeViewParams` should not be used until this issue is fixed.


frederickkaempfer added a comment - 19/Nov/11 12:07 PM

It seems the same problem exists in MyFaces.

A workaround could be to attach a String-to-String converter to the view parameters that automatically removes every occurrence of '#{' in the parameter value. To fix this issue UIViewParameter could do something similar in the decode method by default to increase the security of the default behavior.


arjan tijms added a comment - 19/Nov/11 01:01 PM

To fix this issue UIViewParameter could do something similar in the decode method by default to increase the security of the default behavior.

Sounds like a good first quick fix, since this is the place where dangerous values enter the system.

Instead of removing the '#{' characters, escaping might also be an option, for instance '#{myBean.methodThatShouldBeSecure}' could be turned into '#{'#{'}myBean.methodThatShouldBeSecure}', so that the result of evaluating the expression will yield a string. Care must be taken of course that the resulting string is not evaluated again.

However, as BalusC identified the problem to be within the URLBuilder, javax.faces.context.ExternalContext#encodeRedirectURL is also vulnerable, since this ultimately uses the same addParameters method. Should a warning be added to this method that values have to be "el-escaped" if el evaluation is undesirable, or should no el evaluation be done at all?


Jakob Korherr added a comment - 22/Nov/11 03:43 PM

Hi guys,

I created a little project that shows the vulnerability: http://code.google.com/a/apache-extras.org/p/jsf-includeviewparams-security-hole-example/

Also, I created the following blog entry, describing the problem: http://www.jakobk.com/2011/11/jsf-value-expression-injection-vulnerability/

Since this is a really big security hole and there are a lot of versions of JSF out there, which have it (Mojarra and MyFaces), we should really try to spread the word!

Regards,
Jakob


aschwart added a comment - 22/Nov/11 06:02 PM

The problem seems to be that Mojarra's implementation of ExternalContext.encodeRedirectURL() is explicitly performing EL evaluation on the values passed in via the "parameters" map. The spec says nothing about this behavior. I believe that it should be the caller's responsibility to perform EL evaluation on parameter values before calling encodeRedirectURL() if EL evaluation is desired.


rogerk added a comment - 22/Nov/11 07:45 PM

Fix for 2.1.5 and trunk.


rogerk added a comment - 22/Nov/11 07:49 PM

Aldo fix in 2.0.7


arjan tijms added a comment - 22/Nov/11 08:47 PM

The problem seems to be that Mojarra's implementation of ExternalContext.encodeRedirectURL() is explicitly performing EL evaluation on the values passed in via the "parameters" map. The spec says nothing about this behavior. I believe that it should be the caller's responsibility to perform EL evaluation on parameter values before calling encodeRedirectURL() if EL evaluation is desired.

After giving this some more thought I'd more or less came to the same conclusion.

When I simply removed the EL evaluation in .com.sun.faces.context.URLBuilde#addParameters, the test com.sun.faces.application.TestNavigationHandler fails at the following assertion:

// ensure redirect parameter el evaluation is performed more than once
NavigationCase ncase = cnh.getNavigationCase(getFacesContext(),
                                                 null,
                                                 "redirectOutcome3");
String url = getFacesContext().getExternalContext().encodeRedirectURL("/path.xhtml", ncase.getParameters());
System.out.println("URL: " + url);
assertTrue(url.contains("param=1"));

This reveals why the EL evaluation was needed in the first place for the encodeRedirectURL method, as the navigation case contains this:

<navigation-case>
    <from-outcome>redirectOutcome3</from-outcome>
      <to-view-id>/page2.xhtml</to-view-id>
      <redirect>
         <view-param>
            <name>param</name>
            <value>#{navbean.increment}</value>
         </view-param>
      </redirect>
</navigation-case>

aschwart added a comment - 22/Nov/11 09:00 PM

@arjan tijms - Great, thank for tracking this down.

Looks like the problem is that NavigationHandlerImpl assumes that ViewHandler.getRedirectURL() (which calls ExternalContext.encodeRedirectURL()) will perform EL evaluation of the parameter values that are passed in. This is unspecified/undesired (and as we've seen, unsecured) behavior. The proper solution is to:

1. Remove the EL evaluation code from ExternalContext.encodeRedirectURL().
2. Update NavigationHandlerImpl to perform EL evaluation before calling ViewHandler.getRedirectURL().


arjan tijms added a comment - 22/Nov/11 09:44 PM

Update NavigationHandlerImpl to perform EL evaluation before calling ViewHandler.getRedirectURL().

That's what I was thinking too. javax.faces.application.NavigationCase#getRedirectURL probably has to be done as well.

So, for instance something like:

NavigationCase
public URL getRedirectURL(FacesContext context) throws MalformedURLException {

        ExternalContext extContext = context.getExternalContext();

        return new URL(extContext.getRequestScheme(),
                extContext.getRequestServerName(),
                extContext.getRequestServerPort(),
                context.getApplication().getViewHandler().getRedirectURL(context,
                                                               getToViewId(context),
                                                               Util.evaluateExpressions(getParameters()),
                                                               isIncludeViewParams()));

    }

and

NavigationHandlerImpl#handleNavigation
if (caseStruct.navCase.isRedirect() || isUIViewActionBroadcastAndViewdsDiffer) {
    // perform a 302 redirect.
    String redirectUrl =
          viewHandler.getRedirectURL(context,
                                     caseStruct.viewId,
                                     Util.evaluateExpressions(caseStruct.navCase.getParameters()),
                                     caseStruct.navCase.isIncludeViewParams());

lu4242 added a comment - 22/Nov/11 09:59 PM

I was thinking the same. Really this should be done in any place where NavigationCase.getParameters() is called.


Ed Burns added a comment - 22/Nov/11 11:01 PM

Reproducer from Jakob Korherr.


arjan tijms added a comment - 22/Nov/11 11:48 PM

An initial quick patch that implements the idea discussed here. Evaluation is moved away from the UrlBuilder and into the locations where navigation outcomes are computed. Because of the API/RI distinction, I had to copy paste the evaluation code to NavigationCase, which IMO is not so pretty.

Unit tests pass, but I haven't done any other testing or checking whether this is correct.


rogerk added a comment - 23/Nov/11 01:52 PM

Ed starting working on this issue.


jhorstmann added a comment - 23/Nov/11 04:26 PM

The method isExpression in the attached patch and in com.sun.faces.el.ELUtils seems to contain a duplicate calculation of the start index.

It also contains another bug if the tested string contains a closing brace before start of an el expression. This is easily fixable by rewriting it using the two-parameter version of indexOf as follows:

public static boolean isExpression(String expression) {
    if (null == expression) {
        return false;
    }
    //check to see if attribute has an expression
    int start = expression.indexOf("#{");
    return start != -1 && expression.indexOf('}', start+2) != -1;
}

Some testcases, the last one fails in the current version:

Assert.assertTrue(isExpression("#{test}"));
Assert.assertTrue(isExpression("abc#{test}"));
Assert.assertTrue(isExpression("#{test}abc"));
Assert.assertTrue(isExpression("abc#{test}abc"));

Assert.assertTrue(isExpression("x}x#{test}"));

Ed Burns added a comment - 23/Nov/11 05:53 PM - edited

I would like to suggest an alternate approach. Dan Allen, the original author of this code, was clearly adding a useful feature here, but I think it was exposed too broadly.

We want to allow expressions to be evaluated if they originate from the Application Configuration Resources, but not if they originate from the query string or POST data.

I will attempt to produce a fix that follows that approach.


Ed Burns added a comment - 23/Nov/11 08:41 PM

Much less invasive fix.


Ed Burns added a comment - 23/Nov/11 09:13 PM

Do not allow the value of UIViewParam to be an EL Expression http://java.net/jira/browse/JAVASERVERFACES-2247

SECTION: Modified Files
----------------------------
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

  • New message.

+severe.uiviewparam_value_is_expression=The value of a UIParameter must not be an expression literal. Ignoring expression value {0}.

A jsf-tools/template-src/SharedUtils.java
M jsf-api/build-pre-maven-rename.xml
M jsf-api/build.xml

  • In preparation for solidifying ELUtils.isExpression() into a more
    robust implementation and replacing all usages with
    SharedUtil.isExpression(), just put this new class in template-src for
    now.

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

  • Refactor getStringValueFromModel() to exclude the possibility of the
    value of a UIViewParameter being string literal that is an EL
    expression.

M jsf-test/build.xml

  • include new testcase

A jsf-test/JAVASERVERFACES-2247
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit/Issue2247TestCase.java
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/resources
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/pom.xml
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war/UserBean.java
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/i_mojarra_2247_war.xhtml
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/faces-config.xml
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/beans.xml
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/web.xml
A jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/pom.xml
A jsf-test/JAVASERVERFACES-2247/build.xml

  • the new testcase

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/component/UIViewParameter.java
Adding jsf-test/JAVASERVERFACES-2247
Adding jsf-test/JAVASERVERFACES-2247/build.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit/Issue2247TestCase.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/resources
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war/UserBean.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/beans.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/faces-config.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/web.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/i_mojarra_2247_war.xhtml
Sending jsf-test/build.xml
Adding jsf-tools/template-src/SharedUtils.java
Transmitting file data ........................
Committed revision 9467.


aschwart added a comment - 23/Nov/11 09:34 PM

Ed -

I think that your alternate fix misidentifies the source of the problem. This fix assumes that an EL-bound UIViewParameter value must not evaluate to a String that in turn contains an EL expression. While this may be unusual, this is not forbidden, and is not inherently problematic.

The real problem is that the result of the UIViewParameter's EL-evaluated value is being run through EL evaluation a second time. This should never happen. This is only happening due to an implementation bug - ie. due to the fact that the implementation of ExternalContext.encodeRedirectURL() happens to implicitly perform EL evaluation on parameter values, even though this is not specified (or desired) behavior.

The fix is simple:

  • Stop performing EL evaluation under encodeRedirectURL().
  • Update implementation code that was depending on this magic behavior to instead perform EL evaluation before calling encodeRedirectURL().

I very much recommend that we go with a solution along these lines.


Ed Burns added a comment - 24/Nov/11 02:18 AM

Thanks for your analysis, Andy. I will commit another patch that does what you suggest.

In the meantime, I have migrated my existing fix to the trunk, for consistency.

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/component/UIViewParameter.java
Adding jsf-test/JAVASERVERFACES-2247
Adding jsf-test/JAVASERVERFACES-2247/build.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit/Issue2247TestCase.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/resources
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war/UserBean.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/beans.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/faces-config.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/web.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/i_mojarra_2247_war.xhtml
Sending jsf-test/build.xml
Adding jsf-tools/template-src/SharedUtils.java
Transmitting file data ..............
Committed revision 9468.


Ed Burns added a comment - 24/Nov/11 03:59 PM

Hello Andy, if I do as you suggest, then this case, which the spec does require to work, no longer works.

<navigation-rule>
<from-view-id>*</from-view-id>
<navigation-case>
<from-outcome>explicit</from-outcome>
<to-view-id>/i_mojarra_2247_war.xhtml</to-view-id>
<redirect include-view-params="true">
<view-param>
<name>elViewParam</name>
<value>#{3 + 3}</value>
</view-param>
</redirect>
</navigation-case>
</navigation-rule>

I am about to attach a stack trace that shows the necessity of this invocation sequence in light of your suggestion regarding what happens in the implementation of ExternalContext.encodeRedirectUrl().


aschwart added a comment - 28/Nov/11 07:39 PM

Hey Ed - My recommendation was to perform the necessary EL evaluation before calling ExternalContext.encodeRedirectURL(). Why would this break the above use case? (It shouldn't. If it does we've got bigger problems.)


ssilvert added a comment - 29/Nov/11 06:00 PM

Hi Ed,

Is this issue considered closed?

Also, is 2.1.5 considered released? It does not show up on the downloads page but it does show up in this releases repo:
https://maven.java.net/content/repositories/releases/org/glassfish/javax.faces/2.1.5/javax.faces-2.1.5.jar


Ed Burns added a comment - 29/Nov/11 09:47 PM

Committed to 2.0 branch.

Sending jsf-api
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/component/UIViewParameter.java
Sending jsf-ri
Adding jsf-test/JAVASERVERFACES-2247
Adding jsf-test/JAVASERVERFACES-2247/build.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/java/com/sun/faces/i_mojarra_2247_htmlunit/Issue2247TestCase.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_htmlunit/src/main/resources
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/pom.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/java/com/sun/faces/i_mojarra_2247_war/UserBean.java
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/beans.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/faces-config.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/WEB-INF/web.xml
Adding jsf-test/JAVASERVERFACES-2247/i_mojarra_2247_war/src/main/webapp/i_mojarra_2247_war.xhtml
Sending jsf-test/build.xml
Adding jsf-tools/template-src/SharedUtils.java
Transmitting file data ......................
Committed revision 9478.


Ed Burns added a comment - 29/Nov/11 09:49 PM

Re: Stan: yes, 2.1.5 is considered released. I'm behind in updating the page.

Re: Andy. 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.


arjan tijms added a comment - 29/Nov/11 10:22 PM

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).


Ed Burns added a comment - 02/Dec/11 05:41 AM

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.


Ed Burns added a comment - 02/Dec/11 04:24 PM

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.


Ed Burns added a comment - 02/Dec/11 05:31 PM

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.


Ed Burns added a comment - 02/Dec/11 07:28 PM

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.