From a186d911c2736b7a76ec31edb4d73d1360553c8f Mon Sep 17 00:00:00 2001 From: ball42 Date: Sat, 11 Jul 2026 09:55:19 -0500 Subject: [PATCH] fix: guard legacy redirect routes against open-redirect (B8) The legacy /webhooks/* and /cron/* compatibility routes interpolate a user-controlled value (name/target_job/target_webhook) straight into a 301 redirect path. A value like //evil.com or /\evil.com makes the result protocol-relative, so the browser resolves it off-site (open redirect / phishing; CodeQL py/url-redirection). Add bin/url_safety.safe_path_segment (no third-party imports, no cycle risk) that strips slashes/backslashes from single-segment names, and apply it to all five app.py interpolated redirects. The template_view /workflows/ catch-all guards against backslash/'//'/scheme tails, falling back to the catalog. Tests cover off-site payloads per route plus the normal-value happy paths. --- app.py | 27 ++++++-- bin/url_safety.py | 57 ++++++++++++++++ tests/test_redirect_safety.py | 119 ++++++++++++++++++++++++++++++++++ views/template_view.py | 7 ++ 4 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 bin/url_safety.py create mode 100644 tests/test_redirect_safety.py diff --git a/app.py b/app.py index 8e08aa2..75da6a7 100644 --- a/app.py +++ b/app.py @@ -47,6 +47,7 @@ from bin import logger from bin.context_processors import inject_common_vars, register_static_cache_bust +from bin.url_safety import safe_path_segment from bin.view_modifiers import response from views.home_view import _strip_trailing_slash, load_home @@ -192,7 +193,9 @@ def _redir_jamf_new(): @app.route("/webhooks/jamf/edit") def _redir_jamf_edit(): - name = request.args.get("name", "") + name = safe_path_segment(request.args.get("name", "")) + if not name: + return redirect("/automations/jamfpro", code=301) return redirect(f"/automations/jamfpro/{name}/edit", code=301) @@ -218,7 +221,9 @@ def _redir_custom_new(): @app.route("/webhooks/custom/edit") def _redir_custom_edit(): - name = request.args.get("name", "") + name = safe_path_segment(request.args.get("name", "")) + if not name: + return redirect("/automations/custom", code=301) return redirect(f"/automations/custom/{name}/edit", code=301) @@ -234,24 +239,34 @@ def _redir_cron_new(): @app.route("/cron/edit") def _redir_cron_edit(): - name = request.args.get("name", "") + name = safe_path_segment(request.args.get("name", "")) + if not name: + return redirect("/automations/cron", code=301) return redirect(f"/automations/cron/{name}/edit", code=301) @app.route("/cron/delete") def _redir_cron_delete(): - name = request.args.get("target_job", "") + name = safe_path_segment(request.args.get("target_job", "")) + if not name: + return redirect("/automations/cron", code=301) return redirect(f"/automations/cron/{name}/delete", code=301) @app.route("/webhooks/delete") def _redir_webhook_delete(): - name = request.args.get("target_webhook", "") + name = safe_path_segment(request.args.get("target_webhook", "")) + if not name: + return redirect("/automations", code=301) # Need to look up the tag to route properly from bin.data_store import get_webhook_by_name webhook = get_webhook_by_name(name) - tag = webhook.get("tag", "custom") if webhook else "custom" + tag = ( + safe_path_segment(webhook.get("tag", "custom")) + if webhook + else "custom" + ) return redirect(f"/automations/{tag}/{name}/delete", code=301) diff --git a/bin/url_safety.py b/bin/url_safety.py new file mode 100644 index 0000000..e977d80 --- /dev/null +++ b/bin/url_safety.py @@ -0,0 +1,57 @@ +# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # +# +# Copyright (c) 2026 Jamf. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# * Neither the name of the Jamf nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY JAMF SOFTWARE, LLC "AS IS" AND ANY +# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL JAMF SOFTWARE, LLC BE LIABLE FOR ANY +# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# +# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # + +"""URL-safety helpers for building local redirect targets (bug B8). + +Several legacy compatibility routes build a redirect ``Location`` by +interpolating a user-controlled value straight into a path string +(``f"/automations/{name}/edit"``). A value such as ``//evil.com`` or +``/\\evil.com`` turns the result into a protocol-relative URL that the +browser resolves off-site (open redirect / phishing; CodeQL +``py/url-redirection``). + +This module has no third-party imports, so it is safe to import from +``app.py`` and any blueprint without creating an import cycle. +""" + + +def safe_path_segment(value: str) -> str: + """Sanitize a user value that will be interpolated as a *single* + path segment in a local redirect target. + + The interpolated value is an automation name and must never contain + a path separator. Stripping every slash and backslash guarantees the + result cannot introduce an extra segment, a ``//`` run, or a + ``/\\`` sequence -- the three ways an interpolated value can make the + final path protocol-relative or otherwise escape its intended slot. + + Returns a bare segment safe to embed in an f-string path. + """ + if not value: + return "" + return value.replace("/", "").replace("\\", "") diff --git a/tests/test_redirect_safety.py b/tests/test_redirect_safety.py new file mode 100644 index 0000000..a18ee04 --- /dev/null +++ b/tests/test_redirect_safety.py @@ -0,0 +1,119 @@ +"""Open-redirect guard tests for legacy redirect routes (bug B8). + +The legacy compatibility routes in ``app.py`` and +``views/template_view.py`` interpolate user-controlled values straight +into a redirect path. A value like ``//evil.com`` or ``/\\evil.com`` +turns the resulting path into a protocol-relative URL, which the +browser resolves off-site (open redirect / phishing, CodeQL +``py/url-redirection``). + +These routes are anonymous-reachable (they are pure 301 shims), so no +authenticated session is required to exercise them. +""" + +import pytest + +# Payloads that, before the fix, escape the intended local path. +OFF_SITE_PAYLOADS = [ + "//evil.com", + "/\\evil.com", + "\\\\evil.com", + "https://evil.com", + "//evil.com/edit", + # All-separator input sanitizes to empty -> must not leave a + # dangling "//" segment in the local path (defense-in-depth). + "////", + "/", +] + + +def _assert_local_redirect(resp, prefix): + """A safe redirect stays on this origin under the given prefix.""" + assert resp.status_code == 301 + loc = resp.headers["Location"] + # Never protocol-relative and never absolute off-site. + assert not loc.startswith("//"), f"protocol-relative Location: {loc!r}" + assert not loc.startswith("http"), f"absolute Location: {loc!r}" + assert "\\" not in loc, f"backslash in Location: {loc!r}" + # No embedded "//" either: an interpolated separator run can be + # re-parsed as protocol-relative by intermediaries. + assert "//" not in loc, f"embedded '//' in Location: {loc!r}" + assert loc.startswith(prefix), f"Location {loc!r} escaped prefix {prefix!r}" + + +# ---- app.py single-segment name routes ---- + +# (path, query-param, resulting prefix) +NAME_ROUTES = [ + ("/webhooks/jamf/edit", "name", "/automations/jamfpro/"), + ("/webhooks/custom/edit", "name", "/automations/custom/"), + ("/cron/edit", "name", "/automations/cron/"), + ("/cron/delete", "target_job", "/automations/cron/"), +] + + +@pytest.mark.parametrize("path,param,prefix", NAME_ROUTES) +@pytest.mark.parametrize("payload", OFF_SITE_PAYLOADS) +def test_name_route_rejects_off_site(client, path, param, prefix, payload): + resp = client.get(path, query_string={param: payload}) + _assert_local_redirect(resp, "/automations") + + +@pytest.mark.parametrize("path,param,prefix", NAME_ROUTES) +def test_name_route_normal_value_redirects(client, path, param, prefix): + resp = client.get(path, query_string={param: "myhook"}) + _assert_local_redirect(resp, prefix) + assert resp.headers["Location"].endswith("myhook/edit") or resp.headers[ + "Location" + ].endswith("myhook/delete") + + +# ---- app.py webhook delete (target_webhook -> tag lookup) ---- + + +@pytest.mark.parametrize("payload", OFF_SITE_PAYLOADS) +def test_webhook_delete_rejects_off_site(client, jawa_env, payload): + resp = client.get( + "/webhooks/delete", query_string={"target_webhook": payload} + ) + _assert_local_redirect(resp, "/automations") + + +def test_webhook_delete_normal_value_redirects(client, jawa_env): + jawa_env.add_webhook({"name": "myhook", "tag": "custom"}) + resp = client.get( + "/webhooks/delete", query_string={"target_webhook": "myhook"} + ) + _assert_local_redirect(resp, "/automations/custom/") + assert resp.headers["Location"].endswith("myhook/delete") + + +# ---- template_view /workflows/ catch-all ---- + + +@pytest.mark.parametrize( + "path", + [ + "/workflows///evil.com", + "/workflows/\\evil.com", + ], +) +def test_workflows_rest_rejects_off_site(client, path): + resp = client.get(path) + # The invariant is simply: this route never emits an OFF-SITE + # redirect. A 301 must point at a safe local /templates path; any + # non-redirect response (e.g. Werkzeug merges "///" and the URL map + # matches a real page -> 200, or a 404) carries no Location and so + # cannot redirect off-origin. + loc = resp.headers.get("Location") + if loc is not None: + assert not loc.startswith("//"), f"protocol-relative: {loc!r}" + assert not loc.startswith("http"), f"absolute off-site: {loc!r}" + assert "\\" not in loc, f"backslash in Location: {loc!r}" + assert loc.startswith("/templates"), f"escaped prefix: {loc!r}" + + +def test_workflows_rest_normal_value_redirects(client): + resp = client.get("/workflows/device-naming/script") + assert resp.status_code == 301 + assert resp.headers["Location"].endswith("/templates/device-naming/script") diff --git a/views/template_view.py b/views/template_view.py index 199f516..71d5d2a 100644 --- a/views/template_view.py +++ b/views/template_view.py @@ -221,6 +221,13 @@ def workflows_redirect() -> Response: @blueprint.route("/workflows/") def workflows_rest_redirect(rest: str) -> Response: + # `rest` is a user-controlled path tail interpolated into the + # redirect target. A value containing a backslash, a "//" run, or a + # scheme colon could make the result protocol-relative / off-site + # (open redirect, bug B8). Anything suspicious falls back to the + # catalog rather than redirecting off-origin. + if "\\" in rest or "//" in rest or ":" in rest: + return redirect(url_for(CATALOG_ENDPOINT), code=301) return redirect(f"/templates/{rest}", code=301)