Remove access check for anonymous users when creating aliases

caroltron, I'm sure this is by design, but it looks like a valid usecase (especially views with arguments).

As the check does the same as the system does when you load a page, the views+arguments is not a special case. If you want to enhance this then you might want to make the error checking optional and enabled by default.

oooh, nice. In that case I'm going to set it to 'by design'. You can always point the menu to absolute urls then change them when you go into production if you need fake menu links.

Subscribing. path_redirect sounds like it would be a problem, unless the menu checker could check for that, too. That gets into messy territory, though. While I could see ways for path_redirect to get around it, I'm worried that we'll run into other cases where there is no easy way around it.

There was talk of having only one valid alias per page, with the rest doing 301 redirects by default - did that ever get in? If so, path redirect would be deprecated in D6 I guess.

I don't recall that getting in, as that would have involved putting the canonical path right on the node. I'm not sure if that would solve the problem here anyway.

I suppose I'd rather Drupal not try to protect me here.

It would certainly do me good if instead of 'path_redirect module' I would get a proper bug report. I have no time to research what that module is, sorry. I can guess: you want to be able aliases here -- sure, add one line which does the sourcing and stores the Drupal path and add another which displays aliased.

What happened with menu mass aliasing? i18n happened. I can't say there is one definite alias now, as that's language dependent. The new path lookup picks the alias in either the current language or if that does not exist then the language independent version. joining against such a construct is not entirely trivial and I am unsure of the performance results. You are free to try, bench etc. If you need assistance, find me at all the usual places.

The path redirect module uses a completely separate table to create, store, and determine redirects.

You should look at hook_init where it selects from its own table that you can see being created in the install file.

Yes, greggles, but if you want to have an item in the Primary Links menu that is a path redirect, the path you enter into the menu item doesn't actually exist. That's the part that I expect to break.

Would a checkbox on the menu settings page be acceptable (given how late in the cycle we are)? I don't see an alternative other than removing the checking all together, because right now this is a regression that would break 3 sites we've built recently. :-)

Why not add a confirm form?

The path 'test' is either invalid or you do not have access to it.
Are you sure that you want to add this menu item?

I think all path checkers on admin interface should be optional.

specific problem: It is not possible to create path alias for "user/register" with authenticated user. (the joke is, you can do it with anonymous user)

@Pasquelle - access to such links is a different issue (an access issue) that we should deal with separately.

Also, at the moment you can enter any path if you enter the full URL, e.g. http://example/com/fake-path

I think this is a "won't fix".

#236915: Cannot create path alias for 'user/login' was duplicate.

And #286683: add alias /register with path -> "invalid or no access" error.

Narrowing the scope of the issue. I would consider any proposal to remove checking the validity of a path a "won't fix", however I think the points raised in http://drupal.org/node/236915 were quite valid that there may not be any reason to check for access to a path as well as its validity when determining whether the current user can make an alias for it.

I think that issue outlined the (minimal code change) needed.

how about an additional permission like.... ignore path validity when creating...

or the following option:

the biggest (or even only) problems are around /register and /user/login, mabye allowing to add aliases for those two to users who have the administer url aliases permissions

Let's narrow down the issue a little more, and get this committed.

There is no security implication for this because (1) you can always check if you have access to a path by going there directly, (2) if you have access to the 'administer url aliases' permission, you can do a lot of messing up anyway (like aliasing /logout to something else, exploiting a XSS vulnerability, etc.), so this permission should only be given to trusted users.

Looks good. Fixes the original problem. This has bugged me too.

Damien's patch solves the problem and causes no regressions in our tests. Committed to CVS HEAD. Thanks!

It should probably go into Drupal 6 so I'm lowering the version number. It is Gabor's call ...

Damien's patch solves the problem and causes no regressions in our tests. Committed to CVS HEAD. Thanks!

It should probably go into Drupal 6 so I'm lowering the version number. It is Gabor's call ...

Revert please. Good patches linger for months and this one sneaks in six hours without anyone even bothering to answer my comments and problems. Despite I was active in the issue I got no word in the "tested by the community" part. It's not like you would need to wait ages to get a comment from me... why the rush??

