Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • Component/s: Core Actions
    • Labels:
      None

      Description

      Struts 2 already supports Strict DMI but it's disabled by default. Strict DMI should be always enable to allow access only specific methods.

        Activity

        Hide
        quaff zhouyanming added a comment - - edited

        setter/getter must be designed to ensure safety, getter method is readonly will not change server state, setter method can always invoked by queryString, setter can not invoked by DMI, action method should be

        public String method( )

        , only String getter can be invoked.

        Show
        quaff zhouyanming added a comment - - edited setter/getter must be designed to ensure safety, getter method is readonly will not change server state, setter method can always invoked by queryString, setter can not invoked by DMI, action method should be public String method( ) , only String getter can be invoked.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        First you can have public setters/getters which can be invoked, next most of the applications that use standard configuration won't break - if method was configured to be an action via @Action annotation or method attribute in struts.xml it will automatically be added as an allowed method. Few people already tested large applications and didn't notice any problems. And there is no overhead at all.

        Show
        lukaszlenart Lukasz Lenart added a comment - First you can have public setters/getters which can be invoked, next most of the applications that use standard configuration won't break - if method was configured to be an action via @Action annotation or method attribute in struts.xml it will automatically be added as an allowed method. Few people already tested large applications and didn't notice any problems. And there is no overhead at all.
        Hide
        quaff zhouyanming added a comment -

        It works with actionConfig.addAllowedMethod(ActionConfig.REGEX_WILDCARD) and strict DMI needn't be false.
        I doubt this is useful, normally we have many methods in action to be invoked, methods shouldn't be invoked can marked as protected or private, what's the value of strict DMI?
        It will bring two problem:

        • It breaked compatibility, application with old version cannot upgrade smoothly, I wish one constant in struts.xml can disable strict DMI globally not per package.
        • It will bring a little overhead for checking strict DMI.
        Show
        quaff zhouyanming added a comment - It works with actionConfig.addAllowedMethod(ActionConfig.REGEX_WILDCARD) and strict DMI needn't be false. I doubt this is useful, normally we have many methods in action to be invoked, methods shouldn't be invoked can marked as protected or private, what's the value of strict DMI? It will bring two problem: It breaked compatibility, application with old version cannot upgrade smoothly, I wish one constant in struts.xml can disable strict DMI globally not per package. It will bring a little overhead for checking strict DMI.
        Show
        lukaszlenart Lukasz Lenart added a comment - https://cwiki.apache.org/confluence/display/WW/Action+Configuration#ActionConfiguration-StrictMethodInvocation
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        By default only execute method is allowed, even when SMI is disabled - I will add a note to the docs

        Show
        lukaszlenart Lukasz Lenart added a comment - By default only execute method is allowed, even when SMI is disabled - I will add a note to the docs
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        I think you are missing this logic in you PackageProvider

                if (pkgCfg.isStrictMethodInvocation()) {
                    actionConfig.addAllowedMethod(actionMethod);
                    actionConfig.addAllowedMethod(allowedMethods);
                    actionConfig.addAllowedMethod(pkgCfg.getGlobalAllowedMethods());
                } else {
                    actionConfig.addAllowedMethod(ActionConfig.REGEX_WILDCARD);
                }
        

        https://github.com/apache/struts/blob/master/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L924-L930

        Show
        lukaszlenart Lukasz Lenart added a comment - I think you are missing this logic in you PackageProvider if (pkgCfg.isStrictMethodInvocation()) { actionConfig.addAllowedMethod(actionMethod); actionConfig.addAllowedMethod(allowedMethods); actionConfig.addAllowedMethod(pkgCfg.getGlobalAllowedMethods()); } else { actionConfig.addAllowedMethod(ActionConfig.REGEX_WILDCARD); } https://github.com/apache/struts/blob/master/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L924-L930
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK7-master #371 (See https://builds.apache.org/job/Struts-JDK7-master/371/)
        WW-4540 Enable Strict DMI be default (jogep: rev ecde389ee6fa2a58318d5c78d0186edf8b189b63)

        • plugins/rest/src/main/resources/struts-plugin.xml
          WW-4540 Enable Strict DMI be default (jogep: rev a6241552b67986056da32a70ae75d69c5c40a878)
        • apps/rest-showcase/src/main/resources/struts.xml
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #371 (See https://builds.apache.org/job/Struts-JDK7-master/371/ ) WW-4540 Enable Strict DMI be default (jogep: rev ecde389ee6fa2a58318d5c78d0186edf8b189b63) plugins/rest/src/main/resources/struts-plugin.xml WW-4540 Enable Strict DMI be default (jogep: rev a6241552b67986056da32a70ae75d69c5c40a878) apps/rest-showcase/src/main/resources/struts.xml
        Hide
        quaff zhouyanming added a comment -

        I don't use xml for action, I am using one xml for base package, then use my own PackageProvider scan action classes, those package extends base package in xml which already strict-method-invocation="false", and pkgConfig.strictMethodInvocation(false) in java.

        Show
        quaff zhouyanming added a comment - I don't use xml for action, I am using one xml for base package, then use my own PackageProvider scan action classes, those package extends base package in xml which already strict-method-invocation="false", and pkgConfig.strictMethodInvocation(false) in java.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Can you share the whole action's configuration?

        Show
        lukaszlenart Lukasz Lenart added a comment - Can you share the whole action's configuration?
        Hide
        quaff zhouyanming added a comment -

        Test failed with both

        <package name="my-default" extends="struts-default" strict-method-invocation="false">
        

        and

        pkgConfig = new PackageConfig.Builder(packageName);
        pkgConfig.strictMethodInvocation(false);
        

        It throw exception

        This method: input for action setting is not allowed! - [unknown location]
        at com.opensymphony.xwork2.DefaultActionProxy.prepare(DefaultActionProxy.java:200)
        at org.apache.struts2.factory.StrutsActionProxy.prepare(StrutsActionProxy.java:63)
        at org.apache.struts2.factory.StrutsActionProxyFactory.createActionProxy(StrutsActionProxyFactory.java:37)
        at com.opensymphony.xwork2.DefaultActionProxyFactory.createActionProxy(DefaultActionProxyFactory.java:58)
        at org.apache.struts2.dispatcher.Dispatcher.serviceAction(Dispatcher.java:543)
        at org.apache.struts2.dispatcher.ExecuteOperations.executeAction(ExecuteOperations.java:81)
        at org.apache.struts2.dispatcher.filter.StrutsExecuteFilter.doFilter(StrutsExecuteFilter.java:88)

        Show
        quaff zhouyanming added a comment - Test failed with both <package name= "my-default" extends= "struts-default" strict-method-invocation= "false" > and pkgConfig = new PackageConfig.Builder(packageName); pkgConfig.strictMethodInvocation( false ); It throw exception This method: input for action setting is not allowed! - [unknown location] at com.opensymphony.xwork2.DefaultActionProxy.prepare(DefaultActionProxy.java:200) at org.apache.struts2.factory.StrutsActionProxy.prepare(StrutsActionProxy.java:63) at org.apache.struts2.factory.StrutsActionProxyFactory.createActionProxy(StrutsActionProxyFactory.java:37) at com.opensymphony.xwork2.DefaultActionProxyFactory.createActionProxy(DefaultActionProxyFactory.java:58) at org.apache.struts2.dispatcher.Dispatcher.serviceAction(Dispatcher.java:543) at org.apache.struts2.dispatcher.ExecuteOperations.executeAction(ExecuteOperations.java:81) at org.apache.struts2.dispatcher.filter.StrutsExecuteFilter.doFilter(StrutsExecuteFilter.java:88)
        Hide
        lukaszlenart Lukasz Lenart added a comment -
        <?xml version="1.0" encoding="UTF-8" ?>
        <!DOCTYPE struts PUBLIC
                "-//Apache Software Foundation//DTD Struts Configuration 2.5//EN"
                "http://struts.apache.org/dtds/struts-2.5.dtd">
        <struts>
          <package strict-method-invocation="false">
          ...
          </package
        </struts>
        
        

        https://cwiki.apache.org/confluence/display/WW/Action+Configuration#ActionConfiguration-StrictMethodInvocation

        Show
        lukaszlenart Lukasz Lenart added a comment - <?xml version= "1.0" encoding= "UTF-8" ?> <!DOCTYPE struts PUBLIC "-//Apache Software Foundation//DTD Struts Configuration 2.5//EN" "http://struts.apache.org/dtds/struts-2.5.dtd" > <struts> <package strict-method-invocation= "false" > ... </package </struts> https://cwiki.apache.org/confluence/display/WW/Action+Configuration#ActionConfiguration-StrictMethodInvocation
        Hide
        quaff zhouyanming added a comment -

        How to disable it?

        <constant name="struts.enable.DynamicMethodInvocation" value="true" />
        

        It seems doesn't works.

        Show
        quaff zhouyanming added a comment - How to disable it? <constant name= "struts.enable.DynamicMethodInvocation" value= "true" /> It seems doesn't works.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK7-master #364 (See https://builds.apache.org/job/Struts-JDK7-master/364/)
        WW-4540 Enable Strict DMI be default (jogep: rev ecde389ee6fa2a58318d5c78d0186edf8b189b63)

        • plugins/rest/src/main/resources/struts-plugin.xml
          WW-4540 Enable Strict DMI be default (jogep: rev a6241552b67986056da32a70ae75d69c5c40a878)
        • apps/rest-showcase/src/main/resources/struts.xml
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #364 (See https://builds.apache.org/job/Struts-JDK7-master/364/ ) WW-4540 Enable Strict DMI be default (jogep: rev ecde389ee6fa2a58318d5c78d0186edf8b189b63) plugins/rest/src/main/resources/struts-plugin.xml WW-4540 Enable Strict DMI be default (jogep: rev a6241552b67986056da32a70ae75d69c5c40a878) apps/rest-showcase/src/main/resources/struts.xml
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit a6241552b67986056da32a70ae75d69c5c40a878 in struts's branch refs/heads/master from Johannes Geppert
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=a624155 ]

        WW-4540 Enable Strict DMI be default

        • Add to global allowed methods to rest showcase configuration
        Show
        jira-bot ASF subversion and git services added a comment - Commit a6241552b67986056da32a70ae75d69c5c40a878 in struts's branch refs/heads/master from Johannes Geppert [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=a624155 ] WW-4540 Enable Strict DMI be default Add to global allowed methods to rest showcase configuration
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ecde389ee6fa2a58318d5c78d0186edf8b189b63 in struts's branch refs/heads/master from Johannes Geppert
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ecde389 ]

        WW-4540 Enable Strict DMI be default

        • Extend global allowed methods in rest plugin with rest plugin specific default methods
        Show
        jira-bot ASF subversion and git services added a comment - Commit ecde389ee6fa2a58318d5c78d0186edf8b189b63 in struts's branch refs/heads/master from Johannes Geppert [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ecde389 ] WW-4540 Enable Strict DMI be default Extend global allowed methods in rest plugin with rest plugin specific default methods
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/struts/pull/47

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/47
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit b8826816550422be78be1f7f86ef28f86ee3ca3c in struts's branch refs/heads/master from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=b882681 ]

        WW-4540 Implements Strict DMI aka SMI

        Show
        jira-bot ASF subversion and git services added a comment - Commit b8826816550422be78be1f7f86ef28f86ee3ca3c in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=b882681 ] WW-4540 Implements Strict DMI aka SMI
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user lukaszlenart opened a pull request:

        https://github.com/apache/struts/pull/47

        WW-4540: Strict DMI

        This PR enables `Strict DMI` be default (or rather it's always enabled). Thus will limit possible methods which can be called and executed as an action methods.

        Right now you can configure `global-allowed-methods` and `allowed-methods` via `struts.xml` only but I'm going to add support for annotations as well.

        To use the new functionality you must update DTD definition to `2.5`

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/lukaszlenart/struts strict-dmi

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/struts/pull/47.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #47


        commit 065b5b79ae068ab7891a4232a0769290fd21bb17
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:31:59Z

        Drops wildcard as a valid action method

        commit ce884e92a15ef601b0e119963d3c521fa68d8bb1
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:33:31Z

        Defines global-allowed-methods

        commit fd22e3a16c88ef0528c1e26e0d6bdfdf1c02c755
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:35:16Z

        Uses global-allowed-methods config para

        commit 55b8070048cbec0a6e08b1781f81b1bfdb3354f2
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:41:57Z

        Drops strict DMI

        commit fb0c4a58507c7fb1af135bb376af5b475f43d7ee
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:42:44Z

        Drops outdated attribute

        commit 4565993463f660e9be90b9fe9c3597ce54b58917
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:43:21Z

        Extends Unknown Handler to allowed check if method is allowed

        commit c3f4457b8b8ad6bd0e89646d825f2ef5f9f91118
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T12:43:31Z

        Fixes tests

        commit c1928ad06bdfbe245b1ed7d5bfeb07ed9bface37
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T16:36:04Z

        Fixes tests

        commit 3b31c428856766389ad6df4ba1edc3d60ecf5e24
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T16:36:29Z

        Adds support for wildcards

        commit 185530464b838b3aac9681b5ff5b16401ccef56d
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T16:36:41Z

        Simplifies implementation

        commit 47a01eab10d940fdc134cb666d3d2db0280d8ca8
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T18:28:45Z

        Fixes typo

        commit 63bb6e30e75facf5382608857494cf971f0378dd
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T19:06:06Z

        Adds missing comma

        commit 4c7a7dd6c02457cf006318ed4621b7c432cc478c
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T19:46:16Z

        Adds null-safety

        commit 77691563b9b8d2ad01c078a66d1ed207bf3611b3
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T19:46:47Z

        Implements required method

        commit dd406fbb04e755c0545c318c3ea099674fb78363
        Author: Lukasz Lenart <lukaszlenart@apache.org>
        Date: 2015-08-31T19:46:55Z

        Fixes test


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user lukaszlenart opened a pull request: https://github.com/apache/struts/pull/47 WW-4540 : Strict DMI This PR enables `Strict DMI` be default (or rather it's always enabled). Thus will limit possible methods which can be called and executed as an action methods. Right now you can configure `global-allowed-methods` and `allowed-methods` via `struts.xml` only but I'm going to add support for annotations as well. To use the new functionality you must update DTD definition to `2.5` You can merge this pull request into a Git repository by running: $ git pull https://github.com/lukaszlenart/struts strict-dmi Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/47.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #47 commit 065b5b79ae068ab7891a4232a0769290fd21bb17 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:31:59Z Drops wildcard as a valid action method commit ce884e92a15ef601b0e119963d3c521fa68d8bb1 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:33:31Z Defines global-allowed-methods commit fd22e3a16c88ef0528c1e26e0d6bdfdf1c02c755 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:35:16Z Uses global-allowed-methods config para commit 55b8070048cbec0a6e08b1781f81b1bfdb3354f2 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:41:57Z Drops strict DMI commit fb0c4a58507c7fb1af135bb376af5b475f43d7ee Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:42:44Z Drops outdated attribute commit 4565993463f660e9be90b9fe9c3597ce54b58917 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:43:21Z Extends Unknown Handler to allowed check if method is allowed commit c3f4457b8b8ad6bd0e89646d825f2ef5f9f91118 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T12:43:31Z Fixes tests commit c1928ad06bdfbe245b1ed7d5bfeb07ed9bface37 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T16:36:04Z Fixes tests commit 3b31c428856766389ad6df4ba1edc3d60ecf5e24 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T16:36:29Z Adds support for wildcards commit 185530464b838b3aac9681b5ff5b16401ccef56d Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T16:36:41Z Simplifies implementation commit 47a01eab10d940fdc134cb666d3d2db0280d8ca8 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T18:28:45Z Fixes typo commit 63bb6e30e75facf5382608857494cf971f0378dd Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T19:06:06Z Adds missing comma commit 4c7a7dd6c02457cf006318ed4621b7c432cc478c Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T19:46:16Z Adds null-safety commit 77691563b9b8d2ad01c078a66d1ed207bf3611b3 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T19:46:47Z Implements required method commit dd406fbb04e755c0545c318c3ea099674fb78363 Author: Lukasz Lenart <lukaszlenart@apache.org> Date: 2015-08-31T19:46:55Z Fixes test

          People

          • Assignee:
            lukaszlenart Lukasz Lenart
            Reporter:
            lukaszlenart Lukasz Lenart
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development