Issue 29772555: Issue 6647 - Stop converting domains from punycode to unicode

Issue 29772555: Issue 6647 - Stop converting domains from punycode to unicode (Closed)

Created:
May 6, 2018, 2:42 p.m. by Sebastian Noack

Modified:
May 8, 2018, 4:35 p.m.

Reviewers:
Manish Jethani

CC:
kzar

Visibility:
Public.

Description

Issue 6647 - Stop converting domains from punycode to unicode

Patch Set 1 #

Total comments: 8

Created: May 6, 2018, 2:42 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+38 lines, -558 lines)			Patch
M	.eslintignore	View	1 chunk	+0 lines, -1 line	0 comments	Download
M	lib/csp.js	View	3 chunks	+5 lines, -7 lines	5 comments	Download
M	lib/filterComposer.js	View	2 chunks	+3 lines, -5 lines	0 comments	Download
M	lib/notificationHelper.js	View	2 chunks	+1 line, -2 lines	0 comments	Download
M	lib/options.js	View	2 chunks	+1 line, -2 lines	3 comments	Download
M	lib/popupBlocker.js	View	4 chunks	+5 lines, -6 lines	0 comments	Download
R	lib/punycode.js	View	1 chunk	+0 lines, -462 lines	0 comments	Download
M	lib/requestBlocker.js	View	4 chunks	+12 lines, -16 lines	0 comments	Download
M	lib/url.js	View	3 chunks	+3 lines, -47 lines	0 comments	Download
M	lib/whitelisting.js	View	6 chunks	+8 lines, -10 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

Manish Jethani

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js#newcode37 lib/csp.js:37: let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname, For a URL ...

May 6, 2018, 6:37 p.m. (2018-05-06 18:37:13 UTC) #2

Sebastian Noack

May 6, 2018, 6:47 p.m. (2018-05-06 18:47:40 UTC) #3

Sebastian Noack

I also prepared a development build announcement. Can you please review it as well, thanks: ...

May 7, 2018, 4:35 p.m. (2018-05-07 16:35:37 UTC) #4

Manish Jethani

I'm taking another look at the changes, meanwhile some comment inline. https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js File lib/csp.js (right): ...

May 7, 2018, 5:03 p.m. (2018-05-07 17:03:45 UTC) #5