So: optional yes but rip it out totally? No way.

chx: can you elaborate on the 'no way' part? I'd be happy to rollback the patch if I understand a bit more. Thanks.

Drupal 6 menu system provides a way to consistently access check a path. Whether you go there, get a menu link to the page, administer it -- it's one consistent access check and if that fails you have nothing to do with said path. This is consistent and even rereading the issue I do not see any valid argument for laxing it here. Why again you should have be able to alias a path that you can't access? If it's during development and not existing then this patch does not help you. It only removed an access check...

Pasqualle though has a valid problem here , menu administration has the exception for anonymous users coded for him it just needs to be added here.

I can agree that there is logic to not allowing people to create menu links to pages they can't access (403). However, the inability to create menu links to a page that doesn't exist yet (404) is a problem during development. We frequently build out a site skeleton before actual content has even been written. While you can greek some of it (Lorem Ipsum etc.), that's not the case for, say, panels or views. Creating a dummy page node there only to replace it later with a View is a waste of more time than you'd expect. (I speak from experience.)

In production an error check of that sort is a usability feature. I development, an error check of that sort is a usability bug.

So the patch as committed fixed only the case for 403 not for aliasing 404 pathes. We can get back to 404 once the anon case is fixed which was a) the cause for the patch b) fixing is rather trivial.

Just realized another use case: Put a View at /my/path. Create a menu item that points to /my/path/somearg. The path doesn't exist per se, but is absolutely valid. That makes the 404 check a problem even in production, if you ever touch a View after launch.

#28 works fine for adding user/login as a menu item. Marking RTBC.

the argument for menu_valid_path() function is quite unusual for me. Is there any other function in core where two different things can be accepted as an argument?

@Pasqualle - sadly, yes (see node_load, for example) - but I think we could write a simpler patch without that option (attached).

I agree, #32 is cleaner. Also tested it and it works great too.

why do we use an array argument when only a simple string (path) is necessary? there are only 2 places where this function is called, so I don't see a problem to change it..

but other thing: $form_item['link_title'] value is used inside menu_valid_path() function, so we may need an array argument here. Is it covered when this index (link_title) is not set?

As noted by Pasqualle, there is a E_ALL error in one branch of menu_valid_path().

On another basis, I still don't understand why we are linking path alias creation to the accessibility of the menu item. After all, someone with 'administer url aliases' can today see, delete and alter all URL aliases, regardless of their access to the corresponding menu items. The only thing that's impossible is to create an alias pointing *to* a menu item the user don't have access to. Does that make any sense?

The only thing I want is an unsubscribe option. Noone reads or cares of what I write and I won't repeat myself.

#29->#2
and
#35->#2
?

