Possibility of Open Redirect #13

mtb-beta · 2018-08-29T07:10:04Z

オープンリダイレクトの可能性があったので修正しました。取り込みをお願いします。
以下の修正が含まれています。

logout viewのopen redirect脆弱性の修正
login viewのredirectのユニットテストに問題があったので合わせて修正/追加
login viewのユニットテストでdeprecatedなメソッドを使っていたので修正

mtb-beta · 2018-08-29T07:12:35Z

newauth/views.py

@@ -72,7 +73,7 @@ def logout(request, next_page=None, template_name='registration/logged_out.html'
            redirect_to = request.POST.get(redirect_field_name, getattr(settings, 'LOGOUT_REDIRECT_URL', ''))
        else:
            redirect_to = request.GET.get(redirect_field_name, getattr(settings, 'LOGOUT_REDIRECT_URL', ''))
-        if redirect_to:
+        if redirect_to and is_safe_url(redirect_to):


ログアウト時のリダイレクト先が安全かを確認します。（ログアウト時のリダイレクトは、URLクエリで受け取れるため信用できない）

mtb-beta · 2018-08-29T07:13:24Z

tests/test_views.py

@@ -28,7 +30,7 @@ def test_login(self):
            'password': 'password',
        })
        self.assertEquals(response.status_code, 302)
-        self.assert_(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))
+        self.assertTrue(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))


非推奨のメソッドを使っていたので推奨のメソッドに差し替え。

mtb-beta · 2018-08-29T07:20:50Z

tests/test_views.py

@@ -58,41 +60,122 @@ def test_fail_login_blank_fields(self):
    def test_bad_redirect_space(self):
        bad_next_url = 'test space url'
        self.assertContains(self.client.get('/account/login/'), '<form')
-        response = self.client.post('/account/login/?%s=%s' % (REDIRECT_FIELD_NAME, quote(bad_next_url)), {
+        response = self.client.post('/account/login/', {


redirectは、HTTP POSTパラメータで渡さないと読まれず、ただしくテストできてなかったので、テスト修正

https://github.com/beproud/django-newauth/blob/master/newauth/views.py#L53

mtb-beta · 2018-08-29T07:22:35Z

tests/test_views.py

+        self.assertTrue(response['Location'].endswith(ok_url))
+
+@pytest.mark.django_db
+class LogoutViewsTest(DjangoTestCase):


ログアウト時のOpenRedirect用ユニットテストを追加するついでに、通常のlogout viewのテストも追加

aodag · 2018-08-29T07:35:03Z

tests/test_views.py

 import pytest
 from django.utils.six.moves.urllib_parse import quote
 from django.test import TestCase as DjangoTestCase
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.conf import settings
+from django.http import HttpResponse
+from django import shortcuts

 from testapp.models import TestBasicUser


 @pytest.mark.django_db


DjangoTestCase使ってるならこのmarkはいらいないのでは

いらなかったので消しました。

mtb-beta · 2018-08-29T08:23:14Z

どなたかレビューお願いします！

mtb-beta · 2018-08-30T07:44:20Z

tests/test_views.py

+        self.assertEquals(response.status_code, 302)
+        self.assertTrue(response['Location'].endswith('/some/url?param=http://example.com/'))
+
+    def test_ok_redirect(self):


正常系がなかったため、FIXME:のテストの切り分けが難しくなったと思われた。

正常系のテストケースを追加した。

tell-k

軽く見れる範囲でレビューしました

tell-k · 2018-09-05T14:22:24Z

tests/test_views.py

    fixtures = ['authutils_testdata.json']

    def setUp(self):
-        super(ViewsTest, self).setUp()
+        super(LoginViewsTest, self).setUp()


[SHOULD]

Django の TestCase の setUp で superは不要です。

Wrapper around default __call__ method to perform common Django test set up. This means that user-defined Test Cases aren't required to include a call to super().setUp(). refs https://github.com/django/django/blob/master/django/test/testcases.py#L192

tell-k · 2018-09-05T14:22:44Z

tests/test_views.py

@@ -28,7 +30,7 @@ def test_login(self):
            'password': 'password',
        })
        self.assertEquals(response.status_code, 302)
-        self.assert_(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))
+        self.assertTrue(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))


tell-k · 2018-09-05T14:23:30Z

tests/test_views.py

@@ -58,41 +60,122 @@ def test_fail_login_blank_fields(self):
    def test_bad_redirect_space(self):
        bad_next_url = 'test space url'
        self.assertContains(self.client.get('/account/login/'), '<form')
-        response = self.client.post('/account/login/?%s=%s' % (REDIRECT_FIELD_NAME, quote(bad_next_url)), {
+        response = self.client.post('/account/login/', {


tell-k · 2018-09-05T14:31:01Z

tests/test_views.py

        })
        self.assertEquals(response.status_code, 302)
-        self.assert_(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))
+        self.assertTrue(response['Location'].endswith(settings.LOGIN_REDIRECT_URL))


[MAY] LOGIN_REDIRECT_URL を明示的に設定した方が良い？

settings.LOGIN_REDIRECT_URL が具体的にどんな値なのかよくわからない(多分Djangoのデフォルト値？)

よくわからないからテストケースが妥当なのか判別つかない

settings.py に定義 or override_settingsを利用して、LOGIN_REDIRECT_URLがなんなのか明示的にした方が良さそう。

例えば、 settings.LOGIN_REDIRECT_URL="/" だとしたら、bad_next_url とendswith で一致してバグっててもテストが通ってしまいそう。

settings.py に定義 & override_settingsを利用のテストケース追加

tell-k · 2018-09-05T14:31:17Z

tests/test_views.py

+        self.assertEquals(response.status_code, 302)
+        self.assertTrue(response['Location'].endswith('/some/url?param=http://example.com/'))
+
+    def test_ok_redirect(self):


tell-k · 2018-09-05T14:48:16Z

tests/test_views.py

+class LogoutViewsTest(DjangoTestCase):
+    fixtures = ['authutils_testdata.json']
+
+    @mock.patch('newauth.views.render')


[SHOULD] mockは無くてもテストできる

mockが必要な場面になったらmockを使う方が良いかと。

普通に response 受け取ってテストすれば良い。

def test_get_logout_default(self): """ logout with HTTP GET default LOGOUT_render_URL settings """ res = self.client.get('/account/logout/') self.assertEqual(res.status_code, 200)

tell-k · 2018-09-05T14:56:26Z

tests/test_views.py

+        redirect_url = 'http://example.com/path/to/resource/'
+        self.client.get('/account/logout/?%s=%s' % (REDIRECT_FIELD_NAME, quote(redirect_url)))
+        mock_render.assert_called()
+


[MAY] 可能なら next_page 引数がある場合もテストする。

login, logout 両方の view で next_page引数が受け取れるようになってる。

可能であればそれらを受け取る場合の分岐もテストケースにあると良さそう。(現状そのままでは self.cleint ではテストできないから面倒だけど)

[MAY] ついでに logout_then_login のテストがあっても良い。

logout_then_login のテスト

こちらのテストケースが該当すると思います。

https://github.com/beproud/django-newauth/blob/master/tests/test_api.py#L114

next_page 引数がある場合もテスト

こちらすぐにどうやるかわからなかったので、Issueたてて、ひとまず保留したいと思います。

こちらのテストケースが該当すると思います。
https://github.com/beproud/django-newauth/blob/master/tests/test_api.py#L114

私が言ってるのは、views.pyの下の方にひっそりと存在してる logout_then_login というview関数の話です。

https://github.com/beproud/django-newauth/blob/master/newauth/views.py#L85-L87

こちらすぐにどうやるかわからなかったので、Issueたてて、ひとまず保留したいと思います。

良いと思います。テストを書くのであれば以下の方法が考えれます。

案A. overriede_settings で ROOT_URL_CONF を上書きする。

https://stackoverflow.com/a/31838024/7724457

ここの name を override_settings 突っ込むの微妙だから、別ファイルにした方が良さそう。

テスト用のURLパターンに next_page をダイレクトに渡す。

https://docs.djangoproject.com/en/1.11/topics/http/urls/#passing-extra-options-to-view-functions

案B. login, logout を関数としてテストする。

self.clientは使わずにnext_pageが渡された場合の分岐をテストする。

request は RequestFactoryなどで作ってパスする

tell-k

LGTM

mtb-beta added 6 commits August 28, 2018 08:27

Security: safe redirect url for open redirect possibility

9dad6a3

refactor: replace deprecated method to new method

a3140d8

add test case: success logged in redirect

42acb31

fix test case: sent redirect URL by POST parameter

ae278d0

fix test case: unexpected redirect

b0bcf0e

Security: logout view, defend open redirect

31a5f51

mtb-beta changed the title ~~[WIP] オープンリダイレクトの可能性~~ [WIP] Possibility of Open Redirect Aug 29, 2018

mtb-beta commented Aug 29, 2018

View reviewed changes

aodag reviewed Aug 29, 2018

View reviewed changes

delete unnecessary pytest mark, check flake8

4c9d005

mtb-beta changed the title ~~[WIP] Possibility of Open Redirect~~ Possibility of Open Redirect Aug 29, 2018

mtb-beta commented Aug 30, 2018

View reviewed changes

tell-k reviewed Sep 5, 2018

View reviewed changes

refs #13 review. test quality improvement

00bf11c

tell-k approved these changes Sep 6, 2018

View reviewed changes

mtb-beta mentioned this pull request Sep 6, 2018

Add test pattern login/logout view #14

Open

mtb-beta merged commit d605992 into master Sep 6, 2018

mtb-beta deleted the safe_redirect branch September 6, 2018 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possibility of Open Redirect #13

Possibility of Open Redirect #13

mtb-beta commented Aug 29, 2018 •

edited

Loading

mtb-beta Aug 29, 2018

mtb-beta Aug 29, 2018

tell-k Sep 5, 2018

mtb-beta Aug 29, 2018

tell-k Sep 5, 2018

mtb-beta Aug 29, 2018

aodag Aug 29, 2018

mtb-beta Aug 29, 2018

mtb-beta commented Aug 29, 2018

mtb-beta Aug 30, 2018

tell-k Sep 5, 2018

tell-k left a comment

tell-k Sep 5, 2018

tell-k Sep 5, 2018

tell-k Sep 5, 2018

tell-k Sep 5, 2018

mtb-beta Sep 6, 2018

tell-k Sep 5, 2018

tell-k Sep 5, 2018

tell-k Sep 5, 2018

tell-k Sep 5, 2018

mtb-beta Sep 6, 2018

mtb-beta Sep 6, 2018

tell-k Sep 6, 2018

tell-k Sep 6, 2018 •

edited

Loading

mtb-beta Sep 6, 2018 •

edited

Loading

tell-k left a comment

Possibility of Open Redirect #13

Possibility of Open Redirect #13

Conversation

mtb-beta commented Aug 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtb-beta commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tell-k left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tell-k Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

mtb-beta Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

tell-k left a comment

Choose a reason for hiding this comment

mtb-beta commented Aug 29, 2018 •

edited

Loading

tell-k Sep 6, 2018 •

edited

Loading

mtb-beta Sep 6, 2018 •

edited

Loading