Update National_Geographic.xml #14211

episkorowski · 2018-01-06T09:55:14Z

#12901
@Bisaloo New PR. Added a barebones list of subdomains and merged National_Geographic.com-problematic.xml into the primary ruleset in order to use the simplified regex.

Ruleset no longer uses www-s, so the website should load now.

Bisaloo · 2018-01-06T11:51:30Z

Great! It is passing tests now. Some things to change:

archive.nationalgeographic.com has mixed content issues. See for example https://archive.nationalgeographic.com/?iid=133596#folio=CV1. So it should not be included in this ruleset. You can add it to the top comment and explain there is active mixed content.
Update the top comment. Right now, it includes a lot of unnecessary info so if you want, you can stick to the format in our contributing guide.
Remove the trailing tab line 108
Remove line 126. donate.nationalgeographic.com is not listed in your targets so we shouldn't try to secure the cookie for this subdomain.
http://media-members.nationalgeographic.com, http://media-mmdb.nationalgeographic.com, http://media-yourshot.nationalgeographic.com all return HTTP 403. In this case, we like having a test url that returns HTTP 200.
For example

	<target host="media-mmdb.nationalgeographic.com" />
		<test url="http://media-mmdb.nationalgeographic.com/static-media/img/favicon.ico" />

press.nationalgeographic.com has mixed content as well but as far as I can tell, it is only fonts and ads so it is okay to keep it.
Rename the file to NationalGeographic.com.xml

episkorowski · 2018-01-07T04:53:19Z

I've implemented most of the changes, but I'm running into some trouble with getting a test url for media-yourshot.nationalgeographic.com. The one you provided works fine for http://media-members.nationalgeographic.com and http://media-mmdb.nationalgeographic.com, but results in a 404 for media-yourshot, which means it fails the testing.

This might just be lack of experience on my part, but I'm not sure how to get a test URL for it either, since the URL by itself results in the 403. Do you have any suggestions? For now I just left it as is.

Bisaloo · 2018-01-07T08:27:17Z

Yes, finding test urls can be tricky.

Usually, you can find them with your search engine. site:media-yourshot.nationalgeographic.com or "media-yourshort.nationalgeographic.com. But it doesn't work in this case.

According to the name, it must be a cdn for yourshort.nationalgeographic.com so I tried visiting this subdomain but still nothing.

A last thing that can sometimes help for cdn is visiting reddit. Still nothing.

If we can't find a working test url, it is safer to remove the target and add it to the top comment under No working URL known.

Bisaloo · 2018-01-07T08:29:52Z

As you have noticed, it is easy to miss the mixed content on archive.nationalgeographic.com. Could you add the URL I provided to the top comment as example? It will greatly help the next person who works on this ruleset.

Bisaloo · 2018-01-07T13:12:48Z

src/chrome/content/rules/National_Geographic.com.xml

+
+	<!--	Tracking cookies:
+					-->
+	<securecookie host="^\.nationalgeographic\.com$" name="^(?:gpv_p\d+|s_\w+|__utm)\w$" />


While we are at it, could you use a capturing group in the regex?

Bisaloo · 2018-01-07T13:15:30Z

src/chrome/content/rules/National_Geographic.com.xml

+-->
+<ruleset name="National Geographic">
+
+	<target host="www.nationalgeographic.com" />


Please also add

<target host="nationalgeographic.com" /> <rule from="^http://nationalgeographic.com/" to="https://www.nationalgeographic.com/" />

with a note in the top comment explaining that ^ times out over https.

episkorowski · 2018-01-07T22:49:55Z

I'm not totally sure if what I did with the regex is correct, but I've implemented the other changes.

EDIT:
Looks like the CI build gave an error I didn't get when I was testing locally:
failure: /opt/utils/../src/chrome/content/rules/National_Geographic.com.xml failed test: The 'from' rule contains unescaped period in regular expression. Try escaping it with a backslash. ERROR: Validation of rulesets failed.

I'm guessing I need an escape character before the period. I'll try it with:
<rule from="^http://nationalgeographic\.com/" to="https://www.nationalgeographic.com/" />

I'm not totally sure what the other error is about, though:
test/selenium/test_navigation.py::TestNavigation::test_httpnowhere_blocks FAILED [ 22%]
Probably messed up the regex.

EDIT2:
Well, looks like it fixed both.

Bisaloo · 2018-01-08T09:12:40Z

src/chrome/content/rules/National_Geographic.com.xml

+
+	Redirect to HTTP:
+
+		- adventureblog


I have Invalid certificate for this one.

Bisaloo · 2018-01-08T09:13:11Z

src/chrome/content/rules/National_Geographic.com.xml

+
+		- adventureblog
+		- channel
+		- help


Invalid certificate for me.

Bisaloo · 2018-01-08T09:13:49Z

src/chrome/content/rules/National_Geographic.com.xml

+		- adventureblog
+		- channel
+		- help
+		- yourshot


works for me over https

Bisaloo · 2018-01-08T09:14:02Z

src/chrome/content/rules/National_Geographic.com.xml

+		- channel
+		- help
+		- yourshot
+		- yourshotblog


invalid certificate

Bisaloo · 2018-01-08T09:15:05Z

src/chrome/content/rules/National_Geographic.com.xml

+
+		- archive (Example: https://archive.nationalgeographic.com/?iid=133596#folio=CV1)
+		- intelligenttravel		
+		- news


I can't find any page with mixed content for news. Do you have an example?

Bisaloo · 2018-01-08T09:15:46Z

src/chrome/content/rules/National_Geographic.com.xml

+		- intelligenttravel		
+		- news
+		- press
+		- video


I can't find any mixed content, do you have an example?

(Note that we only care about active mixed content)

Bisaloo · 2018-01-08T09:16:43Z

I don't have the same results as you for some subdomains listed in the top comment, can you double check?

episkorowski · 2018-01-17T21:26:33Z

Sorry for the delay. I ran through those subdomains again and most of it was consistent with what you said, so I made the changes. Also, I wasn't sure of the difference between active and passive mixed content at first, so I fixed that as well.

I couldn't get channel to work over HTTPS, I was getting the "too many redirects" error in the tests, so I made a note of that in the comment.

Bisaloo · 2018-01-17T21:30:43Z

@episkorowski, thank you and sorry for the hassle. As I said, this is a very tricky ruleset. To be honest, I have been wanting to update it myself for a long time but hadn't found the time yet. I knew it was going to be tedious.

@jeremyn @J0WI, feel free to add some comments but this should be about ready to merge (follow up of #14164).

Bisaloo · 2018-01-17T21:32:03Z

I couldn't get channel to work over HTTPS, I was getting the "too many redirects" error in the tests, so I made a note of that in the comment.

This means that the target redirects to plain http.

episkorowski · 2018-01-18T03:04:26Z

Ah, okay. I'll correct that real quick.

J0WI · 2018-01-18T12:01:59Z

Thanks @Bisaloo for the comments.
@episkorowski please make sure that all previous comments/rules are present in the new ruleset (as long as the host does still exist). E. g. mshop does still exist, but is no longer present in the new ruleset.

episkorowski · 2018-01-22T19:54:36Z

I added the rules I missed. I also added the sections for CDN buckets and the mixed content on news from the old ruleset. I'm not totally sure how relevant the info still is, though.

images doesn't appear to exist anymore, so I removed mentions of it.

I wasn't sure what to do with metric, since the link in the rule (https://nationalgeographic-com.112.2o7.net/) leads to a 404 now.

Bisaloo · 2018-01-22T20:29:35Z

For metric, you are right we don't want to keep the old rule but you can still list the subdomain under Invalid certificate.

J0WI

I'm still missing images and media-channel.

J0WI · 2018-01-23T00:48:16Z

src/chrome/content/rules/National_Geographic.com.xml

+<ruleset name="National Geographic">
+
+	<target host="www.nationalgeographic.com" />
+	<target host="nationalgeographic.com" />


Should be the first target.

episkorowski · 2018-01-23T02:04:16Z

I've added media-channel to the "Redirects to HTTP" section.

I'm not totally sure what to do with images, though. https://images.nationalgeographic.com/ gives me a 502 Error, same as https://www-s.nationalgeographic.com/. Should I make a category for it or just put it into one of the existing ones?

J0WI · 2018-01-23T13:15:48Z

I've added media-channel to the "Redirects to HTTP" section.

There is only channel

I'm not totally sure what to do with images, though.

Since http and https doesn't behave the same way I would add a new category for it.

…id certs sections

episkorowski · 2018-01-23T15:33:44Z

Added a category for images and put media-channel in the "Redirects to HTTP" section.

Reordered targets and subdomains in the Invalid certificate section to meet standards.

J0WI

LGTM, thanks!

Added list of subdomains, simplified regex

a55f6ca

Hainish added the top-10k label Jan 6, 2018

Renamed file, cleaned up top comment

f2b54b3

Bisaloo reviewed Jan 7, 2018

View reviewed changes

Added new rule, modified regex to use capturing group

a321593

Fixed new rule, added escape character

8eb4c59

Bisaloo reviewed Jan 8, 2018

View reviewed changes

src/chrome/content/rules/National_Geographic.com.xml Outdated

- adventureblog

- channel

- help

Copy link

Collaborator

Bisaloo Jan 8, 2018

Choose a reason for hiding this comment

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

Invalid certificate for me.

Bisaloo reviewed Jan 8, 2018

View reviewed changes

src/chrome/content/rules/National_Geographic.com.xml Outdated

- channel

- help

- yourshot

- yourshotblog

Copy link

Collaborator

Bisaloo Jan 8, 2018

Choose a reason for hiding this comment

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

invalid certificate

Bisaloo reviewed Jan 8, 2018

View reviewed changes

Corrected top comment

2fbf67d

Changed channel category in comment

9315381

Added info and rules from old ruleset

38989dd

Added metric to Invalid Certificate section, removed from CDN buckets

206bbba

J0WI reviewed Jan 23, 2018

View reviewed changes

Added 502 error category for images, reordered target hosts and inval…

945a54f

…id certs sections

J0WI closed this Jan 23, 2018

J0WI reopened this Jan 23, 2018

J0WI approved these changes Jan 23, 2018

View reviewed changes

J0WI merged commit 4f9df9d into EFForg:master Jan 23, 2018

episkorowski deleted the nationalgeographic.com branch January 24, 2018 03:23

Bisaloo mentioned this pull request Feb 12, 2018

www.nationalgeographic.com #12901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update National_Geographic.xml #14211

Update National_Geographic.xml #14211

episkorowski commented Jan 6, 2018 •

edited

Loading

Bisaloo commented Jan 6, 2018 •

edited

Loading

episkorowski commented Jan 7, 2018

Bisaloo commented Jan 7, 2018

Bisaloo commented Jan 7, 2018

Bisaloo Jan 7, 2018

Bisaloo Jan 7, 2018

episkorowski commented Jan 7, 2018 •

edited

Loading

Bisaloo Jan 8, 2018

Bisaloo Jan 8, 2018

Bisaloo Jan 8, 2018

Bisaloo Jan 8, 2018

Bisaloo Jan 8, 2018

Bisaloo Jan 8, 2018

Bisaloo commented Jan 8, 2018

episkorowski commented Jan 17, 2018

Bisaloo commented Jan 17, 2018

Bisaloo commented Jan 17, 2018

episkorowski commented Jan 18, 2018

J0WI commented Jan 18, 2018

episkorowski commented Jan 22, 2018

Bisaloo commented Jan 22, 2018

J0WI left a comment

J0WI Jan 23, 2018

episkorowski commented Jan 23, 2018

J0WI commented Jan 23, 2018

episkorowski commented Jan 23, 2018

J0WI left a comment

Update National_Geographic.xml #14211

Update National_Geographic.xml #14211

Conversation

episkorowski commented Jan 6, 2018 • edited Loading

Bisaloo commented Jan 6, 2018 • edited Loading

episkorowski commented Jan 7, 2018

Bisaloo commented Jan 7, 2018

Bisaloo commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

episkorowski commented Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bisaloo commented Jan 8, 2018

episkorowski commented Jan 17, 2018

Bisaloo commented Jan 17, 2018

Bisaloo commented Jan 17, 2018

episkorowski commented Jan 18, 2018

J0WI commented Jan 18, 2018

episkorowski commented Jan 22, 2018

Bisaloo commented Jan 22, 2018

J0WI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

episkorowski commented Jan 23, 2018

J0WI commented Jan 23, 2018

episkorowski commented Jan 23, 2018

J0WI left a comment

Choose a reason for hiding this comment

episkorowski commented Jan 6, 2018 •

edited

Loading

Bisaloo commented Jan 6, 2018 •

edited

Loading

episkorowski commented Jan 7, 2018 •

edited

Loading