[jplbrun]

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Caller-supplied memory and message IDs were inserted directly into URL paths, allowing reserved characters to change the request URL. Encode memory_id and message_id with quote(..., safe="") in memory retrieval, update and deletion, and message retrieval and deletion. Preserve the surrounding memories(<id>) and messages(<id>) syntax, identifier spelling and request bodies. Encoding failures raise the existing AgentMemoryHttpError.

Related Issue

N/A

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Code refactoring
• Dependency update

How to Test

Run from the repository root after checking out this branch.

1. Run the Agent Memory unit suite:

   uv sync --frozen --all-extras --group dev
   uv run --frozen pytest tests/agent_memory/unit -q

   Expected: 252 passed, including 13 regression cases covering all five URL call sites, pre-encoded inputs, UUID spelling preservation, dot values and encoding failures.

2. Compare the old URL construction with the updated client. This example prepares requests with Session.send mocked; it sends no network requests:

   uv run --frozen python - <<'PY'
   from unittest.mock import patch
   from requests import Request, Response
   from sap_cloud_sdk.core.protocol.http import HttpClient
   from sap_cloud_sdk.agent_memory.client import AgentMemoryClient
   
   base_url = "https://memory.example.invalid/agent-memory"
   memory_id = "x)/../OtherEntity?injected=yes#fragment"
   raw_url = f"{base_url}/v1/memories({memory_id})"
   print("Before:", Request("DELETE", raw_url).prepare().url)
   
   http = HttpClient(base_url)
   client = AgentMemoryClient(http, tenant="subscriber")
   response = Response()
   response.status_code = 204
   try:
       with patch("requests.Session.send", return_value=response) as send:
           client.delete_memory(memory_id)
           print("After: ", send.call_args.args[0].url)
   finally:
       http.close()
   PY

   Expected:

   Before: https://memory.example.invalid/agent-memory/v1/OtherEntity?injected=yes#fragment)
   After:  https://memory.example.invalid/agent-memory/v1/memories(x%29%2F..%2FOtherEntity%3Finjected%3Dyes%23fragment)
   

   Before, .. changes the path, injected=yes becomes a query, and fragment) becomes a URL fragment. Fragments are not sent in the HTTP request target. After, those characters remain encoded inside the memory identifier.

3. Run the repository checks:

   uv run --frozen pre-commit run --files src/sap_cloud_sdk/agent_memory/client.py tests/agent_memory/unit/test_url_identifiers.py

   Expected: formatting, lint, whitespace and type checks pass. Unchanged YAML and lock-file hooks are skipped.

Checklist

• I have read the Contributing Guidelines
• I have verified that my changes solve the issue
• I have added/updated automated tests to cover my changes
• All tests pass locally
• I have verified that my code follows the Code Guidelines
• I have updated documentation (if applicable)
• I have added type hints for all public APIs
• My code does not contain sensitive information (credentials, tokens, etc.)
• I have followed Conventional Commits for commit messages
• (New modules only) I have attached e2e evidence showing the module working against a real BTP service
• (Security-sensitive changes only) I have run /security-review and addressed all ❌ findings

Breaking Changes

No breaking changes for documented Agent Memory usage. Public client signatures, request bodies, and URLs for valid memory and message UUIDs are unchanged. These callers need no migration. UUID validation and normalization are not added.

This security fix changes how other identifier inputs behave:

• URL delimiters such as ), /, ?, #, and % are encoded as identifier data instead of being interpreted as URL structure.
• Pre-encoded identifiers are treated literally and encoded again. Callers passing pre-encoded values should pass the original raw identifier instead.
• Unicode values that cannot be encoded raise AgentMemoryHttpError before an HTTP request.

Literal . and .. values remain allowed because they stay inside memories(...) or messages(...), rather than forming standalone path segments.

These behavior changes affect inputs outside the service's documented UUID format; identical behavior for every previously accepted input is not claimed.

Additional Notes

Security Review

Reviewed against the 12-criterion security-review contract. Verdict: ✅ No security issues — nothing blocking and no open warnings.

• [S7] / [S8] ✅ The core of the fix. _encode_entity_id (client.py:91) uses quote(segment, safe=""), applied at all five identifier call sites (get_memory, update_memory, delete_memory, get_message, delete_message). URL delimiters () / ? # %) are encoded as identifier data, so neither memory_id nor message_id can alter the request URL structure. Path traversal and query/fragment injection are both closed.
• [S7] ✅ (design note — ./.. intentionally allowed) Unlike the sibling Print fix (fix(print): encode queue and task identifiers in URL paths #453), this PR deliberately permits literal . and .., and that is correct here. The ids are wrapped in OData key syntax — f"{MEMORIES}({_encode_entity_id(id)})" with MEMORIES = "/v1/memories" — so the value sits inside memories(...) rather than forming a standalone path segment. /v1/memories(..) cannot climb the path, and once encoded any / becomes %2F regardless. test_dot_values_remain_inside_odata_keys pins this intended behavior.
• [S1] ✅ (unchanged, correctly reused) Tenant routing is unaffected: __init__ already calls _validate_tenant_subdomain(tenant) (client.py:132) and only sets self._tenant for the SUBSCRIBER access strategy. The new encoding sits downstream of that validated routing.
• [S7] ✅ (exception choice is deliberate and non-breaking) Encoding failures raise AgentMemoryHttpError (client.py:99). AgentMemoryHttpError is the error type callers already handle for these operations, so routing the UnicodeEncodeError through it preserves the public exception contract rather than leaking a raw UnicodeEncodeError; the original is kept as __cause__ (asserted by test_unencodable_ids_raise_the_existing_http_exception). A new exception type here would be the breaking change.
• [S5] / [S6] / [S11] ✅ No hardcoded secrets, no new logging of sensitive values (_request's debug log records method, path, and tenant — all operational identifiers), and no verify=False.
• Done well: the lone-surrogate guard ("\ud800" → UnicodeEncodeError → AgentMemoryHttpError before any HTTP call) handles an encoding edge case explicitly, and the regression suite drives the real HttpClient.request URL-building path via a mocked Session.send, so it proves the prepared OData URL rather than asserting on a mock.

[jplbrun]

Code Review

Reviewed against the 23-criterion review-pr contract. Verdict: ⚠️ Needs Minor Work — no blocking issues; one non-blocking item.

Still open

• [A3] ⚠️ (non-blocking) No linked issue (body says "Related Issue: N/A", no Closes #N). Fine to merge without one by the review rules; a public issue reference would help traceability if this tracks a finding.

Clean / verified

• [A1] ✅ Checklist complete — the security-review box is now ticked (review run; see the Security Review section in the PR description). Remaining unticked boxes are the non-applicable "Type of Change" alternatives and the correctly-N/A new-module e2e item.
• [A2] ✅ PR title fix(agent-memory): … is valid Conventional Commits; Commit Validation passes.
• [C1] ✅ All CI green, including Integration Tests. (The SDK-review gate was the only red check; applying the review label clears it.)
• [C3] / [D5] ✅ _encode_entity_id is fully annotated (value: object -> str); the five touched methods retain their @record_metrics decorators and no public API surface changes, so telemetry and typing are intact.
• [C7] / [C8] ✅ No unused code, no dependency changes (quote is stdlib).
• [E1] ✅ New regression suite (tests/agent_memory/unit/test_url_identifiers.py) mirrors the src change and parametrizes across all five call sites, plus pre-encoded input, UUID-spelling preservation, the intentional ./..-inside-OData-key case, and the unencodable-surrogate path.

Things done well

• The fix is minimal and surgical (one helper, five call sites) and makes a correct, deliberate divergence from the sibling Print fix (fix(print): encode queue and task identifiers in URL paths #453): because ids live inside OData key syntax memories(<id>), ./.. are safely allowed rather than rejected.
• The lone-surrogate guard ("\ud800" → AgentMemoryHttpError before any HTTP call) handles an encoding edge case explicitly.
• The regression tests drive the real HttpClient.request URL-building path via a mocked Session.send, so they prove the prepared OData URL rather than asserting on a mock.

[tiagoek]

PR Review: #454 — fix(agent-memory): encode memory and message identifiers in URL paths

Author: jplbrun Branch: fix/hasi-agent-memory-url-injection → main
Verdict: ⚠️ Needs Minor Work
Summary: Security fix is correct, well-tested, and well-documented; two minor housekeeping items before merge.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	✅ Pass	All checklist items ticked or explicitly marked N/A; security-review checkbox correctly ticked and review provided in Additional Notes
A2	Conventional Commits	✅ Pass	fix(agent-memory): … — valid; Commit Validation passes
A3	Issue linked	⚠️ Warning	No Closes #N — body marks it N/A; non-blocking per review rules
A4	AI-generated code disclosure	➖ N/A	No evidence of generated code

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅ Pass	No credentials or internal URLs; .invalid TLD used throughout examples
B2	No sensitive data in PR body	✅ Pass	Demo URL uses fictional domain memory.example.invalid

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅ Pass	All 12 required jobs green, including Integration Tests
C2	Version bump	➖ N/A	pyproject.toml untouched
C3	Type hints	✅ Pass	_encode_entity_id(value: object) -> str fully annotated; existing method signatures unchanged
C4	No hardcoded values	✅ Pass	safe="" is a correct stdlib parameter, not a magic value
C5	Import organization	✅ Pass	quote added to the existing top-level from urllib.parse import … line
C6	Naming conventions	✅ Pass	_encode_entity_id — private, snake_case; test file test_url_identifiers.py — snake_case
C7	No unused code	✅ Pass	All five call sites use _encode_entity_id; quote import was added
C8	No unjustified new dependencies	✅ Pass	quote is stdlib (urllib.parse)
C9	Proto code freshness	➖ N/A	No proto changes

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅ Pass	Helper is private; public signatures unchanged
D2	Public API hygiene	✅ Pass	_encode_entity_id has underscore prefix; no new public symbols
D3	Breaking changes marked	✅ Pass	PR explicitly documents behavior changes for non-UUID inputs in the Breaking Changes section
D4	Pagination & tenant filtering	➖ N/A	No changes to list/query operations
D5	Telemetry instrumentation	✅ Pass	All five modified methods retain their @record_metrics decorators
D6	Multi-tenancy support	➖ N/A	Tenant routing is upstream of the encoding fix; no multi-tenancy changes

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅ Pass	New test_url_identifiers.py covers all 5 call sites via @pytest.mark.parametrize, path injection, pre-encoded double-encoding, UUID spelling preservation, .. inside OData key, and the surrogate-char encoding failure path
E2	Documentation quality	⚠️ Warning	The 5 changed methods document AgentMemoryHttpError: If the request fails. — after this fix the error can also be raised for unencodable IDs before any HTTP call. A "or if the identifier cannot be encoded" clause would make the contract complete. Affected methods: get_memory (line 253), update_memory (line 278), delete_memory (line 308), get_message (line 512), delete_message (line 530)
E2a	Multi-tenancy documentation	➖ N/A	No user-guide.md modified
E3	Module structure compliance	➖ N/A	No new module

---

❌ Blocking Issues

None.

⚠️ Non-Blocking Suggestions

• [A3] No linked issue in the PR body — non-blocking but a public issue reference would help traceability if this tracks an internal finding or CVE.

• [E2] client.py lines 253, 278, 308, 512, 530 — Each of the five changed methods documents AgentMemoryHttpError: If the request fails. in its Raises: section. After this fix, that error can also be raised when memory_id or message_id contains lone surrogates (unencodable characters), before any HTTP request is made. The docstrings should be extended: e.g., AgentMemoryHttpError: If the request fails, or if the identifier cannot be encoded as UTF-8.

✅ Things Done Well

• Minimal, surgical fix. One 10-line private helper and five one-line substitutions close the path-traversal and query/fragment-injection vectors in all affected call sites with no collateral changes.
• Correct ./.. decision. The fix deliberately encodes / but leaves . unencoded. Because the ID sits inside OData key syntax memories(..), a literal .. cannot climb the path — this diverges correctly from the sibling Print fix (#453) which rejects dots entirely. The reasoning is documented and the test_dot_values_remain_inside_odata_keys test pins the intent.
• Surrogate guard is explicit. The \ud800 test proves that lone surrogates raise AgentMemoryHttpError (via __cause__ = UnicodeEncodeError) before any network call, and that http.request is never invoked — exactly the right behavior.
• Tests drive the real URL-building path. test_prepared_urls_contain_ids_without_changing_odata_syntax uses a real Session and a mocked Session.send, so it asserts on the actual prepared OData URL rather than on a mock argument.
• Pre-encoded input test. "%29%2F%2e%2e%2Fother" → "%2529%252F%252e%252e%252Fother" confirms that no special treatment of percent signs is applied, closing the double-encoding ambiguity.

[jplbrun]

PR Review: #454 — fix(agent-memory): encode memory and message identifiers in URL paths

Author: jplbrun Branch: fix/hasi-agent-memory-url-injection → main Verdict: ⚠️ Needs Minor Work Summary: Security fix is correct, well-tested, and well-documented; two minor housekeeping items before merge.

A: Process & Compliance

Criterion Status Finding

A1 PR template complete ✅ Pass All checklist items ticked or explicitly marked N/A; security-review checkbox correctly ticked and review provided in Additional Notes
A2 Conventional Commits ✅ Pass fix(agent-memory): … — valid; Commit Validation passes
A3 Issue linked ⚠️ Warning No Closes #N — body marks it N/A; non-blocking per review rules
A4 AI-generated code disclosure ➖ N/A No evidence of generated code

B: Security & Sensitive Data

Criterion Status Finding

B1 No sensitive data in code ✅ Pass No credentials or internal URLs; .invalid TLD used throughout examples
B2 No sensitive data in PR body ✅ Pass Demo URL uses fictional domain memory.example.invalid

C: Code Quality

Criterion Status Finding

C1 CI checks passing ✅ Pass All 12 required jobs green, including Integration Tests
C2 Version bump ➖ N/A pyproject.toml untouched
C3 Type hints ✅ Pass _encode_entity_id(value: object) -> str fully annotated; existing method signatures unchanged
C4 No hardcoded values ✅ Pass safe="" is a correct stdlib parameter, not a magic value
C5 Import organization ✅ Pass quote added to the existing top-level from urllib.parse import … line
C6 Naming conventions ✅ Pass _encode_entity_id — private, snake_case; test file test_url_identifiers.py — snake_case
C7 No unused code ✅ Pass All five call sites use _encode_entity_id; quote import was added
C8 No unjustified new dependencies ✅ Pass quote is stdlib (urllib.parse)
C9 Proto code freshness ➖ N/A No proto changes

D: API & Design

Criterion Status Finding

D1 API future-proofing ✅ Pass Helper is private; public signatures unchanged
D2 Public API hygiene ✅ Pass _encode_entity_id has underscore prefix; no new public symbols
D3 Breaking changes marked ✅ Pass PR explicitly documents behavior changes for non-UUID inputs in the Breaking Changes section
D4 Pagination & tenant filtering ➖ N/A No changes to list/query operations
D5 Telemetry instrumentation ✅ Pass All five modified methods retain their @record_metrics decorators
D6 Multi-tenancy support ➖ N/A Tenant routing is upstream of the encoding fix; no multi-tenancy changes

E: Tests & Documentation

Criterion Status Finding

E1 Tests added/updated ✅ Pass New test_url_identifiers.py covers all 5 call sites via @pytest.mark.parametrize, path injection, pre-encoded double-encoding, UUID spelling preservation, .. inside OData key, and the surrogate-char encoding failure path
E2 Documentation quality ⚠️ Warning The 5 changed methods document AgentMemoryHttpError: If the request fails. — after this fix the error can also be raised for unencodable IDs before any HTTP call. A "or if the identifier cannot be encoded" clause would make the contract complete. Affected methods: get_memory (line 253), update_memory (line 278), delete_memory (line 308), get_message (line 512), delete_message (line 530)
E2a Multi-tenancy documentation ➖ N/A No user-guide.md modified
E3 Module structure compliance ➖ N/A No new module

❌ Blocking Issues

None.

⚠️ Non-Blocking Suggestions

* **[A3]** No linked issue in the PR body — non-blocking but a public issue reference would help traceability if this tracks an internal finding or CVE.

* **[E2] `client.py` lines 253, 278, 308, 512, 530** — Each of the five changed methods documents `AgentMemoryHttpError: If the request fails.` in its `Raises:` section. After this fix, that error can also be raised when `memory_id` or `message_id` contains lone surrogates (unencodable characters), before any HTTP request is made. The docstrings should be extended: e.g., `AgentMemoryHttpError: If the request fails, or if the identifier cannot be encoded as UTF-8.`

✅ Things Done Well

* **Minimal, surgical fix.** One 10-line private helper and five one-line substitutions close the path-traversal and query/fragment-injection vectors in all affected call sites with no collateral changes.

* **Correct `.`/`..` decision.** The fix deliberately encodes `/` but leaves `.` unencoded. Because the ID sits inside OData key syntax `memories(..)`, a literal `..` cannot climb the path — this diverges correctly from the sibling Print fix (#453) which rejects dots entirely. The reasoning is documented and the `test_dot_values_remain_inside_odata_keys` test pins the intent.

* **Surrogate guard is explicit.** The `\ud800` test proves that lone surrogates raise `AgentMemoryHttpError` (via `__cause__ = UnicodeEncodeError`) before any network call, and that `http.request` is never invoked — exactly the right behavior.

* **Tests drive the real URL-building path.** `test_prepared_urls_contain_ids_without_changing_odata_syntax` uses a real `Session` and a mocked `Session.send`, so it asserts on the actual prepared OData URL rather than on a mock argument.

* **Pre-encoded input test.** `"%29%2F%2e%2e%2Fother"` → `"%2529%252F%252e%252e%252Fother"` confirms that no special treatment of percent signs is applied, closing the double-encoding ambiguity.

addressed