I'm taking another look at the changes, meanwhile some comment inline.

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js#newcode37
lib/csp.js:37: let cspMatch = defaultMatcher.matchesAny(details.url,
typeMap.CSP, hostname,
On 2018/05/06 18:47:40, Sebastian Noack wrote:
> On 2018/05/06 18:37:13, Manish Jethani wrote:
> > For a URL like https://user:pass@example.com:8080/foo?bar=1#hash details.url
> > will also contains the #hash part. I think we should strip it out to be
> > compatible with stringifyURL.
> 
> IIRC, the webRequest API is agnostic of any hash given in the URL. Please
> correct me if I'm wrong. But otherwise stripping the hash seems redundant.

At least on Chrome 67 (Canary), if you have a document like this:

  <!DOCTYPE html>
  <iframe src="https://example.com/#foo"></iframe>

webRequest.onBeforeRequest will include the "#foo" part in details.url.

For what it's worth, I think the hash part is significant for documents. Modern
documents (post Ajax) will display different content based on the hash part.
They treat it effectively like it's part of the query string, but on the client
side. The hash part is also relevant for Flash (SWF) objects (incidentally I
have a patent related to this).

I'm not sure the hash part is relevant for ad content loaded in embedded frames
though, but if you want to keep it I'm OK with it. It just might break some
filters, that's all.

https://codereview.adblockplus.org/29772555/diff/29772556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29772555/diff/29772556/lib/options.js#newc...
lib/options.js:216: host: currentPage.url.hostname.replace(/^www\./, ""),
On 2018/05/06 18:47:40, Sebastian Noack wrote:
> On 2018/05/06 18:37:13, Manish Jethani wrote:
> > Should we also display the encoded hostname in the UI? Maybe this is a case
> > where it would still be a good idea to display the Unicode version of the
> name.
> 
> Whatever we do here, it will be inconsistent with what the browser shows in
the
> address bar. Firefox and Chrome apply different heuristics to determine
whether
> to render the domain in the URL in the address bar as unicode (or to show the
> raw punycode). Ideally, we would use the same representation as in the address
> bar but that isn't possible. Also note that this is only for the mobile
options
> page in Firefox on Android. Also keep in mind that we would have to keep the
> punycode library included in the repository and the builds just for this.

I remember when I was testing ABP on Firefox for Android it was neat to see the
domain i❤️.ws appear on the options page. I don't even remember if it's
displayed like that in the address bar. Anyway, I'm OK with this change.

Manish Jethani

Looks good modulo my comment about <iframe src="https://example.com/#foo">

May 7, 2018, 5:20 p.m. (2018-05-07 17:20:38 UTC) #6

Sebastian Noack

On 2018/05/07 16:35:37, Sebastian Noack wrote: > I also prepared a development build announcement. Can ...

May 8, 2018, 6:51 a.m. (2018-05-08 06:51:30 UTC) #7

On 2018/05/07 16:35:37, Sebastian Noack wrote:
> I also prepared a development build announcement. Can you please review it as
> well, thanks:
>
https://docs.google.com/document/d/1crgiL4Xxon4HWHLzxuIfKwf6YStPlMFfVoObqBMsd...

Addressed your comments in the draft for the announcement as well.

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js#newcode37
lib/csp.js:37: let cspMatch = defaultMatcher.matchesAny(details.url,
typeMap.CSP, hostname,
On 2018/05/07 17:03:45, Manish Jethani wrote:
> On 2018/05/06 18:47:40, Sebastian Noack wrote:
> > On 2018/05/06 18:37:13, Manish Jethani wrote:
> > > For a URL like https://user:pass@example.com:8080/foo?bar=1#hash
details.url
> > > will also contains the #hash part. I think we should strip it out to be
> > > compatible with stringifyURL.
> > 
> > IIRC, the webRequest API is agnostic of any hash given in the URL. Please
> > correct me if I'm wrong. But otherwise stripping the hash seems redundant.
> 
> At least on Chrome 67 (Canary), if you have a document like this:
> 
>   <!DOCTYPE html>
>   <iframe src="https://example.com/#foo"></iframe>
> 
> webRequest.onBeforeRequest will include the "#foo" part in details.url.

That is interesting.

> For what it's worth, I think the hash part is significant for documents.
Modern
> documents (post Ajax) will display different content based on the hash part.
> They treat it effectively like it's part of the query string, but on the
client
> side. The hash part is also relevant for Flash (SWF) objects (incidentally I
> have a patent related to this).

The only thing is if the hash (unlike the query string) changes (for an
HTML document), we are not going to see a new request in the webRequest API.
Still I am uncertain if its worth to bother. 

> It just might break some filters, that's all.

I don't think that it will break much (if anything). Let's say we have
https://example.com/#foo, so all of following filters will still match
as they did before:

||example.com
||example.com^
||example.com/
||example.com/^

Manish Jethani

May 8, 2018, 12:23 p.m. (2018-05-08 12:23:31 UTC) #8

LGTM

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29772555/diff/29772556/lib/csp.js#newcode37
lib/csp.js:37: let cspMatch = defaultMatcher.matchesAny(details.url,
typeMap.CSP, hostname,
On 2018/05/08 06:51:30, Sebastian Noack wrote:
> On 2018/05/07 17:03:45, Manish Jethani wrote:
> > On 2018/05/06 18:47:40, Sebastian Noack wrote:
> > > On 2018/05/06 18:37:13, Manish Jethani wrote:
> > > > For a URL like https://user:pass@example.com:8080/foo?bar=1#hash
> details.url
> > > > will also contains the #hash part. I think we should strip it out to be
> > > > compatible with stringifyURL.
> > > 
> > > IIRC, the webRequest API is agnostic of any hash given in the URL. Please
> > > correct me if I'm wrong. But otherwise stripping the hash seems redundant.
> > 
> > At least on Chrome 67 (Canary), if you have a document like this:
> > 
> >   <!DOCTYPE html>
> >   <iframe src="https://example.com/#foo"></iframe>
> > 
> > webRequest.onBeforeRequest will include the "#foo" part in details.url.
> 
> That is interesting.
> 
> > For what it's worth, I think the hash part is significant for documents.
> Modern
> > documents (post Ajax) will display different content based on the hash part.
> > They treat it effectively like it's part of the query string, but on the
> client
> > side. The hash part is also relevant for Flash (SWF) objects (incidentally I
> > have a patent related to this).
> 
> The only thing is if the hash (unlike the query string) changes (for an
> HTML document), we are not going to see a new request in the webRequest API.
> Still I am uncertain if its worth to bother. 
> 
> > It just might break some filters, that's all.
> 
> I don't think that it will break much (if anything). Let's say we have
> https://example.com/#foo, so all of following filters will still match
> as they did before:
> 
> ||example.com
> ||example.com^
> ||example.com/
> ||example.com/^

Acknowledged.

Expand All Messages | Collapse All Messages