Forum OpenACS Development: Flaw/Bug in Forum Message Posting

While looking through some of the code running the new forums package,
I realized that no checks were being made to make sure the html posted
in messages would behave nicely.  Last night I did some tests and sure
enough, there is no kind of html validation going on when someone
posts a new message to a forum.

In short, this means that any user can break the formatting of the
pages viewing messages.  It may also mean that a user could paste
sufficient javascript, read another users cookies, and somehow pass
that info along throught some kind of link or image tag.  I haven't
done sufficient testing to see if the above scenario is possible, but
I do know that javascript can read and write cookies, so it does seems
that such a hack could work.

It surprises me that such a flaw could be overlooked.  A few simple
function calls and a -validate section in the content form element is
all that would be required to close this potential hole.  There are
already two procs designed specifically to prevent dangerous html from
being input by users:

ad_html_security_check

util_close_html_tags

By using these procs to properly check for unwanted html, and closing
all unclosed tags, each message would be contained and would not
affect the display of other posts on the same page.

I'm kind of amazed this wasn't caught earlier. Maybe we should
have a checklist of things to check before a package is anointed
"done"?

Item #1: is all user input run through security filters?

User input is only put through security filters if it is brought into the script through ad_page_contract. The message-post script brings the values in by calling form get_values. This isn't necessarily a problem, since formtemplating allows you to set up -validate blocks of code to make sure the user inputs valid information and then give user-readable feedback about error. Unfortunately, no -validation was set up in the message-post script.

I would rather not see a forced security filtering of everything coming through user feedback through the rp. There are times when allhtml should be allowed (edit-this-page comes to mind, as well as admin pages). The tools already exists to do proper validation, as developers, we just need to be sure to use them.

I have thought for a long time that it would be useful to be able to use the ad_page_contract filters outside of the ad_page_contract block of code. There have been many times that I have needed to bring variables in that I won't known the names of until the proc is used. In these instances I've relied on the trusty ns_queryget, which does no kind of validation or :trim, etc. At one time I tried to see how such a proc could be constructed, but gave up for lack of time. I envision such a proc taking all the arguments of the -query section of ad_page_contract and grabbing the appropriate query variable and setting them up locally. Possibly like:

set varname [lindex $list_of_vars 0]

ad_queryget_vars $varname:trim

I know I would find such a proc very useful.