Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1272)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by Sebastian Noack
Modified:
1 year, 2 months ago
Reviewers:
Manish Jethani
CC:
kzar
Visibility:
Public.

Description

Issue 6647 - Stop converting domains from punycode to unicode

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set 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
Sebastian Noack
1 year, 2 months ago (2018-05-06 14:43:28 UTC) #1
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 ...
1 year, 2 months ago (2018-05-06 18:37:13 UTC) #2
Sebastian Noack
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:37:13, ...
1 year, 2 months ago (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: ...
1 year, 2 months ago (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): ...
1 year, 2 months ago (2018-05-07 17:03:45 UTC) #5
Manish Jethani
Looks good modulo my comment about <iframe src="https://example.com/#foo">
1 year, 2 months ago (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 ...
1 year, 2 months ago (2018-05-08 06:51:30 UTC) #7
Manish Jethani
1 year, 2 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5