@chx and all others ;)
I agree with you we need the security check, especially with 403 pages (except the anon cases), but be able for the 404 error to dismiss it when someone has the permission to administer the menu. (and mabye adding some watchdog messages when that menu items get's clicked anyway, by catching the 404 and checking the menu table!)

I've just found this thread thanks to a pointer from gpk on my query http://drupal.org/node/306922.

I've been banging my brains out on this for over a week in whatever time I have spare. I'm astonished. None of this discussion seems to recognise that this change has silently broken upgrade from 5.x to 6. I've been looking for a bug in the upgrade, quietly dropping my menu items to non-Drupal parts of my site. See http://drupal.org/node/302272 for further detail.

It never occurred to me that this could actually be a 'feature' of 6. I was alerted to that by someone, checked, and sure enough that's how 6 behaves!

PLEASE reconsider this. The default behaviour IMO should be for the security check (strange way to describe it, I'd call it a link to non-Drupal content) to ALLOW making the link, at least for the administrator, preferably for all. If you feel it's useful to control these links then the admin should be able to turn off the ability to make the links.

How many people does this have to burn? I'll bet there's a lot more than you know about if I couldn't find this issue in 9 days of searching and neither could the people who tried to answer my various related questions.

I must apologise for the tone of this, I don't want to sound ungrateful for all the wonderful work that has gone into Drupal. But I do feel I have to try to put the point across in such a way that you can empathise a little with my pain and frustration. Especially seeing that it appears to be regarded as a 7.x issue rather than a serious problem in 6 and in upgrade from 5 to 6.

Revert vandalism. Bugs are fixed in the most current development version and then (and only then) backported.

Gee Damien, calling me a vandal is hardly the nicest way to greet my contribution. OK, so this is the first time I've ever had the courage to post something on a core issue thread and I'm ignorant, but I'm certainly not a deliberate vandal.

Neither am I a muppet sitting on his backside waiting for the priests to fix this. I'd be very happy to hear some discussion, some suggestions of how this would best be fixed (in other words not just fixed to suit me, but fixed so that it also addresses the perceived problems that led to these changes in the first place) and pointers to the parts of the code I should look at.

I'm here to help and to play a part in the community.

jwuk, Damien may have been a bit curt, but we have to revert issue statuses very, very often due to people insisting that bugs are being fixed in the 'wrong' place, and it's quite frustrating. Not to mention this issue has already gone a bit wayward a couple of times due to misinformed comments, when the particular fix to the anonymous check is a couple of lines.

If you want to help, either #28 should be tested and marked RTBC, or #32 needs to be re-rolled without the E_ALL error (then tested and marked RTBC).

Thanks Catch, I've learnt a lesson about issue threads. Though if you're being plagued by this often, I guess I won't be the first to suggest the UI needs review, rather than shooting the messengers. Someone sent me http://drupal.org/webchick-wins-best-contributor-open-source-awards and that cartoon is brilliant! :)

I'd like to follow your suggestion and look at #28 and #32, though I'm not sure how much use I'll be. I can't even understand what chx has written in #28, perhaps he should have clarified it rather than posting #36.

I might find it easier to find my way round this stuff if I understood the new menu tables better. Functions are well doc'd in api.drupal.org, thanks to Doxygen, but I'm not getting anywhere with my request http://drupal.org/node/303405 for info on the {menu_links} and {menu_router} tables. My impression is that menu_router is built from menu_links and is only a performance optimisation, it contains no info that isn't in menu_links, but I'm unsure if that's really true.

More generally it would be useful if every table were doc'd somewhere, with both a detailed description of each field and an overview of how the tables are used.

Correct me if I'm wrong, but the patch in #32, using menu_valid_path will result in a call to menu_get_item in most cases anyway (not used on an external path or a path containing '%') and will return a boolean if ($item && $item['access']), which is what the original patch was trying to not check.

Revised patch that just checks if the user has the permission 'administer menus' (since we assume anyone who can edit the menus knows the paths they're working with) and sets an error message instead of a form error.

Tested and confirmed to work on:
user/login
paththatdoesnotexistyet

Path tests passed 100%. Marking as needs review.

#44 reviewed - no problems here, another review please!

As far as I understand the issue, someone with 'administer menus' may still not have access to nodes, and hence their paths, due to node_access. So needs work.

Many thanks, Dave, #44 looks exactly what I need for Drupal 6.

At least, if I were building a new site in 6, it would be, but since it's concerned only with the form I don't see how it would solve the related issue of silently dropping such menu links created in 5.x when upgrading. I'm still trying to figure out the problem in system_update_6021(). But this patch takes me a step forward because I can now build a test example in 6 and see what entries in menu_links/menu_router are needed.

#47 -- perhaps I'm misunderstanding, but this sounds to me like further ambition for functionality within core.

Rereading this thread, there appear to be a variety of use cases being proposed:

• Allow users to create menu items freely (as in 4.6 and 5.x)
• Warn users if they attempt to create 'unacceptable' menu items (but allow them)
• Block users from creating 'unacceptable' menu items

And, on a separate dimension, 'unacceptable' might be

• Links to locations that don't (currently) exist
• Links to Drupal locations that would be forbidden for some classes of user
• Links to non-Drupal locations that are accessible (e.g. I have .htm pages on my site that I permit by having .htaccess files in those directories)
• Links to non-Drupal locations that aren't accessible (I'm uncertain whether those can be distinguished)

Perhaps some site builders really want such fine-grained control (and finer than that could even be proposed, since the menu item creator isn't necessarily the same class of user as the one to whom the menu is displayed).

But I don't see it as the business of core to include such rampant featuritis. If you must, add hooks to allow its implementation externally?

#44 isn't sufficient for me in Drupal 6.4 (the only version I've tried).

Bear in mind that my inexperience may mean I've applied the patch incorrectly, but here's what I find:

I apply the patch.

I go to Administer > Site building > Menus > Navigation > Add item.

I set Path to z/index.htm (this is a legal url on my site, I have a z directory with a .htaccess file in it).

I set menu link title, description, leave other items unchanged and press Save

I get a red warning "The path 'z/index.htm' is either invalid or ...". This is being generated by menu_edit_item_validate() in menu.admin.inc where there is somewhat similar code.

It's interesting to look at this function in api.drupal.org. 7.x looks the same as 6.x (I've not checked line by line). 5 has no such function! 4.6 has the function, but much smaller (and I would argue, better -- I feel it's wrong for core to impose the strict and limited policy that was added for this in 6, but of course that's just my opinion).

There's a third place where this message is generated, system_site_information_settings_validate(), but I've not looked at that. That was also a D6 addition.

I'm not familiar with operator binding precedence in PHP, but if it works the way I imagine, I believe the patch in #44, the first line has a typo, && instead of ||, so it wipes out the error message in all conditions (and would presumably abort if $item were null?). However, wiping out the error message is exactly what I want, so for me this patch is good! :)

Sorry, couldn't resist! Needs someone capable to review it, I meant. But I'd hate to be boring! :)

[update] Actually, I've now restored path.admin.inc to its previous state, and the function path_admin_form_validate() is not generating the error. So for the recipe I give above, this patch isn't relevant.

Thanks guys, I feel I'm getting closer to my move to D6, which I'm really looking forward to.

To make it very, very clear. The focus of this issue is on removing the access check for paths which the 'anonymous' user has access to (i.e. user/login) so that administrators can add links to them via the menu UI. That's it.

All other gripes about menu access check really need to be dealt with in their own issues - if other issues don't exist yet, then it's easy to create them and link from here. That means paths that don't exist, paths (other than anonymous user ones, which are a quirk of the permissions system) which the user doesn't have access to, etc. etc. The more time is spent discussing everything else here, the less chance of fixing this specific issue.

@jwuk: you can use absolute links for paths like that, putting .htm files inside your Drupal folder isn't something core needs to support explicitly, but there's nothing stopping you from doing this at the moment.

@catch:
> @jwuk: you can use absolute links for paths like that, putting .htm files inside your Drupal folder isn't something core needs to support explicitly, but there's nothing stopping you from doing this at the moment.

I don't need to go into why that's unacceptable. If you look around the forums you'll find a ton of people with a variety of reasons.

You clearly want me to go away and stop bothering you. And there was me thinking I was helping. I'm sorry to have wasted your time.

jwuk: no, I want this issue to get fixed and backported to 6.x as quickly as possible. That only happens when issues and associated patches remain in scope. A patch to do pretty what you want already went in earlier in this issue without much review, and subsequent discussion and patching has been attempting to revert it to a point where it can be backported. Trying to address the access checks and validation in this issue is a lost cause, you're welcome to keep doing so, but it's counter-productive (as is taking things personally and flouncing ;) ).

I wasn't able to find another issue for removing validation and access checks, so I've opened one: #308263: Allow privileged users to bypass the validation of menu items

Let me try to put this sorrowful piece on track again. There are two issues here.

One of them is allowing to add a path alias to an anonymous - only accessible path. This can be done by calling menu_valid_path as that sets the necessary global. This was deadended by pointing out that there is a possible code track where link_title is not set and this is an E_ALL but that is only fired if you want to add an alias to something like user/%. This is not supported. We can add a simple option to menu_valid_path to disallow.

Moreover, noone took the effort to read the incredibly long list of functions calling menu_valid_path. How hard it is to read two functions? You will quickly find that one of them is system_site_information_settings_validate which calls menu_valid_path and noone whined so far about a) possible E_ALL issue b) not able to set a frontpage to a path disallowed. So it's truly only about expectations, isn't it?

Action plan for someone else because I am not going to code an issue where patches get committed against my comments:

1. Write a patch which adds an option to menu_valid_path which disallows foo/% . For D7 make TRUE the default. For D6 make FALSE the default to keep BC.
2. Call menu_valid_path as appropriate from path_admin_form_validate
3. Call menu_valid_path as appropriate from system_site_information_settings_validate for D6 in a separate issue.
4. File a separate issue for the ability to add aliases to nonexisting paths

In regards to #50-53, this issue very clearly started against the general problem jwuk describes (even though the original issue was a specific use case during development, the real problem was not being able to create a menu item linked to a non-existent path), and was co-opted by the user access check thread (as pointed out in #15). I will be heading over to #308263: Allow privileged users to bypass the validation of menu items and adding in a patch or two from above to that thread, as I'm having the same issues with this core regression. But it seems a little odd to chastise someone and tell them to do what you should have done yourself (opening a new thread for a separate issue).

Regarding #26, I don't quite understand why a user with administer menus permission would be forbidden from creating a menu item that links to a resource they don't have access to. Just because *I* can't see something, doesn't mean there aren't others who can and I don't have a need to put it in a menu.

It seems the menu system access check is more useful on viewing the menu item, not creating it. If a user doesn't have permission to view a resource, any menu links to that resource shouldn't show up. I'd love an explanation of why this behavior was designed to work this way...

in response to #55: you don't want 404 pages in production, they're ugly.
also, you don't want to let a visitor see what paying customers can see, 403 pages are ugly too!
but that is NOT the issue here, see #308263: Allow privileged users to bypass the validation of menu items for that.

this issue is specifically to remove the access check in such way, you're able to create valid, anonymous-only accessible paths.

for any discussion about bypassing the access checks for privileged users, go to: #308263: Allow privileged users to bypass the validation of menu items

(sorry if i'm too harsh, but i'm trying to keep this issue focused!)

well the poor issue is abandoned and D7 is broken. I am going to fix it as listed above but only if the broken patch that got committed gets reverted. I attached the reverting patch.

No fails in the test results, setting this back.

I've rolled back this patch. Let's see what chx can come up with, and compare the different solutions. Thanks chx.

Blocked by #204077: Allow menu links pointing to dynamic paths , so first clean up the menu_valid_path function and then get back here.

subscribing...

Moving to the appropriate path.module component.

The whole 403 error when a logged in user visits user/login has always seemed completely ridiculous to me and my clients and the users of their websites. Completely unintuitive behaviour from a user (and an admin/developer's) point of view. Can anyone explain the use case/thinking behind this behaviour?

Even more ridiculous is the fact that if I grant anonymous user role permissions that allow an anonymous user to access admin pages and administer menus and path aliases, I can accomplish exactly what I want which is to create a path alias and a menu link to the user/login page and the user/register page - surely this is silly and dangerous?

I would bump this up for a patch to at least address the issue above. From reading this thread, I can't tell if a working patch for this has been provided yet or not? If I'm being silly, please don't hesitate to point it out!

Please do not change the version information.

I got here from #470072: Hide broken menu items, instead of removing them, which was marked as a duplicate of this issue, but after reading the whole thread, I don't see that the problem reported there is addressed here. To summarize, if a page is removed or made inaccessible, the menu item pointing to that page is broken, along with all sub-menus attached to it.

Yesterday morning I decided to replace a panel page with a regular page. This panel page parented a major section in my menu structure. I created the new page, unpublished the panel page, and changed the alias I had been using to point to the new page. This would have worked in D5. But in D6 all of a sudden half of my menu was gone. When I checked the menu_links table, the entries were still there, but inaccessible through the menu editing interface. I tried to recreate the missing menu links and had numerous problems with pages not recognizing where in the menu they were supposed to be. After reading #470072, I realized what had probably caused the error, and I just republished the panel page, edited the menu to point to the new page, and unpublished the panel page again. Now all is working normally.

I spent about a day and a half recovering from D6 refusing to display a menu section because one node in it was unpublished. It seems to me that the menu system should have let me see the menu item, but marked as disabled, or something comparable. And I'm not convinced that converting aliases to nodes is helping me, either. I think that should be optional behavior.

So my question is, will this problem be addressed in this issue, or should #470072 be re-opened as a separate issue?

Edited to add: this bug corrupted the whole branch of the menu so that I had to manually re-integrate menu items between the new menu items I had created after the branch mysteriously disappeared, and the old items under the branch once I was able to recover it. Until I manually re-shuffled all the items, none of the pages were able to recognize which menu they belonged to for the purposes of expanding sub-menus or using Block Menu. Whether the bug is to be addressed in this issue or in the one closed as duplicate, I think it does need to be addressed soon, before someone else loses several days worth of work.

Has this issue been resolved - is creating an alias directly in the database a possible workaround? I currently have 2 different themes running on my site, I want to use an alias so the registration and login pages use the alternative theme which has been effective for other pages.

see http://drupal.org/node/270788 for workaround solution.

Wow guys more than a year passed and noone acted on the battle plan? Here it is again:

1. Write a patch which adds an option to menu_valid_path which disallows foo/% . For D7 make TRUE the default. For D6 make FALSE the default to keep BC.
2. Call menu_valid_path as appropriate from path_admin_form_validate
3. Call menu_valid_path as appropriate from system_site_information_settings_validate for D6 in a separate issue.
4. File a separate issue for the ability to add aliases to nonexisting paths

Why not just for like 1 minute give 'anonymous user' the permissions to 'create url aliases'.

The check is on a path which only anonymous users have access to (user/login) to see if the current user has access, it doesn't matter who has permissions to create aliases.

Executing that plan is not particularly difficult.

Makes more sense with the menu.inc hunk :)

