Skip to content

Use placeholder identifiers in the URL validation tests #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
igor-ctrl merged 1 commit into from
Aug 4, 2026
Merged

Use placeholder identifiers in the URL validation tests #29

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
101 changes: 50 additions & 51 deletions tests/test_url/test_record_key_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@
``build_url`` validated the custom-API route segments (publisher/group/version)
but spliced ``entity_set_name`` and ``record_id`` in raw. A record key is
attacker-influenced in any layer that accepts one from a caller, and a raw ``/``
in it starts a new path segment — so the composed URL could address an entity
in it starts a new path segment, so the composed URL could address an entity
other than the one the registry was consulted about:

entity_set_name="engineOverviews"
record_id="1)/../../../../../../glEntries('X'"
→ .../engineOverviews(1)/../../../../../../glEntries('X')
entity_set_name="widgets"
record_id="1)/../../../../../../ledgerEntries('X'"
→ .../widgets(1)/../../../../../../ledgerEntries('X')

which collapses to ``glEntries('X')``. The registry only ever saw
``engineOverviews``, so its decision was made about the wrong target. Business
Central still applies the caller's own permission set, so this is not a way to
read data the user could not otherwise reach — but it does defeat the
client-side registry restriction, and ``bcli/client/_async.py``'s bound-action
resolver documents an assumption ("everything from the ``(`` onward is opaque to
the registry ... gated on the parent, which is the security-relevant identity")
that only holds once keys cannot contain path syntax.
which collapses to ``ledgerEntries('X')``. The registry only ever saw
``widgets``, so its decision was made about the wrong target. The server still
applies whatever permissions the caller has, so this is not a way to read data
the user could not otherwise reach — but it does defeat the client-side registry
restriction, and ``bcli/client/_async.py``'s bound-action resolver documents an
assumption ("everything from the ``(`` onward is opaque to the registry ... gated
on the parent, which is the security-relevant identity") that only holds once
keys cannot contain path syntax.

The fix rejects raw path and query delimiters. Everything else a real OData key
needs — quotes, commas, equals signs, hyphens, parentheses inside a quoted
Expand All @@ -30,8 +30,8 @@

from bcli._url import build_url

ENV = "SBEnvJun26"
COMPANY = "f99bd320-b400-4189-b3c1-c62c05d4e7a5"
ENV = "Sandbox"
COMPANY = "abc-123"


def _build(**kw) -> str:
Expand All @@ -42,15 +42,15 @@ class TestRecordIdRejectsPathSyntax:
def test_rejects_the_reported_traversal_payload(self):
with pytest.raises(ValueError, match="record_id"):
_build(
entity_set_name="engineOverviews",
record_id="1)/../../../../../../glEntries('X'",
entity_set_name="widgets",
record_id="1)/../../../../../../ledgerEntries('X'",
)

@pytest.mark.parametrize(
"bad",
[
"1/2",
"1)/glEntries('X'",
"1)/ledgerEntries('X'",
"..",
"../customers",
"1\\2",
Expand All @@ -60,34 +60,34 @@ def test_rejects_the_reported_traversal_payload(self):
)
def test_rejects_path_and_query_delimiters(self, bad: str):
with pytest.raises(ValueError, match="record_id"):
_build(entity_set_name="engineOverviews", record_id=bad)
_build(entity_set_name="widgets", record_id=bad)

def test_error_names_percent_encoding_as_the_remedy(self):
"""A caller with a genuine '/' in a key needs to know what to do."""
with pytest.raises(ValueError, match="percent-encode"):
_build(entity_set_name="engineOverviews", record_id="AB/CD")
_build(entity_set_name="widgets", record_id="AB/CD")


class TestRecordIdStillAcceptsRealKeys:
@pytest.mark.parametrize(
"good",
[
"f99bd320-b400-4189-b3c1-c62c05d4e7a5", # GUID
"00000000-0000-0000-0000-000000000001", # GUID
"42", # integer key
"'V00010'", # quoted string
"'ABC-001'", # quoted string
"'O''Brien'", # quoted string, escaped quote
"'ACME (US)'", # parens inside a quoted string are legitimate
"k1='a',k2='b'", # composite key
"%2F", # already percent-encoded separator
],
)
def test_accepts(self, good: str):
url = _build(entity_set_name="engineOverviews", record_id=good)
assert url.endswith(f"engineOverviews({good})")
url = _build(entity_set_name="widgets", record_id=good)
assert url.endswith(f"widgets({good})")

def test_none_record_id_is_unchanged(self):
url = _build(entity_set_name="engineOverviews")
assert url.endswith("engineOverviews")
url = _build(entity_set_name="widgets")
assert url.endswith("widgets")


class TestEmptyRecordIdIsNotTheSameAsNone:
Expand All @@ -99,23 +99,23 @@ class TestEmptyRecordIdIsNotTheSameAsNone:
falsy key skipped validation *and* skipped appending the key — silently
turning a single-record operation into a collection one. ``delete`` and
``patch`` take ``record_id`` as a required positional with no default, so
``bcli delete engineOverviews ""`` composed a DELETE against the whole
entity set. Whether BC would honour that is not the point; the client must
not build it.
``bcli delete widgets ""`` composed a DELETE against the whole entity set.
Whether the server would honour that is not the point; the client must not
build it.
"""

@pytest.mark.parametrize("empty", ["", " ", "\t", "\n"])
def test_empty_record_id_is_rejected(self, empty: str):
with pytest.raises(ValueError, match="must not be empty"):
_build(entity_set_name="engineOverviews", record_id=empty)
_build(entity_set_name="widgets", record_id=empty)

def test_empty_record_id_does_not_silently_become_a_collection_url(self):
with pytest.raises(ValueError):
_build(entity_set_name="engineOverviews", record_id="")
_build(entity_set_name="widgets", record_id="")

def test_none_still_means_collection(self):
url = _build(entity_set_name="engineOverviews", record_id=None)
assert url.endswith("engineOverviews")
url = _build(entity_set_name="widgets", record_id=None)
assert url.endswith("widgets")
assert "(" not in url.rsplit("/", 1)[-1]


Expand All @@ -126,7 +126,7 @@ def test_rejects_path_syntax(self, bad: str):
_build(entity_set_name=bad)

def test_accepts_a_normal_entity_set(self):
assert _build(entity_set_name="engineOverviews").endswith("engineOverviews")
assert _build(entity_set_name="widgets").endswith("widgets")


class TestCustomApiRouteStillValidated:
Expand All @@ -135,20 +135,20 @@ class TestCustomApiRouteStillValidated:
def test_publisher_traversal_still_rejected(self):
with pytest.raises(ValueError, match="publisher"):
_build(
entity_set_name="engineOverviews",
entity_set_name="widgets",
publisher="../..",
group="technical",
version="v1.5",
group="ops",
version="v1.0",
)

def test_custom_route_composes(self):
url = _build(
entity_set_name="engineOverviews",
publisher="beautech",
group="technical",
version="v1.5",
entity_set_name="widgets",
publisher="contoso",
group="ops",
version="v1.0",
)
assert "/api/beautech/technical/v1.5/" in url
assert "/api/contoso/ops/v1.0/" in url


class TestBoundActionKeyIsValidated:
Expand Down Expand Up @@ -177,23 +177,22 @@ def test_rejects_a_traversing_key_in_a_bound_action(self):
client._resolve_url_for_target(
ENV,
COMPANY,
"engineOverviews(1)/../../glEntries('X')/Microsoft.NAV.doThing",
publisher="beautech",
group="technical",
version="v1.5",
"widgets(1)/../../ledgerEntries('X')/Microsoft.NAV.doThing",
publisher="contoso",
group="ops",
version="v1.0",
)

def test_still_resolves_a_legitimate_bound_action(self):
client = self._client()
url = client._resolve_url_for_target(
ENV,
COMPANY,
"engineOverviews(f99bd320-b400-4189-b3c1-c62c05d4e7a5)/Microsoft.NAV.updateLlpUtilization",
publisher="beautech",
group="technical",
version="v1.5",
"widgets(00000000-0000-0000-0000-000000000001)/Microsoft.NAV.doThing",
publisher="contoso",
group="ops",
version="v1.0",
)
assert url.endswith(
"engineOverviews(f99bd320-b400-4189-b3c1-c62c05d4e7a5)"
"/Microsoft.NAV.updateLlpUtilization"
"widgets(00000000-0000-0000-0000-000000000001)/Microsoft.NAV.doThing"
)
Loading