Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
🔒️ Fix another open redirect issue
The redirect parameter to the login dialog now gets restricted to local URLs (as detected by unset scheme & netloc) that match a short list of allowed endpoints routes.
  • Loading branch information
foosel committed Aug 4, 2022
1 parent 4a9b185 commit dabdd40
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 4 deletions.
20 changes: 20 additions & 0 deletions src/octoprint/server/util/__init__.py
Expand Up @@ -421,3 +421,23 @@ def require_login(*permissions):
)

return None


def validate_local_redirect(url, allowed_paths):
"""Validates the given local redirect URL against the given allowed paths.
An `url` is valid for a local redirect if it has neither scheme nor netloc defined,
and its path is one of the given allowed paths.
Args:
url (str): URL to validate
allowed_paths (List[str]): List of allowed paths, only paths contained
will be considered valid.
Returns:
bool: Whether the `url` passed validation or not.
"""
from urllib.parse import urlparse

parsed = urlparse(url)
return parsed.scheme == "" and parsed.netloc == "" and parsed.path in allowed_paths
11 changes: 7 additions & 4 deletions src/octoprint/server/views.py
Expand Up @@ -8,7 +8,6 @@
import os
import re
from collections import defaultdict
from urllib.parse import urlparse

from flask import (
Response,
Expand Down Expand Up @@ -39,7 +38,11 @@
preemptiveCache,
userManager,
)
from octoprint.server.util import has_permissions, require_login_with
from octoprint.server.util import (
has_permissions,
require_login_with,
validate_local_redirect,
)
from octoprint.settings import settings
from octoprint.util import sv, to_bytes, to_unicode
from octoprint.util.version import get_python_version_string
Expand Down Expand Up @@ -173,9 +176,9 @@ def login():

default_redirect_url = request.script_root + url_for("index")
redirect_url = request.args.get("redirect", default_redirect_url)
allowed_paths = [url_for("index"), url_for("recovery")]

parsed = urlparse(redirect_url) # check if redirect url is valid
if parsed.scheme != "" or parsed.netloc != "":
if not validate_local_redirect(redirect_url, allowed_paths):
_logger.warning(
f"Got an invalid redirect URL with the login attempt, misconfiguration or attack attempt: {redirect_url}"
)
Expand Down
36 changes: 36 additions & 0 deletions tests/server/util/test_util.py
@@ -0,0 +1,36 @@
__license__ = "GNU Affero General Public License http://www.gnu.org/licenses/agpl.html"
__copyright__ = "Copyright (C) 2022 The OctoPrint Project - Released under terms of the AGPLv3 License"

import pytest


@pytest.mark.parametrize(
"url,paths,expected",
[
# various default UI URLs
("/", ["/", "/recovery/"], True),
("/?", ["/", "/recovery/"], True),
("/?l10n=de", ["/", "/recovery/"], True),
("/?l10n=de&", ["/", "/recovery/"], True),
("/octoprint/", ["/octoprint/", "/octoprint/recovery/"], True),
# various recovery URLs
("/recovery/", ["/", "/recovery/"], True),
("/recovery/?", ["/", "/recovery/"], True),
("/recovery/?l10n=de", ["/", "/recovery/"], True),
("/octoprint/recovery/?l10n=de", ["/octoprint/", "/octoprint/recovery/"], True),
# various external URLs
("http://example.com", ["/", "/recovery/"], False),
("https://example.com", ["/", "/recovery/"], False),
("//example.com", ["/", "/recovery/"], False),
("/\\/\\example.com", ["/", "/recovery/"], False),
(" /\\/\\example.com", ["/", "/recovery/"], False),
("\\/\\/example.com", ["/", "/recovery/"], False),
(" \\/\\/example.com", ["/", "/recovery/"], False),
# other stuff
("javascript:alert(document.cookie)", ["/", "/recovery/"], False),
],
)
def test_validate_local_redirect(url, paths, expected):
from octoprint.server.util import validate_local_redirect

assert validate_local_redirect(url, paths) == expected

1 comment on commit dabdd40

@GitIssueBot
Copy link

Choose a reason for hiding this comment

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

This commit has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/octoprint-goes-to-sleep-after-upgrade-to-1-8-2/46070/14

Please sign in to comment.