Added doxygen for the new argument and renamed the function to drupal_valid_path and moved to path.inc. Drupal 6 will get a different patch obviously.

This looks good to me, too bad the test bot isn't running. Committed to CVS HEAD.

#76 is the one that could be ported. With path not calling menu_valid_path the extra arg might not be necessary after all, it just does what menu_valid_path would do.

This looks good to me, too bad the test bot isn't running. Committed to CVS HEAD.

I think this issue is now officially a definitive proof of why the testbot is a good thing :)

Also seems like this needs documentation since it was an API change.

This is the correct fix, thanks David.

Committed to CVS HEAD. Thanks.

Back to NW for docs.

#77 broke HEAD:

Exception system.admin.inc 1487 Undefined variable: item
Fail system.test 979 "The path 'kittens' is either invalid or you do not have access to it." found

OH NO! chx lost the kittens :(

The test bot is very depressed about that and isn't in the mood to review anything. Let's fix that by committing #86.

http://qa.drupal.org/pifr/test/33

Oops, there were no kittens to begin with. In any case, we need the test bot back.

Oops. :P Committed #86 to HEAD!

I added documentation for the D7 change at http://drupal.org/update/modules/6/7#drupal_valid_path

subscribe

I tracked this issue down in cvs from bug report #876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus).

drupal_valid_path is using a $form_item variable that is undefined. When chx changed menu_valid_path to drupal_valid_path the parameters changed more than the innards.

Not sure if this should stay open or be declared a duplicate - either way it just produces a notice so not critical.

#876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus) has a better description of the problem, so let's set this status back.

#76: 190867_path_access.patch queued for re-testing.

I came here from #236915: Cannot create path alias for 'user/login'. Is there a useable patch in the comments for that issue?

I'm running Drupal 7.19 and can't add an alias for 'user/login'...

...unassigning chx since he doesn't seem to be working on this (last post on this thread was back in 2009).

HI all ,

I came across this thread as I was trying to add user/login link to a menu. As Drupal is always checking access it kinda fails.
But the reason it actually fails is because this access was already cached before global $menu_admin; so it was always false;
I am not sure if in drupal_valid_path function after the menu_get_item($path) we should add _menu_check_access($item, $item['original_map']); so this way we make sure that we check the access accroding to $menu_admin = TRUE;

I am not sure how to proceed with the dynamic links though.

I would appreciate any thoughts on this.

Kind Regards,
Dobromir

related: #1634916: "My account" used across site although "User account" is shown for Menu link title and Path