Skip to content
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

Remove logic for guessing slug from an unregistered domain #5143

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 10 additions & 40 deletions readthedocs/core/middleware.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,14 @@
# -*- coding: utf-8 -*-

"""Middleware for core app."""

import logging

from django.conf import settings
from django.contrib.sessions.middleware import SessionMiddleware
from django.core.cache import cache
from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
from django.http import Http404, HttpResponseBadRequest
from django.urls.base import set_urlconf
from django.utils.deprecation import MiddlewareMixin
from django.utils.translation import ugettext_lazy as _
from django.shortcuts import render

from readthedocs.core.utils import cname_to_slug
from readthedocs.projects.models import Domain, Project


Expand Down Expand Up @@ -112,46 +107,21 @@ def process_request(self, request):
)
# Try header first, then DNS
elif not hasattr(request, 'domain_object'):
try:
slug = cache.get(host)
if not slug:
slug = cname_to_slug(host)
cache.set(host, slug, 60 * 60)
# Cache the slug -> host mapping permanently.
log.info(
LOG_TEMPLATE.format(
msg='CNAME cached: {}->{}'.format(slug, host),
**log_kwargs
),
)
request.slug = slug
request.urlconf = SUBDOMAIN_URLCONF
log.warning(
LOG_TEMPLATE.format(
msg='CNAME detected: %s' % request.slug,
**log_kwargs
),
)
except: # noqa
# Some crazy person is CNAMEing to us. 404.
log.warning(
LOG_TEMPLATE.format(msg='CNAME 404', **log_kwargs),
)
raise Http404(_('Invalid hostname'))
# Some person is CNAMEing to us without configuring a domain - 404.
log.warning(LOG_TEMPLATE.format(msg='CNAME 404', **log_kwargs))
return render(request, 'core/dns-404.html', context={'host': host}, status=404)
# Google was finding crazy www.blah.readthedocs.org domains.
# Block these explicitly after trying CNAME logic.
if len(domain_parts) > 3 and not settings.DEBUG:
# Stop www.fooo.readthedocs.org
if domain_parts[0] == 'www':
log.debug(
LOG_TEMPLATE.format(msg='404ing long domain', **log_kwargs),
)
log.debug(LOG_TEMPLATE.format(
msg='404ing long domain', **log_kwargs
))
return HttpResponseBadRequest(_('Invalid hostname'))
log.debug(
LOG_TEMPLATE
.format(msg='Allowing long domain name', **log_kwargs),
)
# raise Http404(_('Invalid hostname'))
log.debug(LOG_TEMPLATE.format(
msg='Allowing long domain name', **log_kwargs
))
# Normal request.
return None

Expand Down
43 changes: 43 additions & 0 deletions readthedocs/core/templates/core/dns-404.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{% extends "base.html" %}
{% load core_tags %}
{% load i18n %}

{% block title %}
{% trans "Maze Found - Invalid Host" %}
{% endblock %}

{% block header-wrapper %}
{% include "error_header.html" %}
{% endblock %}

{% block notify %}{% endblock %}

{# Hide the language select form so we don't set a CSRF cookie #}
{% block language-select-form %}{% endblock %}


{% block content %}
<h1>{% trans '404 - Invalid Host' %}</h1>
<h3>{% blocktrans %}The host "{{ host }}" is unknown to Read the Docs{% endblocktrans %}</h3>

<p>
{% with docsurl='https://docs.readthedocs.io/en/stable/custom_domains.html' %}
{% blocktrans trimmed %}
If you control this domain and believe this is in error,
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't be is an error?

Suggested change
If you control this domain and believe this is in error,
If you control this domain and believe this is an error,

Copy link
Contributor

Choose a reason for hiding this comment

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

Either works but I think yours is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, I guess is used more in other areas?

please review our <a href="{{ docsurl }}">custom domain documentation</a>.
In the past, we allowed custom domains to point to us without configuring the domain in the Read the Docs dashboard
and we attempted to intelligently guess the correct project based on DNS settings.
Now, we believe that explicit is better than implicit.
Below are some steps to help you get your domain working again:
{% endblocktrans %}
{% endwith %}
</p>


<ul style="margin-left: 2em; list-style: inherit">
<li>{% trans 'Ensure you have a CNAME record pointing to <code>readthedocs.io</code>' %}</li>
<li>{% trans 'Add your desired domain in the Read the Docs dashboard for your project (under Your Project &gt;&gt; Admin &gt;&gt; Domains)' %}</li>
</ul>


{% endblock %}
9 changes: 0 additions & 9 deletions readthedocs/core/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,6 @@ def broadcast(type, task, args, kwargs=None, callback=None): # pylint: disable=
return task_promise


def cname_to_slug(host):
# TODO: remove
from dns import resolver
answer = [ans for ans in resolver.query(host, 'CNAME')][0]
domain = answer.target.to_unicode()
slug = domain.split('.')[0]
return slug


def prepare_build(
project,
version=None,
Expand Down
69 changes: 41 additions & 28 deletions readthedocs/rtd_tests/tests/test_middleware.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
# -*- coding: utf-8 -*-

from corsheaders.middleware import CorsMiddleware
from django.conf import settings
from django.core.cache import cache
from django.http import Http404
from django.test import TestCase
from django.test.client import RequestFactory
from django.test.utils import override_settings
from django.urls.base import get_urlconf, set_urlconf
from django_dynamic_fixture import get
from mock import patch

from readthedocs.core.middleware import SubdomainMiddleware
from readthedocs.projects.models import Domain, Project, ProjectRelationship
Expand All @@ -26,12 +22,18 @@ def setUp(self):
self.middleware = SubdomainMiddleware()
self.url = '/'
self.owner = create_user(username='owner', password='test')
self.pip = get(Project, slug='pip', users=[self.owner], privacy_level='public')
self.pip = get(
Project,
slug='pip',
users=[self.owner],
privacy_level='public'
)

def test_failey_cname(self):
self.assertFalse(Domain.objects.filter(domain='my.host.com').exists())
request = self.factory.get(self.url, HTTP_HOST='my.host.com')
with self.assertRaises(Http404):
self.middleware.process_request(request)
r = self.middleware.process_request(request)
self.assertEqual(r.status_code, 404)
self.assertEqual(request.cname, True)

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
Expand Down Expand Up @@ -74,7 +76,9 @@ def test_restore_urlconf_after_request(self):

@override_settings(PRODUCTION_DOMAIN='prod.readthedocs.org')
def test_subdomain_different_length(self):
request = self.factory.get(self.url, HTTP_HOST='pip.prod.readthedocs.org')
request = self.factory.get(
self.url, HTTP_HOST='pip.prod.readthedocs.org'
)
self.middleware.process_request(request)
self.assertEqual(request.urlconf, self.urlconf_subdomain)
self.assertEqual(request.subdomain, True)
Expand All @@ -92,19 +96,13 @@ def test_domain_object(self):
def test_domain_object_missing(self):
self.domain = get(Domain, domain='docs.foobar2.com', project=self.pip)
request = self.factory.get(self.url, HTTP_HOST='docs.foobar.com')
with self.assertRaises(Http404):
self.middleware.process_request(request)

def test_proper_cname(self):
cache.get = lambda x: 'my_slug'
request = self.factory.get(self.url, HTTP_HOST='my.valid.homename')
self.middleware.process_request(request)
self.assertEqual(request.urlconf, self.urlconf_subdomain)
self.assertEqual(request.cname, True)
self.assertEqual(request.slug, 'my_slug')
r = self.middleware.process_request(request)
self.assertEqual(r.status_code, 404)

def test_request_header(self):
request = self.factory.get(self.url, HTTP_HOST='some.random.com', HTTP_X_RTD_SLUG='pip')
request = self.factory.get(
self.url, HTTP_HOST='some.random.com', HTTP_X_RTD_SLUG='pip'
)
self.middleware.process_request(request)
self.assertEqual(request.urlconf, self.urlconf_subdomain)
self.assertEqual(request.cname, True)
Expand All @@ -113,28 +111,43 @@ def test_request_header(self):

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
def test_proper_cname_uppercase(self):
cache.get = lambda x: x.split('.')[0]
get(Domain, project=self.pip, domain='pip.random.com')
request = self.factory.get(self.url, HTTP_HOST='PIP.RANDOM.COM')
self.middleware.process_request(request)
self.assertEqual(request.urlconf, self.urlconf_subdomain)
self.assertEqual(request.cname, True)
self.assertEqual(request.slug, 'pip')

def test_request_header_uppercase(self):
request = self.factory.get(self.url, HTTP_HOST='some.random.com', HTTP_X_RTD_SLUG='PIP')
request = self.factory.get(
self.url, HTTP_HOST='some.random.com', HTTP_X_RTD_SLUG='PIP'
)
self.middleware.process_request(request)
self.assertEqual(request.urlconf, self.urlconf_subdomain)
self.assertEqual(request.cname, True)
self.assertEqual(request.rtdheader, True)
self.assertEqual(request.slug, 'pip')

@override_settings(USE_SUBDOMAIN=True)
# no need to do a real dns query so patch cname_to_slug
@patch('readthedocs.core.middleware.cname_to_slug', new=lambda x: 'doesnt')
def test_use_subdomain_on(self):
request = self.factory.get(self.url, HTTP_HOST='doesnt.really.matter')
ret_val = self.middleware.process_request(request)
self.assertIsNone(ret_val, None)
def test_use_subdomain(self):
domain = 'doesnt.exists.org'
get(Domain, project=self.pip, domain=domain)
request = self.factory.get(self.url, HTTP_HOST=domain)
res = self.middleware.process_request(request)
self.assertIsNone(res)
self.assertEqual(request.slug, 'pip')
self.assertTrue(request.domain_object)

def test_long_bad_subdomain(self):
domain = 'www.pip.readthedocs.org'
request = self.factory.get(self.url, HTTP_HOST=domain)
res = self.middleware.process_request(request)
self.assertEqual(res.status_code, 400)

def test_long_subdomain(self):
domain = 'some.long.readthedocs.org'
request = self.factory.get(self.url, HTTP_HOST=domain)
res = self.middleware.process_request(request)
self.assertIsNone(res)


class TestCORSMiddleware(TestCase):
Expand Down
2 changes: 0 additions & 2 deletions requirements/pip.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ celery==4.1.1

django-allauth==0.38.0

dnspython==1.16.0

# VCS
httplib2==0.12.0

Expand Down