Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2683793010: Block domain labels made of Cyrillic letters that look alike Latin (Closed)

Created:
1 year, 7 months ago by jungshik at Google
Modified:
1 year, 6 months ago
Reviewers:
lgarron, Peter Kasting, sffc
CC:
markdavis, sffc, emilyschechter
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Block domain labels made of Cyrillic letters that look alike Latin Block a label made entirely of Latin-look-alike Cyrillic letters when the TLD is not an IDN (i.e. this check is ON only for TLDs like 'com', 'net', 'uk', but not applied for IDN TLDs like рф. BUG=683314 TEST=components_unittests --gtest_filter=U*IDN* Review-Url: https://codereview.chromium.org/2683793010 Cr-Commit-Position: refs/heads/master@{#459226} Committed: https://chromium.googlesource.com/chromium/src/+/08cb718ba7c3961c1006176c9faba0a5841ec792

Patch Set 1 #

Patch Set 2 : check Cyrl-Latn alikes only for non-IDN tlds #

Total comments: 11

Patch Set 3 : exclude non-Letter when checking against Cyrl-Latn look-alikes #

Patch Set 4 : Streamline isTLD ASCII check #

Patch Set 5 : Add ӕъЬвҽвԍнкпгмтцѵѡү to the list of Cyr-Lat alike set #

Patch Set 6 : Do not include "ӕ вҽвԍн мтц" in the Cyr-Lat alike list #

Patch Set 7 : Do not include "ҽк ѡү" in Cyr-Lat alike list #

Patch Set 8 : Add back ҽ and ѡ to the Cyr-Lat alike-set #

Patch Set 9 : a bit more tweaking; no change in the blocked set #

Patch Set 10 : add a test with U+00B7 and Latin-alike Cyrillics #

Total comments: 8

Patch Set 11 : address review comments by Peter #

Patch Set 12 : remove unncessary check #

Patch Set 13 : go back to ps11 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+93 lines, -16 lines) Patch
M components/url_formatter/url_formatter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 11 chunks +63 lines, -16 lines 0 comments Download
M components/url_formatter/url_formatter_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +30 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (9 generated)
jungshik at Google
1 year, 7 months ago (2017-02-15 18:47:55 UTC) #3
jungshik at Google
https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc#newcode331 components/url_formatter/url_formatter.cc:331: icu::UnicodeString("[аеорсухьѕіјһмӏтнв]"), status); "м, т, н, в" look like smallcap ...
1 year, 7 months ago (2017-02-15 18:50:21 UTC) #4
sffc
https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc#newcode209 components/url_formatter/url_formatter.cc:209: is_tld_ascii = false; I don't really understand what this ...
1 year, 7 months ago (2017-02-15 19:57:31 UTC) #6
lgarron
https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc#newcode331 components/url_formatter/url_formatter.cc:331: icu::UnicodeString("[аеорсухьѕіјһмӏтнв]"), status); Also consider ԁ, Ӏ, and maybe ѵ, ...
1 year, 7 months ago (2017-02-15 20:24:49 UTC) #8
lgarron
https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc#newcode331 components/url_formatter/url_formatter.cc:331: icu::UnicodeString("[аеорсухьѕіјһмӏтнв]"), status); On 2017/02/15 at 20:24:49, lgarron wrote: > ...
1 year, 7 months ago (2017-02-15 20:25:56 UTC) #9
jungshik at Google
https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/20001/components/url_formatter/url_formatter.cc#newcode209 components/url_formatter/url_formatter.cc:209: is_tld_ascii = false; On 2017/02/15 19:57:30, sffc wrote: > ...
1 year, 7 months ago (2017-02-15 20:55:52 UTC) #10
lgarron
*ping* (Motivated by a bug that uses Turkish, although it's not a perfect spoof like ...
1 year, 6 months ago (2017-03-21 20:45:41 UTC) #11
jungshik at Google
Something is not right here. My reply to comments are not included.
1 year, 6 months ago (2017-03-21 21:51:59 UTC) #12
jungshik at Google
On 2017/03/21 21:51:59, jungshik at Google wrote: > Something is not right here. My reply ...
1 year, 6 months ago (2017-03-21 22:45:45 UTC) #13
jungshik at Google
Can you take a look? I'll not try to fine-tune a Latin-alike Cyrillic set to ...
1 year, 6 months ago (2017-03-22 19:10:59 UTC) #15
Peter Kasting
LGTM https://codereview.chromium.org/2683793010/diff/180001/components/url_formatter/url_formatter.cc File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2683793010/diff/180001/components/url_formatter/url_formatter.cc#newcode253 components/url_formatter/url_formatter.cc:253: // Returns true if |label| is safe to ...
1 year, 6 months ago (2017-03-22 22:14:08 UTC) #16
jungshik at Google
Thanks, Peter for the review. What's the best practice of landing a 'private' CL? I'm ...
1 year, 6 months ago (2017-03-23 20:10:17 UTC) #17
lgarron
On 2017/03/23 at 20:10:17, jshin wrote: > Thanks, Peter for the review. > > What's ...
1 year, 6 months ago (2017-03-23 20:12:12 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2683793010/240001
1 year, 6 months ago (2017-03-23 20:13:56 UTC) #21
jungshik at Google
On 2017/03/23 20:12:12, lgarron wrote: > On 2017/03/23 at 20:10:17, jshin wrote: > > Thanks, ...
1 year, 6 months ago (2017-03-23 20:14:00 UTC) #22
commit-bot: I haz the power
1 year, 6 months ago (2017-03-23 21:26:23 UTC) #25
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/08cb718ba7c3961c1006176c9fab...

Powered by Google App Engine
This is Rietveld 408576698