Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean CC sensible data on Gold subscriptions #4291

Merged
merged 3 commits into from Sep 5, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 22, 2018

PaymentView.submit_form function removes all CC sensible data.

This function is used for GoldView in this repo and also for
DonateView in -ext repo.

Since the behavior is shared across them, it's better to modify the
parent's function and remove the override from the children.

Just to be clear here: we completely ignore this information in our servers. We do not read it or save it anywhere.

@humitos humitos requested a review from a team June 22, 2018 15:31
@humitos humitos requested a review from agjohnson June 25, 2018 12:02
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure which form might use a different id here. Maybe the .com forms?

@@ -123,8 +123,14 @@ function PaymentView (config) {
}

PaymentView.prototype.submit_form = function (card_digits, token) {
this.form.find('#id_card_digits').val(card_digits);
this.form.find('#id_last_4_digits').val(card_digits);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This id form specific and shouldn't be changed unless all the forms match. At least one form is using #id_card_digits as the id, and it appears each of the classes extending the base prototype specify fields to match what is used locally. I suppose before this change is made, we need to know why this is #id_card_digits on the base class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grepped all the repositories and I didn't find any place where this ID is used. I suppose it's an old value.

~/rtfd/code ⌚ 14:59:31
$ rg id_card_digits
readthedocs.org/readthedocs/payments/static-src/payments/js/base.js
126:    this.form.find('#id_card_digits').val(card_digits);

Link to the query https://github.com/search?q=org%3Artfd+id_card_digits&type=Code

It seems we are safe by modifying the ID in the base class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this comes from a form field named card_digits, not explicit html markup.

@humitos humitos requested a review from agjohnson June 25, 2018 18:01
@humitos humitos force-pushed the humitos/stripe/blank-data branch 2 times, most recently from 77e7ee9 to c932ba2 Compare July 16, 2018 21:10
`PaymentView.submit_form` function removes all CC sensible data.

This function is used for `GoldView` in this repo and also for
`DonateView` in -ext repo.

Since the behavior is shared across them, it's better to modify the
parent's function and remove the override from the children.
Use `last_4_card_digits` to avoid confusion and express exactly what
it contains.
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks fine. Once the related PRs are merged, we'll want to do a QA pass here as well.

@agjohnson agjohnson merged commit c32d612 into master Sep 5, 2018
@agjohnson agjohnson deleted the humitos/stripe/blank-data branch September 5, 2018 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants