Controller input strings are URL encoded

judgej

I noticed in Vanilla 2.0.13 (and I assume earlier versions) that strings passed into the controller functions are all URL encoded. Is that intentional? I would have thought all the decoding (which lives in the HTTP layer) would have been done before the data is passed to the controller.

For example, to start a new discussion you can call up page:

projecttalk/messages/add/Fred Bloggs

However the messages/add controller method is given "Fred+Bloggs" as its value, and the page fails to find the user. Why the "+"? Whether the page is called up using "+", " " or "%20", the controller is given "+" instead of spaces. Similar things happen to other characters that would normally be encoded.

So, is this an oversight, or is it really the intended way Vanilla 2 calls up its controllers? I'm hoping it is a new bug, because it seems bizarre to handle controller input like this.

-- Jason

judgej

Line 575 of class.dispatcher.php should (IMO) decode the parameters like this (I have added the urldecode():

$this->_ControllerMethodArgs[] = urldecode($Parts[$i]);

Should I raise this as a bug? I am kind of reluctant just to leave it on the bug tracker because it does not look like that is being looked at seriously.

-- Jason

Linc

A space is not a valid URL character. You're of course welcome to urldecode any arguments you like in your app's controller, but I'm not sure why that'd make a better default.

judgej

This would be a better default just down to the context. The controllers do not deal with HTTP stuff, so they should not have to be dealing with encoding that comes up from that layer. HTTP is dealt with before the controller is run, and the controller should just be given the end-to-end data that was intended for it. If I put a space in my address bar then I intend the controller to get a space. If I give it a '@' then I expect it to get that character and not have to deal with "%40".

It is how software stacks work - the layers at each end of the stack exchange data, and it matters not what encoding is performed at lower levels to get that data there. I don't believe the main controllers should be decoding HTTP strings any more than they should be unwrapping TCP packets to get at their contents.

I'm sure earlier versions of Vanilla did decode the path components when passing them into the controllers, because stuff that used to work is not working for me now, but I can't see exactly when it changed.

-- Jason

Linc

I disagree but I don't feel qualified to go into this one. Maybe @Todd? In any case, that particular bit of code hasn't changed since the first commit: https://github.com/vanillaforums/Garden/blame/master/library/core/class.dispatcher.php

Todd

On the face of it I would say that the path should come into the controller decoded and I'm actually surprised it isn't since php is supposed to handle all of that. This is one of those things that may have unintended consequences elsewhere in the code.

If you try the code with it for a while and don't see anything going wrong we can integrate it in.

judgej

I'll let you know how it goes - got users testing it tomorrow.

Most of the path-based parameters used across the applications and plugins tend to be numeric IDs. Where strings are passed in, they are usually for formatting convenience, but don't actually have a functional use. The "message/add" page is just one of the few examples where it does have a functional use.

So on the whole, it does not look like there will be many - if any - consequences.

One other area that I have had lots of problems with is the image management pages in the profile section. Many of those pages pass around the username as path-based parameters, but never seem to be able to file the users. I am beginning to wonder if this is because of the lack of decoding, since all my usernames have spaces in, but not many other people seem to have this issue, and maybe they don't have spaces in their username. Something for me to explore in the morning anyway.

@Lincoln - I'm curious about what exactly it is that you disagree with?

Linc

I don't see the utility of having kudgy strings in the URL (or encouraging that sort of URL design) and I'm vaguely worried about injecting code, but I haven't looked at that yet.

//edit: And note I'm just answering your question, not debating the point.

judgej

Fair enough, I was just wondering where you were coming from. So you disagree with what I would like to do (send stuff in the path that may need URL encoding) and don't see the value in the application handling that nicely.

Personally I believe the dispatcher *should* handle its input logically and consistently (which is what I am arguing for here), and it is up to developers to decide what data can be passed in a URL. But yes, keeping the URLs clean and simple is something to strive for, but having to store non-encode-required "slugs" for every item that can be accessed by means other than its numeric ID, is a bit of overhead that it would be nice to be able to avoid at times.

judgej

I've raised this as a bug, with a slightly different fix to the one I gave above (since some of the path-based controller parameters do not go through that point in the code, so I've move the fix back a level to where the path is first split up):

https://github.com/vanillaforums/Garden/issues/issue/644