Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)


Expand All @@ -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)


Expand All @@ -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)


Expand Down
57 changes: 57 additions & 0 deletions bin/url_safety.py
Original file line number Diff line number Diff line change
@@ -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("\\", "")
119 changes: 119 additions & 0 deletions tests/test_redirect_safety.py
Original file line number Diff line number Diff line change
@@ -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/<path:rest> 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")
7 changes: 7 additions & 0 deletions views/template_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,13 @@ def workflows_redirect() -> Response:

@blueprint.route("/workflows/<path:rest>")
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)


Expand Down
Loading