New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 868566 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Chrome's Elo card mapping code is incorrect

Project Member Reported by jsaul@google.com, Jul 27

Issue description

The six-digit ELO card mappings that begin with 4 (link 1) are getting superseded by the initial check that all Visa cards begin with 4 (link 2).  It looks like Union Pay's "62" is clobbering the Elo "627780" option as well (link 3).  These become a bigger issue in light of  http://crbug.com/868552 .

Could this be fixed and merged into M69?

[1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=289-290&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0
[2] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=215&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0
[3] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=237&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0
 
Cc: -jsaul@google.com se...@chromium.org
Owner: jsaul@google.com
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/314c21b1f6d6b5c676455969bc6b4446b0fd38b8

commit 314c21b1f6d6b5c676455969bc6b4446b0fd38b8
Author: Jared Saul <jsaul@google.com>
Date: Thu Aug 02 20:30:09 2018

[Autofill] Fix Elo card mapping (Visa's "4" and Union Pay's "62" were bypassing it)

Bug:  868566 
Change-Id: Icf41453cc6b4528463e32338d30eef0c037ce6b4
Reviewed-on: https://chromium-review.googlesource.com/1159738
Commit-Queue: Jared Saul <jsaul@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580301}
[modify] https://crrev.com/314c21b1f6d6b5c676455969bc6b4446b0fd38b8/components/autofill/core/browser/credit_card.cc
[modify] https://crrev.com/314c21b1f6d6b5c676455969bc6b4446b0fd38b8/components/autofill/core/browser/credit_card_unittest.cc

Labels: M-69 Merge-Request-69
Requesting merge to M69.  Chrome was incorrectly identifying Elo cards in 3/6 cases, with additional problems in a fourth case if the card number contained spaces or a hyphen.  This fix is also important in the context of  http://crbug.com/868552 .
Project Member

Comment 4 by sheriffbot@chromium.org, Aug 2

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Less than 29 days to go before AppStore submit on M69
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Before we approve merge to M69, please answer followings:
* Is this M69 regression? Is it critical?
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69?
* Any other important details to justify the merge.
Please note M69 is already in Beta, so merge bar is very high. Thank you.


Could you pls point the launch bug for this? Also is this feature behind finch?
Answering questions in #c5 and #c6:

* It's not specifically an M69 regression.  It's not critical in terms of preventing crashes, but it does cause  Issue 868552 's merge to be much less effective, though, which is preventing a bad user experience.
* I have tested the change in Canary 70.0.3511.0 and it works properly.  There are a high amount of automated tests for this feature (there were previously some gaps in the test coverage which I have remedied with this CL).
* This particular CL was a bugfix that is NOT behind a Finch flag.  My Autofill reviewer determined it to be "simple enough for a merge" though.
* Still discussing with durgapandey@ if launch bug is necessary
Per the new Chrome launch process, 
https://docs.google.com/document/d/1hJ1U8-7DNa7lGfTJWRgSgqQyNnOFO4Ks5Czr1-3--8I/edit#

I'm assuming we don't need a launch bug for this since it's a bug fix. Krishna can you pls confirm?

Cc: durgapandey@chromium.org
Is this fall under "bug fixes or other trivial changes"? If yes, then no launch bug is needed. +durgapandey@.
Yes, it does. Thanks
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #7, #8, #10 and per internal email thread and offline chat with  durgapandey@. This is small bugfix; self-contained but not flag protected, well-tested with new automated tests.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 3

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9f60ddaa46395b1a7c6e25c3b837495d0001de7

commit f9f60ddaa46395b1a7c6e25c3b837495d0001de7
Author: sebsg <sebsg@chromium.org>
Date: Fri Aug 03 21:27:58 2018

Merge-69 [AF] Fix Elo card mapping.

Visa's "4" and Union Pay's "62" were bypassing it

TBR=jsaul@google.com

(cherry picked from commit 314c21b1f6d6b5c676455969bc6b4446b0fd38b8)

Bug:  868566 
Change-Id: Icf41453cc6b4528463e32338d30eef0c037ce6b4
Reviewed-on: https://chromium-review.googlesource.com/1159738
Commit-Queue: Jared Saul <jsaul@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580301}
Reviewed-on: https://chromium-review.googlesource.com/1162761
Cr-Commit-Position: refs/branch-heads/3497@{#396}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/f9f60ddaa46395b1a7c6e25c3b837495d0001de7/components/autofill/core/browser/credit_card.cc
[modify] https://crrev.com/f9f60ddaa46395b1a7c6e25c3b837495d0001de7/components/autofill/core/browser/credit_card_unittest.cc

Status: Fixed (was: Assigned)
Closing bug as this was merged into M69.

Sign in to add a comment