URL Segment validation wont allow certain urls

[mediabeastnz]

It seems the SiteTree::validURLSegment function wont allow you to create any sub-pages with the name/url of "menu" as it seems to think "Menu" and "getMenu" are actions.

This doesn't happen to root level pages.

Replication steps: composer create-project silverstripe/installer menutest 3.5 then /admin && create a new page under "About us", call it "Menu" -> watch the URL Segment auto update to 'menu-2'

[tractorcow]

Yes, that's right. Menu is an action on ContentController. It's not an ALLOWED action, but it is a potential action, and thus is blocked for security reasons.

public function Menu($level) {
    return $this->getMenu($level);
}

It doesn't happen on root level pages because there is no parent controller that could potentially have this method invoked.

This is working as intended, or at least by design.

[thezenmonkey]

I wonder if the Menu function should be renamed to something more specific for 4.0. SiteMenu or SiteNavigation. SiteNavigation aligns better with HTML semantics.

On Dec 11, 2016, at 5:01 PM, Damian Mooyman notifications@github.com wrote:

Yes, that's right. Menu is an action on ContentController. It's not an ALLOWED action, but it is a potential action, and thus is blocked for security reasons.

public function Menu($level) { return $this->getMenu($level); } It doesn't happen on root level pages because there is no parent controller that could potentially have this method invoked.

This is working as intended, or at least by design.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[mediabeastnz]

If it's not an action then it should get through ok?! Maybe that one function needs to be allowed passed validation.

If there's a work around you can think of that'd be super helpful. I'm assuming renaming that function would be more than a patch update?

[tractorcow]

If it's not an action then it should get through ok?!

It will see that it's attempting to invoke an action, and it will fail with an error that this action is not allowed.

[mediabeastnz]

How would i go about submitting a pull request without modifying the name of that function?

[tractorcow]

The problem is that what you are really trying to do is rewrite request handling so that non-allowed actions are ignored. i.e. (pseudocode)

$action = $this->findAction($request);
$actions = $this->getAllowedActions();
if (in_array($action, $actions)) {
    $this->doAction($action);
} else {
    // Search for sub-pages and render that instead
}

The problem is that currently our pipeline is more similar to:

$action = $this->findAction($request);
if (!$action) {
    // Search for sub-pages
}
$actions = $this->getAllowedActions();
if (in_array($action, $actions)) {
    $this->doAction($action);
} else {
    $this->httpError(403);
}

So it can be done, but not in as non-intrusively as you would expect it to be. :)

[patricknelson]

I more concerned with the fact that there's a conflict in the first place. Is there some reason that this collision is occurring, even though it's not an allowed action? Why is it "a potential action?" Should it not be blocked by default and, if not, why?

I think only once it's an allowed action (hence the requirement that the method exist on the corresponding page/content controller) should it then nag "Sorry this URL segment is in use". That means, not just->Menu() but an unlimited number of non-action related methods (defined by developers on their controllers) would cause collisions. So to me this is potentially a cross contamination of concerns.

Personally, I assume it's not an action at all unless explicitly declared as such via ::$allowed_actions. Once the developer actually explicitly defines menu as an allowed action, it's up to his/her discretion how to move forward (e.g. overriding it so that it returns a rendered view of some sort and handling it properly). Then, naturally, SiteTree::validURLSegment will trigger an error.

[dhensby]

@tractorcow has given a pretty good example of how we can resolve this problem - his suggestion also provides an added benefit that it doesn't implicitly expose method names on controllers

[patricknelson]

Please be more specific @dhensby, are you talking to me? If so my point is that the core functionality is potentially flawed (in some sense) unless I've misunderstood stated what's currently happening. Are you specifically referring to his most recent comment? I ask because he said "This is working as intended" but I'm offering a differing opinion from that viewpoint.

[dhensby]

@patricknelson

are you talking to me?

yes

If so my point is that the core functionality is potentially flawed

I think that we're agreeing with you - but that it is working as intended even though that may not be the most desirable outcome. For something to work as intended it means that the developer had an objective and set of boundries they wanted it to work within and this fulfils them. Yes, in hind-sight, it looks like it would be wiser for the code to not complain about un-authorised actions, but perhaps the dev was priorotising ease of debugging over code flexibility?

Are you specifically referring to his most recent comment?

yes

Overall @tractorcow's example code looks like a good way to go:

$action = $this->findAction($request);
$actions = $this->getAllowedActions();
if (in_array($action, $actions)) {
    $this->doAction($action);
} else {
    // Search for sub-pages and render that instead
}

Basically what we should do is check:

1. if the action exists in allowed actions, if so, call it.
2. if the action exists as a child page url, pass the request on to that page.
3. if neither is true, in non-live environments check if the action is a function and throw a warning.

[patricknelson]

I see what you mean @dhensby thanks for the clarification. I just didn't want to make any assumptions since, while suggested, it wasn't obvious to me.

That being said, is this the wrong place to discuss alternatives, since I think that's where we're trying to go (since we do agree on it not being ideal per se)? So far I think I've seen one suggestion to refactor/rename the methods to avoid conflict, but the other is to remove the potential for conflict in the first place.

That's also partly why I was confused... @tractorcow's code looks like an explanation of the current method for calculating which page to render based on the current segment being processed (recursively, I presume) -- not necessarily offering as a solution to the validation issue which was posed.

[dhensby]

not necessarily offering as a solution to the validation issue which was posed.

good point. I think it's probably better for the url validator to be defensive - but if we resolve the underlying problem maybe we could loosen it...

[patricknelson]

To get a little CS'y on this, while I think applying a bit more robustness here would be helpful in the short term, I think framing it as a "separation of concerns" issue would be better suited for the long term.

I think the end result would be succinctly stated as: Restricting validation to being concerned only with that which it should be concerned with; URL-related validation. The implementation of which may be to simply ensure that validation against actions should be oriented toward actions which are actually actions right now. While it's not entirely separated (i.e. actions still live in the same place as controller data, e.g. $Menu corresponding with ->Menu() method on controller) you at least have made a step in the right direction.

[maxime-rainville]

Duplicate of https://github.com/silverstripe/silverstripe-cms/issues/2308