Talk:Security for developers/Architecture

Failure to ensure that new or upgraded extensions function properly with other core extensions
It should be required that all upgraded or new extensions that permit the addition of content visible to the public operate with the revision deletion/suppression module, and that any actions related to content creation be included in the logs reported to checkusers - before installation on any non-testing project. This should be a mandatory criterion before installing, even as a test example, on any "real" project; failure to do this has resulted in extremely inappropriate content addition or difficulty for checkusers to identify and block vandals. AFT5 did not have this ability designed in, and required significant re-engineering to fix the problem; after that, a promise was made not to release future extensions on production wikis, even as tests, until the ability to revision delete/suppress and checkuser was demonstrated. Then Flow was released without the ability to checkuser contributions, or to revision delete/suppress. (Incidentally, the reverse is also true - any actions taken to revision delete/suppress any form of content addition needs to show up in the deletion and/or suppression logs.)

I am certain there are other core extensions with which anything new needs to be able to interact appropriately; these are the two I'm most familiar with, so I'm giving this example. Risker (talk) 01:22, 8 May 2014 (UTC)
 * I have copied and pasted this message from the talk page of the performance guidelines. Thank you, Risker! Sharihareswara (WMF) (talk) 15:00, 9 May 2014 (UTC)

A few thoughts/ideas
Chris, thank you so much for working on this. I look forward to working through the implementation/testing/continuing responsibilities sections when they are more fleshed out.

Some questions, thoughts, and ideas:

"What are we trying to protect?" Are there subcategories here? Also, what about: (I would not be surprised if none of those are applicable, but I want to bring them up.) Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)
 * the fact that a particular IP address or username visited/read a page? (the specific way we keep this private is by not logging it?)
 * entire wikis? e.g., officewiki, Board wiki
 * integrity of the user experience? (like, around CSRF stuff or the insertion of ads?)

"assess your feature's security" - can we give an example or 2? Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

In the examples for "Secure design principles" it's fine to include examples *from the past* and say "we used to do this wrong and now we have fixed it." If you're having trouble finding examples of a few of those principles, that's one source. Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

In the "implementation" section I think it's fine to have "redemption narratives" :-) in which a particular bit of code used to do the wrong thing but now we have fixed it to do things properly. One source of examples: you could go through some security fixes from the last 3-4 years. Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

Idea re unit tests
suggested an additional rule (perhaps this should go into the implementation guide and not the overall guidelines):

Don't ever contact third parties in automated testing. When you are writing automated tests, including unit tests, make super sure that your tests are not contacting the "outside world" (other sites, Tool Labs tools, financial services, non-Wikimedia APIs, and so on). Sharihareswara (WMF) (talk) 17:05, 18 June 2014 (UTC)
 *  sumanah: Yeah... so this is one of those things where it might be good to imply that all third party communication should use one function that's easily overrideable.
 *  People run into issues if they don't do that.
 *  sumanah: I guess what I'm getting at is "wrap anything that you will need to override for testing". In this case, cURL functionality. Does that make sense?
 *  Or even... "wrap anything that will make testing tricky."
 *  ...then you can just not go there.
 * (#wikimedia-fundraising, just now)
 * Sharihareswara (WMF) (talk) 17:30, 18 June 2014 (UTC)

Good example of simplicity
I asked for one last week, and we still need a positive example of where we've created a simple design, implementation, or interface whose simplicity guards against future errors or attacks.

HTMLForm: Suggestion: "HTMLForm, while incredibly complex, has a relatively simple interface for security, i.e., built-in CSRF tokens and validation." DonationInterface: suggested an example from Fundraising. The DonationInterface extension "provides fundraising mechanisms for collecting and tracking payments through various payment gateways." Thus the team wrote unit tests that ensure DonationInterface correctly reads from and writes to those gateways. Instead of writing dozens of individual mocks that might fail and thus incur charges, add noise to logs, cause inaccurate financial reports, or worse, the team:
 * subclassed each unit test from a class that managed all interaction with gateways
 * for the tests, overrode the "actually talking to the outside world" functionality and silenced it

Thus it's simpler to add a new gateway and write a new test, and we're less likely to accidentally break security rules with a bad test. Sharihareswara (WMF) (talk) 17:35, 18 June 2014 (UTC)
 * Corrections from Horn:
 * For clarification, we do mock gateway communication. It's not really an "instead". We just started by damaging the default, for increased safety. (...where in this case, the default was actually talking to other systems.)
 * So, as it turns out, the concept is much simpler than the actual code. :/ ... The problem is that php doesn't do multiple inheritance....
 * ...that one file, is sort of ridiculous. Let me see if I can explain: Every 3rd party gets its own "gateway" class on our end, in the actual code. These classes are all descended from a common parent class. For the tests, we have to do one further descendant for each gateway class... ...and each one overrides the same parent communication function.  So, it looks like a totally insane amount of code duplication. Because you can't inherit from multiple classes.
 * But, given the choice between ugly production code and ugly tests, I decided to shovel all the insanity into the test code. Until we think of something better. The relevant file that creates all these descendants for testing, is DonationInterface/tests/includes/test_gateway/test.adapter.php
 * Maybe you could reclassify this as an example of desperation in code.
 * Sharihareswara (WMF) (talk) 18:27, 18 June 2014 (UTC)