Figure out how to handle form data [#1301024]

Stripe works off the premise that sensitive cardholder data never has to hit your server.

Drupal Forms API assumes everything in a form needs to hit your server.

Removing the 'name' attribute from the html tag will prevent sensitive data from being a part of the post data which is the way stripe intends to work. This is easier said than done with Form API. I think this is way doable from the theme layer, but I'm not sure I would go that route for fear of other overrides mistakenly happening.

I think a custom form element that doesn't output the name attribute that is essentially a text field would work well for the cc number and the cv2 fields. Also, doing something similar with expiration dates. That said, what kind of field should the expiration date be? Perhaps this is an option.

A custom form element could be reused easily. I think it provides the cleanest usage and re-usage possibilities.

Comment	File	Size	Author
#10	stripeexample.patch	6.59 KB	vordude
#9	stripe-1024-9.patch	4.14 KB	vordude

Comments

Comment #1

vordude commented 6 October 2011 at 05:31

I may need to double check my form cache, but I had success with code like this: (D7)

$form['cc-number'] = array(
    '#markup' => '<div class="form-item form-type-textfield form-item-data">
      <label for="cc-number">Credit Card Number <span class="form-required" title="This field is required.">*</span></label>
      <input type="text" id="cc-number" value="" size="60" maxlength="128" class="form-text required" />
      </div>',
  );

Which is pretty hacktaculur!

1)The Data is never posted
2)FAPI doesn't care, since it's not a real form element.

Comment #2

Brackish Lake commented 6 October 2011 at 05:28

I agree that a custom form element would be better than doing it at the theming layer. I'll play around with this in our 6.x branch tomorrow and commit to our fork. (I haven't yet done a pull request because we're still getting things organized.)

Comment #3

sirkitree commented 6 October 2011 at 14:17

My original idea is to have a form agnostic api in place where we can hopefully just add something like this:


  $form['#stripe'] = array($cc_field_name, $cv_field_name, $expiry, $amount);

This would be a way for any form to identify itself as want to be a Stripe processed form and tell the attribute what other relevant field names are present. When we process that form, I think *that* is when we should strip out any field names (probably through javascript?) in order to avoid the values being posted to Drupal and staying out of the form_cache.

I don't know if this is doable, just the basis of what I'd like to try in this regard. In this way, it would be easier for those already familiar with FAPI to just create a typical form, and then we worry about how that form is processed. Thoughts?

Comment #4

Brackish Lake commented 6 October 2011 at 17:00

This, to me, is the single most important issue and something we need to get right early on. I agree that simply an element to the form to trigger Stripe would be a nice, elegant approach. I'll commit the latest of our 6.x branch tonight in case people are interested. (I haven't yet felt there's any reason to do a pull request until these types of discussions take some shape.)

Comment #5

sirkitree commented 6 October 2011 at 17:06

After talking a little with eaton (who knows just a ridiculous amount about FAPI) it seems that this won't be quite possible. We can create something like this, but we will also have to somehow add in a preprocessor as well. So, more like two lines instead of one. I'll do some experimenting to see what this will look like exactly.

Comment #6

Brackish Lake commented 6 October 2011 at 17:26

Thanks and thank you eaton. I was hoping for someone who knows a "ridiculous amount about FAPI." Please let us know what you come up with.

Comment #7

vordude commented 6 October 2011 at 20:37

$form += stripe_standard_form_fields();

$form += stripe_form_fields(array('cc', 'exp', 'cv2'));

$form['cc'] = array(
  '#type' => 'secure_textfield', 
  '#title' => t('Credit Card'), 
  '#size' => 16, 
  '#required' => TRUE,

This is kind of what I was thinking. (The first 2 actually just adding stuff like the third to the form.)

Adding a property sounds interesting, but I'm not sure how it could be done.

Comment #8

sirkitree commented 17 October 2011 at 01:20

vordude, I know you gots some code for this now ;) mind posting a patch?

Comment #9

vordude commented 18 October 2011 at 04:20

Status	File	Size
new	stripe-1024-9.patch	4.14 KB

Sure. Here's a patch that implements 2 new form elements stripe_textfield and stripe_select that keeps sensitive post data off your server. The JS that was previously written still doesn't seem to be functional, but the form elements work.

Quite frankly, this could easily be done with a theme function. That said, for paranoid reasons I don't think it should be to keep a themer from mistakenly re-adding a name attribute to the form elements, unknowingly firehosing sensitive post data at site owners.

Comment #10

vordude commented 19 October 2011 at 05:02

