PR#5355: Ensure the url we redirect to are full URLs - pagure

pagure

#5355 Ensure the url we redirect to are full URLs

Merged 2 years ago by ngompa. Opened 2 years ago by pingou.

fix_redirect into master

Fix unit-tests

Pierre-Yves Chibon • 2 years ago

e42fe70

Ensure the url we redirect to are full URLs

Pierre-Yves Chibon • 2 years ago

b511b88

pagure/flask_app.py

file modified

+7 -2

		`@@ -17,6 +17,7 @@`
		`import time`
		`import os`
		`import warnings`
		`+ from six.moves.urllib.parse import urljoin`

		`import flask`
		`import pygit2`
		`@@ -444,7 +445,9 @@`
		`return_point = flask.url_for("ui_ns.index")`
		`if "next" in flask.request.args:`
		`if pagure.utils.is_safe_url(flask.request.args["next"]):`
		`- return_point = flask.request.args["next"]`
		`+ return_point = urljoin(`
		`+ flask.request.host_url, flask.request.args["next"]`
		`+ )`

		`authenticated = pagure.utils.authenticated()`
		`auth = pagure_config.get("PAGURE_AUTH", None)`
		`@@ -508,7 +511,9 @@`
		`return_point = flask.url_for("ui_ns.index")`
		`if "next" in flask.request.args:`
		`if pagure.utils.is_safe_url(flask.request.args["next"]):`
		`- return_point = flask.request.args["next"]`
		`+ return_point = urljoin(`
		`+ flask.request.host_url, flask.request.args["next"]`
		`+ )`

		`if not pagure.utils.authenticated():`
		`return flask.redirect(return_point)`

pagure/ui/app.py

file modified

+2 -2

		`@@ -1664,7 +1664,7 @@`
		`if admin_session_timedout():`
		`flask.flash("Action canceled, try it again", "error")`
		`return flask.redirect(`
		`- flask.url_for("auth_login", next=flask.request.url)`
		`+ flask.url_for("auth_login", next=flask.request.url, _external=True)`
		`)`

		`# we just need an empty form here to validate that csrf token is present`
		`@@ -1676,7 +1676,7 @@`
		`user.refuse_sessions_before = datetime.datetime.utcnow()`
		`flask.g.session.commit()`
		`flask.flash("All active sessions logged out")`
		`- return flask.redirect(flask.url_for("ui_ns.user_settings"))`
		`+ return flask.redirect(flask.url_for("ui_ns.user_settings", _external=True))`


		`@UI_NS.route("/about")`

pagure/ui/issues.py

file modified

+2 -1

		`@@ -27,6 +27,7 @@`
		`import werkzeug.datastructures`
		`from sqlalchemy.exc import SQLAlchemyError`
		`from binaryornot.helpers import is_binary_string`
		`+ from six.moves.urllib.parse import urljoin`

		`import pagure.doc_utils`
		`import pagure.exceptions`
		`@@ -1655,7 +1656,7 @@`
		`"ui_ns.view_issues", repo=repo, username=username, namespace=namespace`
		`)`
		`if pagure.utils.is_safe_url(flask.request.referrer):`
		`- return_point = flask.request.referrer`
		`+ return_point = urljoin(flask.request.host_url, flask.request.referrer)`

		`form = pagure.forms.AddReportForm()`
		`if not form.validate_on_submit():`

pagure/ui/login.py

file modified

		`@@ -96,6 +96,8 @@`
		`next_url = flask.request.form.get("next_url")`
		`if not next_url or next_url == "None":`
		`next_url = flask.url_for("ui_ns.index")`
		`+ else:`
		`+ next_url = urljoin(flask.request.host_url, next_url)`

		`if form.validate_on_submit():`
		`username = form.username.data`

pagure/ui/repo.py

file modified

+3 -2

		`@@ -25,6 +25,7 @@`
		`import os`
		`import re`
		`from math import ceil`
		`+ from six.moves.urllib.parse import urljoin`

		`import flask`
		`import pygit2`
		`@@ -2781,7 +2782,7 @@`
		`if flask.request.referrer is not None and pagure.utils.is_safe_url(`
		`flask.request.referrer`
		`):`
		`- return_point = flask.request.referrer`
		`+ return_point = urljoin(flask.request.host_url, flask.request.referrer)`

		`form = pagure.forms.ConfirmationForm()`
		`if not form.validate_on_submit():`
		`@@ -2820,7 +2821,7 @@`

		`return_point = flask.url_for("ui_ns.index")`
		`if pagure.utils.is_safe_url(flask.request.referrer):`
		`- return_point = flask.request.referrer`
		`+ return_point = urljoin(flask.request.host_url, flask.request.referrer)`

		`form = pagure.forms.ConfirmationForm()`
		`if not form.validate_on_submit():`

tests/test_pagure_flask_ui_login.py

file modified

