r77119 MediaWiki - Code Review archive

Generally speaking, it would be very helpful if there were a little documentation accompanying this. Based on my conversation with Tomasz, my understanding is that this enables a workflow whereby plain HTML forms in files can be dropped into a directory and used inside our infrastructure. It would be reassuring to know what the planned workflow for this is, and what steps are going to be taken to ensure that the people building the forms don't accidentally create exploits. Is it 100% dependent on human review, or are there some technical constraints on the type of HTML they can include?

The token substitution ("@foo") looks like it would make it really easy to unintentionally add an injection vector in the HTML. It would not appear that there's any escaping done on the data, which is pretty worrisome.

Looking through this, here's one thing that immediately jumps out:

+		if ( !is_dir( $html_dir )) {
+			// throw some error
+		}

Probably ought to pick which error to throw here, then actually throw it :)

Another concern is that this would appear to get the file name from $wgRequest. My PHP is somewhat rusty, so I don't know how much pathinfo() can be trusted to strip out all of the nefarious "../"-style tricks. It seems to catch the most obvious ones, but I'd want to be a little more sure than I currently am about this.

RobLa wrote:

That sounds a lot like HTMLets? Is there a difference?

Based on a very cursory investigation, I think one difference may be that this solution allows building forms that can be populated with form data from a failed submission (e.g. when one field is off, the form is presented again, with everything filled in as the user put it in so that they can make the minor correction rather that start over). Does HTMLet lends itself to that use case?

RobLa is correct - afaik, HTMLets does not support form 'stickiness'

Could you not add that to HTMLets then instead of recreating the cycle?

I could, but it would not meet the requirements for this feature ;)

We need a way for the fundraisers to quickly prototype fully functioning forms (in the sense that they function /exactly/ the same as our other credit card forms) without needing to know any PHP and preferably without directly editing any markup on our payment servers.

What Peachey88 said. I don't really understand how a specialized sub-extension is preferable here when a proper extension already exists and could be modified. (Admittedly I haven't been following the background on this, though.)

See my comment in response to Peachey88

Again, this code is intended to meet a fairly singular WMF need: rapid form prototyping (for production use) while preserving access restrictions to the payment cluster.

Commit summaries can be as long as you'd like them to be. ;-)

Nobody is going to complain if your summary is overly verbose and it will save you (and others) time and energy in the long run. :-)

Truth. Thanks for reminding me, it's dangerously easy to get lazy with commit messages when you're just trying to get something out there!

Thanks for the longer explanation below (#c11422). For the stats geeks, here are the top 100 longest commit messages for this repo: http://p.defau.lt/?4acj0Lij2AvmBlqCX5a0iQ

>Generally speaking, it would be very helpful if there were a little documentation accompanying this

In regards to the documentation, yes, I am working on it amidst the other high-priority fundraising items on my plate ATM. The general idea is that Kaldari will use this to try making some forms super quickly - he already knows everything he needs to in order to make use of this as-is.

>The token substitution ("@foo") looks like it would make it really easy to unintentionally add an injection vector in the HTML. It would not appear that there's any escaping done on the data, which is pretty worrisome.

Well, the data gets escaped in the Payflow Pro gateway code on form submission. As far as I can tell, this shouldn't introduce any new injection vector. The user is limited to only a pre-defined list of tokens to use for substitution which are also mapped to the data array that we use to pass escaped form data around internally. Maybe I'm missing something though - can you give me an example of what you had in mind?

> Probably ought to pick which error to throw here, then actually throw it :) Yes - actually doing something in those failed states is a very good idea ;) Before we start using this, I'll get this updated.

> My PHP is somewhat rusty, so I don't know how much pathinfo() can be trusted to strip out all of the nefarious "../"-style tricks. Yeah, you're totally right. pathinfo() is not enough. I'll come up with something safer.

Er sorry the line breaks got wonky on those last two points. Should read:

> Probably ought to pick which error to throw here, then actually throw it :)

Yes - actually doing something in those failed states is a very good idea ;) Before we start using this, I'll get this updated.

> My PHP is somewhat rusty, so I don't know how much pathinfo() can be trusted to strip out all of the nefarious "../"-style tricks.

Yeah, you're totally right. pathinfo() is not enough. I'll come up with something safer.

> In regards to the documentation, yes, I am working on it amidst the other high-priority fundraising items on my plate ATM.

Your comment (#22119) was all that I was really looking for (thanks!). I'd recommend copy/pasting that into README.txt and checking it in with the code and declaring victory for now.

This is vulnerable to XSS, since some of your data array comes straight from $wgRequest and is being spit out into HTML. All of that data array needs escaping before putting it into HTML.

It also relies on a lot of arbitrary naming of elements, eg: $start = strpos( $html, 'name="state"' );, which doesn't make this highly reusable. I also see hardcoded paths for images and to index.php, which also can cause problems when the fundraiser gets reengineered next year ;-)

Also doesn't look very i18n friendly.

(pressed enter too soon)

In general, I don't like this approach of using lots of raw HTML and substituting in the bits we want. Maybe it's personal preference.

I do, however, like Andrew's HTMLForm, and would like to see that become more versatile and thus widely used.

I suspect any solution that involves going all the way back to the drawing board risks not having anything useful for this year's fundraiser. If it's not necessary to start over in order to deploy a secure solution, that guidance would be useful. If, on the other hand, the current approach is beyond repair, that's useful to know as well.

> In general, I don't like this approach of using lots of raw HTML and substituting in the bits we want. Maybe it's personal preference.

Me neither, but again as a prototyping tool, I think it properly meets the fundraiser's needs.

> I do, however, like Andrew's HTMLForm, and would like to see that become more versatile and thus widely used.

We've looked into HTMLForm and liked what we saw, but in the interest of our specific needs and development/integration time, we decided to go this route.

Oh yeah, also we do not want to give edit access to the payment servers' wiki. Keeping this in audited flat-file templated HTML allows us to keep our security in place on the payments cluster.

>This is vulnerable to XSS, since some of your data array comes straight from $wgRequest and is being spit out into HTML. All of that data array needs escaping before putting it into HTML.

The only thing different about the data in this style form generation versus our other one is how we determine what HTML file to pull. The actual array of data gets built outside of this file in payflowpro_gateway.body.php I'm actually surprised this hasn't been brought up in previous code review of payflowpro_gateway.body.php (and that I hadn't previously thought of it...) But the $data array has been being populated in the same way since before I inherited the code. I will dig a little deeper into this, but I do not think this is a problem endemic to this particular code, but to the overall gateway code in general.

>It also relies on a lot of arbitrary naming of elements, eg: $start = strpos( $html, 'name="state"' );, which doesn't make this highly reusable.

Actually, this is not arbitrary. It is how the fields are named in our current donation forms. Since this was built as a way for the fundraisers to rapidly prototype forms in english and they have assured me that those fields will always be dropdowns and always be named the same way, this should not be a problem.

> Also doesn't look very i18n friendly.

Nope, it's absolutely horrid for i18n, but being i18n friendly was not a requirement for this. Again, this is just a prototyping tool for the fundraisers - if they build a form and decide to go with it, we will convert it to be built the same way we build all of the other form variants, making it i18n-friendly

Just to give a little background on this code for those of you who don't know:

The fundraising team asked the tech team for a way to very quickly prototype english-only credit card forms so they can do very rapid iterative live testing to see, with real world usage data, how to improve the forms. This is a very high priority feature request that needs to be completed ASAP.

The current way we develop our credit card forms does /not/ support this - partly for security, partly for i18n and partly because what we have in place now existed long before this ever became a requirement. We thought about using or devising some solution that would allow for easy form creation in wiki markup (eg using the HTMLForm extension), but one of our tech (and incidentally PCI compliance) requirements is to keep access to the payment servers highly restricted. So, rather than completely change how we build forms, allow for others to build forms, process forms, etc. in the middle of the fundraiser, we devised this scheme: a super-lightweight, flat-file html approach with highly restrictive templating tokens/placeholders for reinserting data on form submission failure (form stickiness).

Yes, this means that forms generated in this way will not be localizable in the normal WMF paradigm - but that's ok. If a form is deemed to be 'successful' by the fundraisers, it will be coded the same way we've been building the credit card forms, allowing it to be localized.

Yes, this means that there is the possibility of introducing a security vulnerability by including a flat HTML file. But that's ok too - the fundraisers will pass the flat files to the tech team for security audit/review before being put into production.

Yes, this is fairly one-off, WMF-specific feature. So yes, that means there are some ugly, static things in the code. And yes this means that we ultimately may be reinventing the wheel in some ways - but for our needs and purposes right now, that's ok too.

So, I'm going to clean up some of the potential security issues in the code. But in the mean time, if someone wants to build something that still meets the requirements but does so more elegantly, please do :)

All of the outstanding issues have been resolved as of r77503