Struts 2
  1. Struts 2
  2. WW-4257

ParametersInterceptor uses same method on ParameterNameAware interface to validate parameters and properties

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.16
    • Fix Version/s: 2.3.20
    • Component/s: None
    • Labels:
      None

      Description

      With version 2.3.16, the ParametersInterceptor uses the same method to validate parameter names and property names.
      As we use the ParameterNameAware interface to implement parameter whitelisting on action level, this breaks our case.

      It might not be how it is intended, but validating a property independent of the actual bean breaks our current implementation.

      Possible fixes would be:

      • alter ParameterNameAware to have an additional separate method to validate properties
      • introduce a new PropertyNameAware interface
      • introduce a new ParameterAndPropertyNameAware interface

      One could also consider to ignore the ParameterNameAware interface when validating properties, as for a parameter foo.bar, the values foo.bar, foo, and bar are passed to the ParameterNameAware interface, which one could see as a bit redundant. Especially given the fact that a context in the case of property validation is not provided. Therefore, it is impossible for the implementation to distinguish between a parameter and a property.

      1. ww-4257.patch
        5 kB
        Christoph Lenggenhager

        Activity

        Hide
        Lukasz Lenart added a comment -
        Show
        Lukasz Lenart added a comment - You can help testing http://markmail.org/thread/soybemgiz5gmkyno
        Hide
        Lukasz Lenart added a comment -

        Underway....

        Show
        Lukasz Lenart added a comment - Underway....
        Hide
        Paresh added a comment -

        When will the version 2.3.17 released? We have an application which needs to be upgraded with 2.3.16 but the issues identified in this ticket is breaking our code.

        Show
        Paresh added a comment - When will the version 2.3.17 released? We have an application which needs to be upgraded with 2.3.16 but the issues identified in this ticket is breaking our code.
        Hide
        Lukasz Lenart added a comment -

        Patch applied, thanks!

        Show
        Lukasz Lenart added a comment - Patch applied, thanks!
        Hide
        Christoph Lenggenhager added a comment -

        Have you had time to consider my suggested patch? This is a blocker for us to update to 2.3.16...

        Show
        Christoph Lenggenhager added a comment - Have you had time to consider my suggested patch? This is a blocker for us to update to 2.3.16...
        Hide
        Christoph Lenggenhager added a comment -

        Hmm, I studied WW-4083 again. It seems to me that WW-4154 made this obsolete. It is no longer possible that the parameter interceptor accepts a parameter solely based on the ParameterNameAware interface. Therefore, the problem that the interceptor accepts the parameter, but SecurityMemberAccess rejects it, does no longer exist.
        I would therefore rather suggest to remove the PropertiesJudge again and only rely on the statically configured property checks in SecurityMemberAccess.
        Patch for that is attached.

        Show
        Christoph Lenggenhager added a comment - Hmm, I studied WW-4083 again. It seems to me that WW-4154 made this obsolete. It is no longer possible that the parameter interceptor accepts a parameter solely based on the ParameterNameAware interface. Therefore, the problem that the interceptor accepts the parameter, but SecurityMemberAccess rejects it, does no longer exist. I would therefore rather suggest to remove the PropertiesJudge again and only rely on the statically configured property checks in SecurityMemberAccess . Patch for that is attached.
        Hide
        Lukasz Lenart added a comment -

        So, introducing a new interface - ParameterNameAware and using it in SecurityMemberAccess is a good idea? If you can prepare a patch, I will be very grateful

        Show
        Lukasz Lenart added a comment - So, introducing a new interface - ParameterNameAware and using it in SecurityMemberAccess is a good idea? If you can prepare a patch, I will be very grateful
        Hide
        Christoph Lenggenhager added a comment -

        Generally, I'm all for WW-4154 and restoring the old behavior.
        The problem is that you have also introduced this behavior in the SecurityMemberAccess class where it wasn't present before.

        I could well imagine that it is a good thing to have a similar mechanism to check member names as it is given for the parameters, but it should not use the same interface. The implementing action has no means to grasp the context and therefore does not know whether it is validating the parameter 'foo' or the member 'foo'.

        Show
        Christoph Lenggenhager added a comment - Generally, I'm all for WW-4154 and restoring the old behavior. The problem is that you have also introduced this behavior in the SecurityMemberAccess class where it wasn't present before. https://github.com/apache/struts2/commit/f6a07e63e62e13e72cc4ee3a034398b4b30abe03#diff-133b11975bdb4f580966d9abea9ffbe9 introduced the possibility to check member names using the ParameterNameAware interface https://github.com/apache/struts2/commit/4e98aaaa1b08cc37374d06e77cf78000d98c5ff0#diff-133b11975bdb4f580966d9abea9ffbe9 changed this check from an ORing the static configuration to ANDing it with the ParameterNameAware interface I could well imagine that it is a good thing to have a similar mechanism to check member names as it is given for the parameters, but it should not use the same interface. The implementing action has no means to grasp the context and therefore does not know whether it is validating the parameter 'foo' or the member 'foo'.
        Hide
        Lukasz Lenart added a comment -

        WW-4154 supposed restore the old behaviour so I'm bit confuse

        What you mean is that the logic in SecurityMemberAccess should also be reverted, right? I mean I can recall that I made some changes in that area to match logic in ParametersInterceptor when used with ParameterNameAware.

        Show
        Lukasz Lenart added a comment - WW-4154 supposed restore the old behaviour so I'm bit confuse What you mean is that the logic in SecurityMemberAccess should also be reverted, right? I mean I can recall that I made some changes in that area to match logic in ParametersInterceptor when used with ParameterNameAware .
        Hide
        Christoph Lenggenhager added a comment -

        I'm well aware that this was not an issue until WW-4154 that enforced the according logical AND in SecurityMemberAccess.
        I can also provide a patch, if there are suggestions on how this should be addressed.

        Show
        Christoph Lenggenhager added a comment - I'm well aware that this was not an issue until WW-4154 that enforced the according logical AND in SecurityMemberAccess . I can also provide a patch, if there are suggestions on how this should be addressed.

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Christoph Lenggenhager
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development