search

unlcms / project-herbie

Drupal 10 implementation at the University of Nebraska–Lincoln
https://cms.unl.edu
GNU General Public License v2.0
5 stars 6 forks source link

Only allow local site HTML pages to be linked in menu #78

Closed ericras closed 4 years ago

ericras commented 4 years ago
macburgee1 commented 4 years ago

I've got some code close to committing for this issue that uses hook_entity_form_display_alter() and hook_form_alter().

macburgee1 commented 4 years ago

unl_menu only allows nodes to be linked. External links and unrouted URLs are disallowed. Leading slashes are automatically added. If a node doesn't exist, then it'll fail validation.

Let's say that a node with nid = 1 exists and there is no node with nid = 2.

The following should pass:

The following should fail:

ericras commented 4 years ago

This is really cool.

I think the big issue is that it only allows nodes to be linked. I think we want Views, Webforms, Taxonomy pages, etc to be allowed too. Really, anything that is a valid internal path should be allowed.

macburgee1 commented 4 years ago

I think we should implement a whitelist of permissible routes.

Re Webforms, I don't think we should expose them directly to the public. The path will be inconsistent with the content hierarchy. http://example.com/form/schedule-a-tour vs http://example.com/visit/schedule-a-tour.

So far, aside from webforms, we have taxonomy terms and views. Anything else come to mind? We can always add routes in the future.

ericras commented 4 years ago

I would lean to allowing everything and blacklisting something if needed. (That shouldn't really be needed anyway since if a user doesn't have access to something it doesn't show anyway.)

Regarding webforms: People are going to want to put those in the menu. There's the "Webform URL alias" setting - doesn't that allow you to set the form path to "/visit/schedule-a-tour" ?

macburgee1 commented 4 years ago

Per discussion, the unl_menu module has been changed from a whitelist to a blacklist model for internal routes. External URLs and unrouted paths continue to be invalid in all instances.

ericras commented 4 years ago

The error message change in 9345e7b missed a second place a few lines up where 'Only nodes and the front page ("") can be linked in a menu item.' needs to be changed to 'The path em>@uri</em is not allowed'

After that, it can be merged.