New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Urgent: fix redirect loops for meta.*.stackexchange.com #9110

Merged
merged 2 commits into from Mar 17, 2017

Conversation

Projects
None yet
5 participants
@NickCraver
Contributor

NickCraver commented Mar 16, 2017

The meta.*.stackexchange.com domains have moved to *.meta.stackexchange.com to for HTTPS support via a wildcard. This now does the appropriate redirects to the new homes. Child blog.*.stackexchange.com sites (actually only gaming) are unchanged and will not move...they continue to downgrade.

Long-term, we can narrow this entire rule set down to the blogs, all other domains are getting HTTPS support and HSTS headers.

This is an urgent issue since users of HTTPS Everywhere are currently getting redirect loops from us rolling out HTTPS support here. It's also my first time modifying these rulesets, if I'm doing something completely incorrectly please let me know and I'll fix it ASAP, or feel free to do so on your end, whatever fixes redirects for our combined user base faster. Thanks!

cc @bardiharborow @Bisaloo @jeremyn

Nick Craver
Fix redirect loops on stackexchange.com
The meta.*.stackexchange.com domains have moved to *.meta.stackexchange.com to for HTTPS support via wildcards. This now does the appropriate redirects to the new homes. Child blog.*.stackexchange.com sites are unchanged and will not move...they continue to downgrade.

Long-term, we can narrow this entire rule set down to the blogs, all other domains are getting HTTPS support and HSTS headers.
@NickCraver

This comment has been minimized.

Contributor

NickCraver commented Mar 16, 2017

I don't think the failure here is related to my commit, it appears to be from #6494 earlier, which also gave me odd behavior locally. It was a hurdle to running tests. I assumed it was my setup, but it appears to have broken every PR since on the build system as well.

Failure log:

['src/chrome/content/rules/Busybox.net.xml', 'src/chrome/content/rules/busybox.net.xml'] failed case-insensitivity testing.
Rules exist with identical case-insensitive names, which breaks some filesystems.
@Vogel612

This comment has been minimized.

Vogel612 commented Mar 16, 2017

seems this is basically blocked by #9107 regarding the travis-check

@jeremyn jeremyn closed this Mar 17, 2017

@jeremyn jeremyn reopened this Mar 17, 2017

@jeremyn

This comment has been minimized.

Contributor

jeremyn commented Mar 17, 2017

I've merged #9107, so that problem is gone.

@NickCraver I can merge this as-is, but a few comments, mainly for your reference and the reference of anyone else reading this:

I'm a little unhappy with the https://blog.gaming.stackexchange.com downgrade. That subdomain has a specific exclusion already, so I think the point of the downgrade is to change HTTPS to plain HTTP if someone navigates to an explicit HTTPS link. That's not really the purpose of HTTPS Everywhere. But, it's a subset of the wildcard downgrade that already exists in this ruleset, so that's fine with me in this specific situation.

If possible, in this comment:

	<!-- meta.* sites moved to *.meta - we can safely redirect to their new equivalents, which support https -->

add a link to a StackExchange blog post or something saying this redirect is actually safe to do. I believe you, since you're the site administrator, but a reviewer looking at this ruleset a year from now without context might not know what to do with that assertion.

Did you test your changes in the HTTPSEverywhereUserRules/ directory as described here?

Let me know when you're ready and I'll merge this.

@jeremyn jeremyn self-assigned this Mar 17, 2017

@NickCraver

This comment has been minimized.

Contributor

NickCraver commented Mar 17, 2017

@jeremyn All good notes, thanks! I was me trying to strictly maintain the old behavior for blog which really isn't necessary - https:// links to it never could have worked before, and so shouldn't exist. I removed the downgrade and added a meta post link. A larger blog post is in progress, but it'll take me a bit.

I tested using the automated suite, but don't I have/use Firefox on my Macbook which was the easiest place to deal with this repo.

<!-- meta.* sites moved to *.meta - we can safely redirect to their new equivalents, which support https -->
<!-- details: https://meta.stackexchange.com/questions/292058/network-wide-https-its-time -->
<rule from="^https://meta\.([\w-]+)\.stackexchange\.com/" to="https://$1.meta.stackexchange.com/" />
<rule from="^http://www\.stackoverflow\.blog/" to="https://stackoverflow.blog/" />

This comment has been minimized.

@Bisaloo

Bisaloo Mar 17, 2017

Collaborator

I think this rule can be removed. This domain is now properly handled with the trivial rewrite.

This is unrelated to the issue at hand though and it doesn't necessarily has to make it in this particular PR.

This comment has been minimized.

@jeremyn

jeremyn Mar 17, 2017

Contributor

The redirect from meta.* to *.meta doesn't happen in HTTPS though, in fact it can't because they don't have valid certificates for meta.*. So I'm okay with rewriting it here.

This comment has been minimized.

@Bisaloo

Bisaloo Mar 17, 2017

Collaborator

My bad, something went wrong with my comment. I was talking about the rule line 148:

<rule from="^http://www\.stackoverflow\.blog/" to="https://stackoverflow.blog/" />

This comment has been minimized.

@jeremyn

jeremyn Mar 17, 2017

Contributor

That's not handled by the trivial rewrite, because it's sending www to ^.

This comment has been minimized.

@Bisaloo

Bisaloo Mar 17, 2017

Collaborator

Yes, what I meant is that https://www.stackoverflow.blog is valid now. Sure, it's 301 redirected to https://stackoverflow.blog/ but according to the ruleset style guidelines, we should use the trivial rewrite in this case, shouldn't we?

This comment has been minimized.

@jeremyn

jeremyn Mar 17, 2017

Contributor

Oh, I see what you mean. You're right, we shouldn't have this rule. However the whole ruleset needs review and we don't need to remove that in this pull request.

@jeremyn jeremyn merged commit c8498d7 into EFForg:master Mar 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeremyn

This comment has been minimized.

Contributor

jeremyn commented Mar 17, 2017

@NickCraver I've merged the commits.

@Hainish Can we do a release soon so we can get these changes out?

@jeremyn jeremyn removed their assignment Mar 17, 2017

@Hainish

This comment has been minimized.

Member

Hainish commented Mar 17, 2017

@jeremyn I can make a release today. I usually default against Friday releases but I won't be able to make one next week.

@Hainish

This comment has been minimized.

Member

Hainish commented Mar 17, 2017

Release made! I didn't open a release-forthcoming ticket for it, since this was an unexpected release and I cut some of the unnecessary steps out.

@jeremyn

This comment has been minimized.

Contributor

jeremyn commented Mar 17, 2017

Thanks @Hainish.

@NickCraver

This comment has been minimized.

Contributor

NickCraver commented Mar 18, 2017

Thanks everyone, we greatly appreciate getting this dealt with swiftly! Also kudos on the entire project: tests, checks, hooks, etc. here. The repo itself is great for new users completely unfamiliar with the structure (e.g. me) to contribute.

@Hainish

This comment has been minimized.

Member

Hainish commented Mar 18, 2017

@NickCraver thanks for the kind words. You're lucky you jumped in when you did - it was just yesterday that we merged in a comprehensive buildout of our documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment