Secure argument passing

Thanks for working on this, chx. It's an important feature. However, this is not 100% what I had in mind. What I had in mind was a simple (optional) "type system". For example:

Then, the first argument would be cast to an integer. Other types could be 'raw' and 'plain' (escaped text). The 'problem' with your version is that arg() can have various return values / types, which is confusing. I think it should always return the value. The comparison shouldn't take place in arg(), IMO.

The problem is that often, people trust the return value of arg(1), which they shouldn't.

neh, the second patch is not good, for example profile uses something close to raw. I will think on it.

But I still like my solution best :)

@chx: can you confirm this can be closed?

The wish for something like this harkens back to the days when I was a Drupal freshie.

We have a new menu property: mask.

mask can be a regular expression as demonstrated -- not for the faint of heart, mind you -- in taxonomy module.

In order to save people from writing regular expressions for the most common cases, we are introducing some shorthands: '+' any character, %d means a number, %c means a word character, %t means a character that can appear in a node type -- lower and upper case letters and the underscore.

What this makes this interesting is that you can also mask the parts beyond the router path. So if you have defined foo/bar then foo/bar/%d means that foo/bar and foo/bar/123 are acceptable paths here, otherwise the path will not be found. foo/bar/%t/%d means that foo/bar/article/12 is good and so is foo/bar/article. If you want to not allow foo/bar/article, never forget that you can supply a regexp yourself. %d+ means any number of numeric arguments so foo/bar/123/1243 etc are also good.

If router path is $path then "$path/+" is what currently Drupal does. Do not let the additional slash fool you: as detailed above, the parts after the router path are always optional.

After an insightful chat with chx on IRC, I'm convinced that we need the extra flexibility of this new 'mask' property.

To clarify, there is three routes hook_menu implementations might take while dealing with this new property:

• Don't bother, if you only have numeric placeholders
• Use the shorthand forms, if you only have simple needs (mixing numeric and text arguments)
• Go the full regexp route, if you have complex urls or if you don't like simple solutions

The shorthand forms ease the definition of matchers for menu parts: you can use '%d', '%c' and '%t' as a menu part and the regexp will be built for you.

Obviously the code needs work (I'm not even convinced it works in its current form), but the architecture looks sane.

Oh, one more thing: I'm not convinced about the usefulness of '%c'. What are the use cases for this? (to be clear: this will prevent any non-word characters (such as . , - etc.) to be used).

Moshe, I am fairly sure that's not the case -- any core function that takes arguments after its router path needs a mask. Contrib... for example, you have og/subscribe/%node and the callback function is function og_subscribe($node, $uid = NULL) the mask is og/subscribe/%d/%d. Thus the $uid is ensured to be a number. Without the mask you can not pass in a $uid. Edit: but even with the mask the $uid is not mandatory. I admit this is not 100% intuitive.

Or you have og/approve/%node/%user/% where the last percent sign is a token. its mask could be og/approve/%d/%d/[a-fA-F0-9] or a simpler, less strict mask could be og/approve/%d/%d/%c. Without the mask you can't pass in hex only numbers.

Note that if you dislike copypaste then X/X/%d/%d/%c would do or I think the code would happily deal with even //%d/%d/%c. Edit: this is a safety belt against typos. We might say that this feature falls in the "we do not babysit broken code" category and remove it.

cwgordon7 argues that using an array would be better than the paths. I tend to agree -- while in the router path we can skip the literal parts and most of the times we can also skip the wildcards as they are numeric anyways.

After discussion with chx in irc, we agreed that the /? was insufficient. We are now using rtrim($path, '/') before checking the path, thus maintaining the old behavior of having node/1, as well as node/1/, as well as node/1///// be valid paths. Whether this behavior should continue is up to community debate.

Note that this is at code needs review because there is no architecture needs review status, there are many known bugs that can only be fixed once the new API is fully adapted throughout all modules, which we don't want to do until we have a solid, supported architecture.

#17 looks all clean, good and shiny, except for that:

First it would be cleaner to have the $suffix addition moved inside the loop (to be consistent with the $prefix). Then, I think the $mask_length is wrong, it should probably be something like:

Finally, because %d is equal to %, why not dropping it?

@chx, ok drop my first and last comment from #18. What about the comment about $mask_length?

just commenting to subscribe for now. chx and I discuessed this a few weeks ago - hopefully it can add some robustness to callback handling.

fixed an edge case, added tests for 'rest', and moved the test file to the correct location

The problem was that the suffix needs to go after the rest, otherwise rest could replace the specified optional arguments. Adjusted the test too.

Patch no longer applies. (menu.test already exists)

Keeping up with HEAD.

I tried to review this but really don't feel qualified. I did notice that it is missing a patch to menu.api.php which described the new mask property.

The last submitted patch failed testing.

The last submitted patch failed testing.

The last submitted patch failed testing.

Aaaaand I managed to upload the same but wrong patch. Damn.

The logic was a bit broken.

I just saw dblog passing without fixing. I fixed a few modules manually but dblog passed here. Let's see what else is breaking. Note that breaking is good because it signals the patch does something useful.

Minor comment - we already use "mask" for a different aspect of the menu system, so that seems like a sub-optimal term for it. Maybe 'match' or 'filter' or 'expect'?

Also, this assumes people will write the regex correctly- can we maybe have an alias for using is_numeric() or have that as a default?

subscribe.

Here is a simple patch for 7.x and 8.x that delivers a 404 if there are extra path arguments.

ok, well that's revealing cruft in comment, forum, and other modules. This fixes some of it.

Oops - put the if in the wrong place before - hopefully this should remove most of the exceptions.

oops missed a {

subscribing

Is this really a Drupal-defining feature? It seems rather a legacy of the old menu system.