Status	File	Size
new	stripeexample.patch	6.59 KB

more code, most of it sloppy, yet functional.

Also forms api strangely knows about my 42424242 cc data. Didn't try to see if this was because it was set as a default value.

Using a type hidden form element will be necessary since the token has to be run server side.

(Also, I redacted my test api key, that you'll have to add in to make this work.) I didn't look to see what the hidden api key will be known as. (since you need 2 of them to run it-

Further stream of consciousness follows:

I added the stripe php library to the module (but didn't add it to the patch) There may be licensing issues, or perhaps it should be an external library that isn't added. I hate external libraries.

Comment #11

rocketeerbkw commented 22 October 2011 at 11:50

Stripe requires that javascript be enabled to work at all. Since we have this requirement, why not remove the name attribute there? This reduces the need for special form elements. Something as naive as adding a stripe-form class to all elements you want to stripe enable would work for now.

Even if this method is not optimal, a backup function in javascript the checks for name attributes should be used. A well meaning dev might try to "fix" invalid form markup on their site.

Comment #12

liorkesos commented 30 October 2011 at 15:36

The patch applies cleanly yet when I do so - I lose the other form fields except of the account.
I understand that this might be the idea. yet I get a "Your card number is invalid" error upon submition.

Comment #13

omerida commented 1 November 2011 at 04:46

per #11 since JS is a dependency, it'd be good to make sure that the input elements used for swipe data load with disabled=disabled attribute , and have javascript remove it to allow users to actually enter data. Are noscript tags needed to tell ppl with JS disabled that they can't do anything?

Comment #14

sirkitree commented 1 November 2011 at 13:28

@rocketeerbkw - part of the problem is not only the $_POST data being sent back, but the form value being cached by formapi. Just removing the name attribute through js might not accomplish the goal of keeping the form data out of the form cache. I haven't verified that though and would love it if you could give it a shot. Perhaps you could open up a separate issue describing the approach and any results you get?

@liorkesos - thank you for the feedback. Any idea where the error might be?

@omerida - That totally sounds like a valid idea. Could you possibly open another issue and perhaps flesh this out a little more with some code?

Thanks for the great feedback from everyone!

Comment #15

letapjar commented 10 November 2011 at 14:40

Thanks for launching this project, I have been meaning to take a crack at implementing stripe on drupal and you folks are already way ahead of me.

@sirkitree - re: form caching - is setting $form[#cache] = FALSE sufficient to avoid the form caching issue? if so, it seems like we just need to make sure that stripe module's setting for form cache is not altered by other modules. I'm a little unclear on whether other modules can change this behavior after the fact - I don't know as much about FAPI as eaton :)

Comment #16

bcn commented 2 January 2012 at 06:23

Maybe I'm missing something, but why use FAPI to define the card number, CVC, and expiration at all? Couldn't those form elements be created with JavaScript on the client side, and subsequently removed (again, with JavaScript) prior to actually submitting the form?

If so, the only element that needs be defined via FAPI (and the only value to hit the server) would be a hidden field used to store the token that's provided by Stipe.

Comment #17

vordude commented 10 January 2012 at 16:24

Assigned:	Unassigned	» vordude
Status:	Active	» Fixed

Because handling form data with the FAPI is the most secure and accepted way of doing it with drupal.

I contacted the stripe folks and asked their opinion on the matter.

They suggested having disabled form items, that are enabled client site while the name attributes are being stripped.

I've checked in code to this end.
http://drupalcode.org/project/stripe.git/commitdiff/dafa9ab7eabfb4dfba51...

Comment #18

bcn commented 10 January 2012 at 21:29

Okay, that makes sense, and the recent code checked in looks good...

Do we still need to verify this point (that the values from name-less elements don't reach the FAPI cache) from above:

@rocketeerbkw - part of the problem is not only the $_POST data being sent back, but the form value being cached by formapi. Just removing the name attribute through js might not accomplish the goal of keeping the form data out of the form cache.

Comment #19

rocketeerbkw commented 10 January 2012 at 22:15

I've verified removing name attribute with javascript keeps the data from being sent. Here is the test I used https://gist.github.com/1591483.

Comment #20

vordude commented 10 January 2012 at 22:24

One more thing to point out if others are testing, the form is currently set up to use default values. Those values will be a part of the form submission regardless of the presence of the name attribute. A true test wouldn't use form default values.

(not a part of the post data but they are a part of drupal's $form_state['values']

Comment #21

24 January 2012 at 22:30

Status:

Fixed

» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Figure out how to handle form data