From b511b88948480def7aec75aa8253b11ffb842ed1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 10 2023 10:05:30 +0000 Subject: [PATCH 1/2] Ensure the url we redirect to are full URLs If they aren't prepend the host_url so they become full URLs. This prevents redirecting to random website using the ?next= URL argument. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 969a3c7..f9f8135 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -17,6 +17,7 @@ import string import time import os import warnings +from six.moves.urllib.parse import urljoin import flask import pygit2 @@ -444,7 +445,9 @@ def auth_login(): # pragma: no cover 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 @@ def auth_logout(): # pragma: no cover 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) diff --git a/pagure/ui/app.py b/pagure/ui/app.py index 7d82909..0e7521b 100644 --- a/pagure/ui/app.py +++ b/pagure/ui/app.py @@ -1664,7 +1664,7 @@ def force_logout(): 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 @@ def force_logout(): 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") diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 298847b..32504b4 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -27,6 +27,7 @@ import pygit2 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 @@ def save_reports(repo, username=None, namespace=None): "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(): diff --git a/pagure/ui/login.py b/pagure/ui/login.py index 91ecad4..4ecc38e 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -96,6 +96,8 @@ def do_login(): 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 diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index c568848..2d660e5 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -25,6 +25,7 @@ import logging import os import re from math import ceil +from six.moves.urllib.parse import urljoin import flask import pygit2 @@ -2781,7 +2782,7 @@ def star_project(repo, star, username=None, namespace=None): 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 @@ def watch_repo(repo, watch, username=None, namespace=None): 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(): diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index db2e900..33ea467 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -474,6 +474,12 @@ class PagureFlaskLogintests(tests.SimplePagureTest): 'Settings', 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 +1074,14 @@ class PagureFlaskLogintests(tests.SimplePagureTest): 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.""" From e42fe70eeac07905bce00c71676cba30c70452b9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 10 2023 10:27:36 +0000 Subject: [PATCH 2/2] Fix unit-tests Looks like something changed in the flask framework and the way it sets the Location header upon redirect. Adjusts these tests to account for this change but keeping them backward compatible. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index 33ea467..5927cb0 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -476,9 +476,7 @@ class PagureFlaskLogintests(tests.SimplePagureTest): 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" - ) + 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}) @@ -1099,7 +1097,14 @@ class PagureFlaskLogintests(tests.SimplePagureTest): # 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): @@ -1141,14 +1146,17 @@ class PagureFlaskLogintests(tests.SimplePagureTest): 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