+26 -4

		`@@ -474,6 +474,10 @@`
		`'<span class="d-none d-md-inline">Settings</span>', output_text`
		`)`

		`+ output = self.app.get("/login/?next=%2f%2f%09%2fgoogle.fr")`
		`+ self.assertEqual(output.status_code, 302)`
		`+ self.assertEqual(output.location, "http://localhost/google.fr")`
		`+`
		`@patch.dict("pagure.config.config", {"PAGURE_AUTH": "local"})`
		`@patch.dict("pagure.config.config", {"CHECK_SESSION_IP": False})`
		`def test_has_settings(self):`
		`@@ -1068,6 +1072,14 @@`
		`output.get_data(as_text=True),`
		`)`

		`+ user = tests.FakeUser(username="foo")`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get("/logout/?next=%2f%2f%09%2fgoogle.fr")`
		`+ self.assertEqual(output.status_code, 302)`
		`+ self.assertTrue(`
		`+ output.headers["location"] in ("http://localhost/google.fr",)`
		`+ )`
		`+`
		`@patch.dict("pagure.config.config", {"PAGURE_AUTH": "local"})`
		`def test_settings_admin_session_timedout(self):`
		`"""Test the admin_session_timedout with settings endpoint."""`
		`@@ -1085,7 +1097,14 @@`
		`# redirect again for the login page`
		`output = self.app.get("/settings/")`
		`self.assertEqual(output.status_code, 302)`
		`- self.assertIn("http://localhost/login/", output.location)`
		`+ self.assertTrue(`
		`+ output.location`
		`+ in (`
		`+ "http://localhost/login/",`
		`+ "/login/?next=http%3A%2F%2Flocalhost%2Fsettings%2F",`
		`+ "http://localhost/login/?next=http%3A%2F%2Flocalhost%2Fsettings%2F",`
		`+ )`
		`+ )`
		`# session did not expire`
		`user.login_time = datetime.datetime.utcnow() - lifetime + td1`
		`with tests.user_set(self.app.application, user):`
		`@@ -1127,14 +1146,17 @@`
		`data = {"csrf_token": self.get_csrf()}`
		`output = self.app.post("/settings/forcelogout/", data=data)`
		`self.assertEqual(output.status_code, 302)`
		`- self.assertEqual(`
		`- output.headers["Location"], "http://localhost/settings"`
		`+ self.assertTrue(`
		`+ output.headers["Location"]`
		`+ in ("http://localhost/settings", "/settings")`
		`)`

		`# We should now get redirected to index, because our session became`
		`# invalid`
		`output = self.app.get("/settings")`
		`- self.assertEqual(output.headers["Location"], "http://localhost/")`
		`+ self.assertTrue(`
		`+ output.headers["Location"] in ("http://localhost/", "/")`
		`+ )`

		`# After changing the login_time to now, the session should again be`
		`# valid`

no initial comment

ngompa commented 2 years ago

Looks like we have three failures?

13:35:57  FAILED tests/test_pagure_flask_ui_login.py::PagureFlaskLogintests::test_settings_admin_session_timedout
13:35:57  FAILED tests/test_style.py::TestStyle::test_code_with_black - AssertionError:...
13:35:57  FAILED tests/test_style.py::TestStyle::test_code_with_flake8 - AssertionError...

rebased onto 417b6e8d4ab6ebde31236d96e310f4fcbf36c32b

2 years ago

pingou commented 2 years ago

Just fixed one missing import in ui/issues.py

I have another branch (and thus another PR) adding support for isort and making black and flake8 happy

rebased onto 96da476aafda3ee67b4c4ffb9578149113f67459

2 years ago

ngompa commented 2 years ago

Two failures currently:

08:17:44  FAILED tests/test_pagure_flask_ui_login.py::PagureFlaskLogintests::test_settings_admin_session_timedout
08:17:44  FAILED tests/test_style.py::TestStyle::test_code_with_black - AssertionError:...

The black failure is pretty easy to fix at least.

pingou commented 2 years ago

Running black locally shows me nothing:

$ git log -1
commit c0b92441ea78a4666148a514394d28366dfd2376 (HEAD -> fix_redirect, origin/fix_redirect)
Author: Pierre-Yves Chibon <pingou@pingoured.fr>
Date:   Wed Jan 4 14:01:14 2023 +0100

    Make black happy

    Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>
$ black -l 79 pagure/ tests/
All done! ✨ 🍰 ✨
75 files left unchanged.

rebased onto 417b6e8d4ab6ebde31236d96e310f4fcbf36c32b

2 years ago

rebased onto e05c2a23df5a9bf8c71f7ac046e22b8f37301a73

2 years ago

pingou commented 2 years ago

fixed the black error from jenkins' output

ngompa commented 2 years ago

So now all that's left is the one test failure:

03:53:44  __________ PagureFlaskLogintests.test_settings_admin_session_timedout __________
03:53:44  [gw0] linux -- Python 3.9.13 /usr/bin/python3
03:53:44  
03:53:44  self = <tests.test_pagure_flask_ui_login.PagureFlaskLogintests testMethod=test_settings_admin_session_timedout>
03:53:44  
03:53:44      @patch.dict("pagure.config.config", {"PAGURE_AUTH": "local"})
03:53:44      def test_settings_admin_session_timedout(self):
03:53:44          """Test the admin_session_timedout with settings endpoint."""
03:53:44          lifetime = pagure.config.config.get(
03:53:44              "ADMIN_SESSION_LIFETIME", datetime.timedelta(minutes=15)
03:53:44          )
03:53:44          td1 = datetime.timedelta(minutes=1)
03:53:44          # session already expired
03:53:44          user = tests.FakeUser(username="foo")
03:53:44          user.login_time = datetime.datetime.utcnow() - lifetime - td1
03:53:44          with tests.user_set(self.app.application, user):
03:53:44              # not following the redirect because user_set contextmanager
03:53:44              # will run again for the login page and set back the user
03:53:44              # which results in a loop, since admin_session_timedout will
03:53:44              # redirect again for the login page
03:53:44              output = self.app.get("/settings/")
03:53:44              self.assertEqual(output.status_code, 302)
03:53:44  >           self.assertTrue(
03:53:44                  output.location
03:53:44                  in (
03:53:44                      "http://localhost/login/",
03:53:44                      "/login/?next=http%3A%2F%2Flocalhost%2Fsettings%2F",
03:53:44                  )
03:53:44              )
03:53:44  E           AssertionError: False is not true
03:53:44  
03:53:44  tests/test_pagure_flask_ui_login.py:1100: AssertionError

rebased onto b511b88

2 years ago

2 new commits added