WW-5408 add option to not fallback to empty namespace when unresolved #912
Conversation
| @@ -230,6 +230,8 @@ public final class StrutsConstants { | |||
| public static final String STRUTS_XWORKCONVERTER = "struts.xworkConverter"; | |||
|
|
|||
| public static final String STRUTS_ALWAYS_SELECT_FULL_NAMESPACE = "struts.mapper.alwaysSelectFullNamespace"; | |||
| /** Disable fallback to empty namespace when request namespace didn't match any in action configuration */ | |||
| public static final String STRUTS_DISABLE_EMPTY_NAMESPACE_FALLBACK = "struts.disableActionConfigFallbackToEmptyNamespace"; | |||
There was a problem hiding this comment.
@lukaszlenart Any thoughts on what the best name for this constant might be?
There was a problem hiding this comment.
Wouldn't be better to have struts.actionConfig.fallbackToEmptyNamespace with default value set to true instead?
There was a problem hiding this comment.
Sounds reasonable to me - @jefferyxhy could you please update?
There was a problem hiding this comment.
@kusalk @lukaszlenart updated. Thanks
… struts.actionConfig.fallbackToEmptyNamespace
| @@ -583,11 +590,10 @@ public ActionConfig getActionConfig(String namespace, String name) { | |||
| } | |||
|
|
|||
| // fail over to empty namespace | |||
| if (config == null && StringUtils.isNotBlank(namespace)) { | |||
| if (config == null && StringUtils.isNotBlank(namespace) && ("/".equals(namespace) || fallbackToEmptyNamespace)) { | |||
There was a problem hiding this comment.
I would extract this logic into a private method to name it, something like
if (config == null && shouldFallbackToEmptyNamespaces(namespace)) {
...| @@ -459,9 +460,12 @@ protected synchronized RuntimeConfiguration buildRuntimeConfiguration() throws C | |||
| boolean appendNamedParameters = Boolean.parseBoolean( | |||
| container.getInstance(String.class, StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS) | |||
| ); | |||
| boolean fallbackToEmptyNamespace = Boolean.parseBoolean( | |||
| Optional.ofNullable(container.getInstance(String.class, StrutsConstants.STRUTS_ACTION_CONFIG_FALLBACK_TO_EMPTY_NAMESPACE)).orElse("true") | |||
There was a problem hiding this comment.
Instead of this fallback I would create a new entry into default.properties with value set to true and document it - this will help others understand the change
There was a problem hiding this comment.
@jefferyxhy and I just discussed this one and one of the drawbacks of putting it in default.properties is that it isn't read by unit tests and causes a bunch of test failures, as the unit tests will default to false. To get around this we could additionally add the constant to StrutsDefaultConfigurationProvider as well as default.properties.
I'm personally not too fussed. In the past I've deliberately made constants default to false to try sidestep this issue. Let us know know what you would prefer
There was a problem hiding this comment.
Understood, yet default.properties is used as a documentation by users so I would define the constant there with short description and keep fallback to true as above
…efault.properties
WW-5408
Reason
There is a case we want to enhance to improve security for struts url mapping: Be able to prevent arbitrary namespace fallback to empty (root) namespace when not match
Changes/ Solution
STRUTS_DISABLE_EMPTY_NAMESPACE_FALLBACK [struts.disableActionConfigFallbackToEmptyNamespace]to disable fallback to empty namespace when not matchResult & Impact
struts.disableActionConfigFallbackToEmptyNamespaceisnull, no difference.struts.disableActionConfigFallbackToEmptyNamespaceastrue, not matched namepsace WILL NOT fallback to empty namespace anymore, and struts will threwConfigurationException, it relies on the application to handle this exception.