Struts 2
  1. Struts 2
  2. WW-3755

make ParametersInterceptor.acceptedParamNames public and static

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.12
    • Component/s: Core Interceptors
    • Labels:
      None
    • Flags:
      Patch

      Description

      let it can be referenced by other classes

      1. patch.txt
        0.9 kB
        zhouyanming
      2. WW-3755.patch
        1.0 kB
        zhouyanming

        Activity

        Hide
        Hudson added a comment -

        Integrated in Struts2-JDK6 #589 (See https://builds.apache.org/job/Struts2-JDK6/589/)
        WW-3755 extracts acceptedParamNames to constant (Revision 1423615)

        Result = UNSTABLE
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Show
        Hudson added a comment - Integrated in Struts2-JDK6 #589 (See https://builds.apache.org/job/Struts2-JDK6/589/ ) WW-3755 extracts acceptedParamNames to constant (Revision 1423615) Result = UNSTABLE lukaszlenart : Files : /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Hide
        Lukasz Lenart added a comment -

        Extracted acceptedParamNames to constant so it can be used in any other place

        Show
        Lukasz Lenart added a comment - Extracted acceptedParamNames to constant so it can be used in any other place
        Hide
        Lukasz Lenart added a comment -

        zhouyanming yes, as Maurizio mentioned is needed to limits parameter names.

        Show
        Lukasz Lenart added a comment - zhouyanming yes, as Maurizio mentioned is needed to limits parameter names.
        Hide
        zhouyanming added a comment -

        @Maurizio, the regexp pattern was introduced because s2-009 ,now we have setParameter(),is regexp pattern necessary any more?

        Show
        zhouyanming added a comment - @Maurizio, the regexp pattern was introduced because s2-009 ,now we have setParameter(),is regexp pattern necessary any more?
        Hide
        Maurizio Cucchiara added a comment -

        @zhouyanming setParameter mitigates some OGNL risks (like evaluation expression, see s2-009 for further details), the regexp pattern limits the variable name which may be accepted.

        @Lukasz
        Good catch!

        Show
        Maurizio Cucchiara added a comment - @zhouyanming setParameter mitigates some OGNL risks (like evaluation expression, see s2-009 for further details), the regexp pattern limits the variable name which may be accepted. @Lukasz Good catch!
        Hide
        Lukasz Lenart added a comment -

        So maybe it would be better to move all the default regexps to dedicated class, like Constants or Regexps.

        Show
        Lukasz Lenart added a comment - So maybe it would be better to move all the default regexps to dedicated class, like Constants or Regexps.
        Hide
        zhouyanming added a comment - - edited

        @Maurizio ,thanks very much, if setParameter() make sense,why ParametersInterceptor needs "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'
        )))*"

        line 293 in ParametersInterceptor : newStack.setParameter(name, value);

        Show
        zhouyanming added a comment - - edited @Maurizio ,thanks very much, if setParameter() make sense,why ParametersInterceptor needs "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+' )))*" line 293 in ParametersInterceptor : newStack.setParameter(name, value);
        Hide
        zhouyanming added a comment -

        @Maurizio
        I want reuse this String:
        public static final String defaultAcceptedParamNames = "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'
        )))*";

        if struts has update it's value,I needn't modify my code.

        Show
        zhouyanming added a comment - @Maurizio I want reuse this String: public static final String defaultAcceptedParamNames = "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+' )))*"; if struts has update it's value,I needn't modify my code.
        Hide
        Maurizio Cucchiara added a comment -

        NB: there are two families of parameter (and relative accessor):

        • accepted
        • accept

        I think that this is the reason of such misunderstanding.

        Show
        Maurizio Cucchiara added a comment - NB: there are two families of parameter (and relative accessor): accepted accept I think that this is the reason of such misunderstanding.
        Hide
        Maurizio Cucchiara added a comment -

        @Rene
        The setAcceptParamNames, which I'm pasting below for your convenience, is not the acceptedParamNames setter, and it never refers it (in very truth nothing refers acceptedParamNames aside from acceptedPattern.

        public void setAcceptParamNames(String commaDelim) {
                Collection<String> acceptPatterns = ArrayUtils.asCollection(commaDelim);
                if (acceptPatterns != null) {
                    acceptParams = new HashSet<Pattern>();
                    for (String pattern : acceptPatterns) {
                        acceptParams.add(Pattern.compile(pattern));
                    }
                }
            }
        
        Show
        Maurizio Cucchiara added a comment - @Rene The setAcceptParamNames , which I'm pasting below for your convenience, is not the acceptedParamNames setter, and it never refers it (in very truth nothing refers acceptedParamNames aside from acceptedPattern . public void setAcceptParamNames( String commaDelim) { Collection< String > acceptPatterns = ArrayUtils.asCollection(commaDelim); if (acceptPatterns != null ) { acceptParams = new HashSet<Pattern>(); for ( String pattern : acceptPatterns) { acceptParams.add(Pattern.compile(pattern)); } } }
        Hide
        Maurizio Cucchiara added a comment -

        OK,
        now I'm really confused.
        Your patch has referenced a private String until last patch (which IMHO is shareable among many instances), now you're talking about a pattern.
        Have you considered to extend ParametersInterceptor and overwrite setParameters method?

        Anyway for security reason, you should change the code below from:

        temp.setValue(entry.getKey(), entry.getValue());
        

        to:

        temp.setParameter(entry.getKey(), entry.getValue());
        
        Show
        Maurizio Cucchiara added a comment - OK, now I'm really confused. Your patch has referenced a private String until last patch (which IMHO is shareable among many instances), now you're talking about a pattern. Have you considered to extend ParametersInterceptor and overwrite setParameters method? Anyway for security reason, you should change the code below from: temp.setValue(entry.getKey(), entry.getValue()); to: temp.setParameter(entry.getKey(), entry.getValue());
        Hide
        zhouyanming added a comment -

        this is a better patch

        Show
        zhouyanming added a comment - this is a better patch
        Hide
        zhouyanming added a comment - - edited

        @Maurizio, I want reuse acceptedPattern for security in my action class

        			ValueStack temp = valueStackFactory.createValueStack();
        			temp.set(getEntityName(), entity);
        			Map<String, Object> context = temp.getContext();
        			Map<String, Object> parameters = ActionContext.getContext()
        					.getParameters();
        			try {
        				ReflectionContextState.setCreatingNullObjects(context, true);
        				ReflectionContextState.setDenyMethodExecution(context, true);
        				for (Map.Entry<String, Object> entry : parameters.entrySet())
        					if (acceptedPattern.matcher(entry.getKey()).matches())
        						temp.setValue(entry.getKey(), entry.getValue());
        			} finally {
        				ReflectionContextState.setCreatingNullObjects(context, false);
        				ReflectionContextState.setDenyMethodExecution(context, false);
        			}
        
        Show
        zhouyanming added a comment - - edited @Maurizio, I want reuse acceptedPattern for security in my action class ValueStack temp = valueStackFactory.createValueStack(); temp.set(getEntityName(), entity); Map< String , Object > context = temp.getContext(); Map< String , Object > parameters = ActionContext.getContext() .getParameters(); try { ReflectionContextState.setCreatingNullObjects(context, true ); ReflectionContextState.setDenyMethodExecution(context, true ); for (Map.Entry< String , Object > entry : parameters.entrySet()) if (acceptedPattern.matcher(entry.getKey()).matches()) temp.setValue(entry.getKey(), entry.getValue()); } finally { ReflectionContextState.setCreatingNullObjects(context, false ); ReflectionContextState.setDenyMethodExecution(context, false ); }
        Hide
        Rene Gielen added a comment -

        Maurizio, are you sure you saw
        public void setAcceptParamNames(String commaDelim)
        which is the instance based setter for the property? As Lukasz and Dave already mentioned, this parameter can be changed per invocation within any given stack, which is a good thing.

        Show
        Rene Gielen added a comment - Maurizio, are you sure you saw public void setAcceptParamNames(String commaDelim) which is the instance based setter for the property? As Lukasz and Dave already mentioned, this parameter can be changed per invocation within any given stack, which is a good thing.
        Hide
        Maurizio Cucchiara added a comment -

        ATM there is no way to change the value of acceptedParamNames: it's private and nothing inside the code tries to change its value.
        FWIW the code:

        private String acceptedParamNames = "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'\\)))*";
        private Pattern acceptedPattern = Pattern.compile(acceptedParamNames);
        

        could be changed to

        private Pattern acceptedPattern = Pattern.compile("\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'\\)))*");
        

        without any side effects (it could also be final)

        Show
        Maurizio Cucchiara added a comment - ATM there is no way to change the value of acceptedParamNames : it's private and nothing inside the code tries to change its value. FWIW the code: private String acceptedParamNames = "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'\\)))*" ; private Pattern acceptedPattern = Pattern.compile(acceptedParamNames); could be changed to private Pattern acceptedPattern = Pattern.compile( "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'\\)))*" ); without any side effects (it could also be final)
        Hide
        Lukasz Lenart added a comment -

        @Maurizio why cannot be more than one ?

        Show
        Lukasz Lenart added a comment - @Maurizio why cannot be more than one ?
        Hide
        Dave Newton added a comment -

        If it's static, acceptedParamNames would be shared across all instances. Since there may be multiple instances, why couldn't there be different values for acceptedParamNames?

        Show
        Dave Newton added a comment - If it's static, acceptedParamNames would be shared across all instances. Since there may be multiple instances, why couldn't there be different values for acceptedParamNames?
        Hide
        Maurizio Cucchiara added a comment -

        Lukasz, I got what you mean, but even if you have more than one instance of PI, there cannot be more than one value for acceptedParamNames (the one assigned in the class).
        (Again ATM I don't see the usefulness in making it public).

        Show
        Maurizio Cucchiara added a comment - Lukasz, I got what you mean, but even if you have more than one instance of PI, there cannot be more than one value for acceptedParamNames (the one assigned in the class). (Again ATM I don't see the usefulness in making it public).
        Hide
        Lukasz Lenart added a comment - - edited

        Interceptors are created per stack, so for each different stack you can have a different acceptedParamNames

        Show
        Lukasz Lenart added a comment - - edited Interceptors are created per stack, so for each different stack you can have a different acceptedParamNames
        Hide
        Maurizio Cucchiara added a comment -

        It cannot be static as there can be multiple instances of a given interceptor, IIRC.

        @Dave I don't get you (mostly because I went to bed late yesterday), what's the matter with a constant shared by multiple instances?
        @zhouyanming may I ask you what is your use case and why should expose the string pattern be so useful (I guess there are many others way to achieve what you're looking for, maybe we could find out the best way)?

        Show
        Maurizio Cucchiara added a comment - It cannot be static as there can be multiple instances of a given interceptor, IIRC. @Dave I don't get you (mostly because I went to bed late yesterday), what's the matter with a constant shared by multiple instances? @zhouyanming may I ask you what is your use case and why should expose the string pattern be so useful (I guess there are many others way to achieve what you're looking for, maybe we could find out the best way)?
        Hide
        Dave Newton added a comment -

        It cannot be static as there can be multiple instances of a given interceptor, IIRC.

        Show
        Dave Newton added a comment - It cannot be static as there can be multiple instances of a given interceptor, IIRC.
        Hide
        zhouyanming added a comment -

        this is patch

        Show
        zhouyanming added a comment - this is patch

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            zhouyanming
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development