From 0c06d1ff61b581ff203cefa917d09cb9a43db884 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Tue, 4 Aug 2026 13:20:06 -0500 Subject: [PATCH] test(url): use placeholder identifiers, not real tenant values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This test was written with a real company GUID, a real sandbox environment name, and a real custom-API publisher/group. This repo is public and domain-neutral by design; concrete tenant identifiers belong in the private overlay that consumes it, never here. Switched to the conventions the neighbouring URL tests already use — company "abc-123", publisher "contoso" — plus a placeholder GUID and generic entity names. The traversal payloads keep their shape, which is the only part that matters to what is being asserted. Worth noting for anyone auditing later: the wheel contains only the three packages, but the sdist ships the test tree, so a value left here would have been published inside an immutable PyPI artifact rather than only living in git. --- tests/test_url/test_record_key_validation.py | 101 +++++++++---------- 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/tests/test_url/test_record_key_validation.py b/tests/test_url/test_record_key_validation.py index ab2c628..802f610 100644 --- a/tests/test_url/test_record_key_validation.py +++ b/tests/test_url/test_record_key_validation.py @@ -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 @@ -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: @@ -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", @@ -60,21 +60,21 @@ 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 @@ -82,12 +82,12 @@ class TestRecordIdStillAcceptsRealKeys: ], ) 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: @@ -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] @@ -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: @@ -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: @@ -177,10 +177,10 @@ 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): @@ -188,12 +188,11 @@ def test_still_resolves_a_legitimate_bound_action(self): 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" )