[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 queue names and task IDs were inserted directly into URL paths, allowing reserved characters to change the request URL. Encode qname and item_id in queue creation, profile retrieval and print-task creation with quote(..., safe=""). Reject literal . and .. segments through the existing HttpError/PrintOperationError handling while preserving identifier spelling and request-body references.

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 Print unit suite:

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

   Expected: 46 passed, including 13 regression cases covering all three URL call sites, dot-segment rejection and UUID spelling preservation.

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 Mock, patch
   from requests import Request, Response
   from sap_cloud_sdk.core.protocol.http import HttpClient
   from sap_cloud_sdk.print import PrintClient, PrintQueue
   
   base_url = "https://print.example.invalid"
   qname = "../other?param=x#frag"
   raw_url = f"{base_url}/qm/api/v1/rest/queues/{qname}"
   print("Before:", Request("PUT", raw_url).prepare().url)
   
   http = HttpClient(base_url)
   client = PrintClient(http, Mock(), lambda: Mock())
   response = Response()
   response.status_code = 204
   try:
       with patch("requests.Session.send", return_value=response) as send:
           client.create_queue(PrintQueue(qname=qname))
           print("After: ", send.call_args.args[0].url)
   finally:
       http.close()
   PY

   Expected:

   Before: https://print.example.invalid/qm/api/v1/rest/other?param=x#frag
   After:  https://print.example.invalid/qm/api/v1/rest/queues/..%2Fother%3Fparam%3Dx%23frag
   

   Before, .. changes the path, param=x becomes a query, and frag becomes a URL fragment. Fragments are not sent in the HTTP request target. After, those characters remain encoded inside the queue identifier.

3. Run the repository checks:

   uv run --frozen pre-commit run --files src/sap_cloud_sdk/print/client.py tests/print/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 — N/A, no new module
• (Security-sensitive changes only) I have run /security-review and addressed all ❌ findings

Breaking Changes

No breaking changes for documented Print Service usage. Public client signatures, request bodies, and URLs for valid queue names and task UUIDs are unchanged. These callers need no migration.

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.
• Literal . and .. identifiers now raise the existing PrintOperationError before an HTTP request.
• Pre-encoded identifiers are treated literally and encoded again. Callers passing pre-encoded values should pass the original raw identifier instead.

These behavior changes affect inputs outside the service's documented identifier constraints; 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; the HttpError choice under S7 is a deliberate non-breaking design (see below), and S4 is a pre-existing, out-of-scope observation.

• [S7] / [S8] ✅ The core of the fix. _encode_path_segment uses quote(segment, safe="") so / ? # % and other delimiters are encoded as identifier data, and literal . / .. are rejected before any request is built. Path traversal and URL-structure injection via qname / item_id are both closed at all three call sites (create_queue, get_print_profiles, create_print_task).
• [S7] ✅ (design note) _encode_path_segment signals the ./.. rejection by raising HttpError (client.py:35). This is deliberate and correct: HttpError and PrintOperationError are siblings under PrintError, and every caller's documented contract is “raises PrintOperationError”. Reusing HttpError routes the rejection through each existing except HttpError block, which re-raises PrintOperationError with the original as __cause__ (asserted by the tests). A ValueError/new validation type would escape the except HttpError blocks in create_queue and create_print_task (raw, undocumented type — a breaking change to the public exception contract) and would be mislabeled as a parse error by the generic except Exception in get_print_profiles. The HttpError name reads slightly oddly for a validation reject in isolation, but preserving the exception contract outweighs that local nit.
• [S4] ➖ (pre-existing, not introduced here) _resolve_username (client.py:39-56) base64-decodes the JWT payload without signature verification to read user_name / client_id. This predates the PR and is out of its scope. It is low-risk in context — the value is only a best-effort display username with a client_id fallback, not an authorization decision — but flagging it so it is on the record for a future hardening pass (verify via IASVerifier, or document why the unverified decode is acceptable here).
• [S5] / [S6] / [S11] ✅ No hardcoded secrets, no new logging of sensitive values (the existing logger.error calls log the operation and qname, which are operational identifiers), and no verify=False.

[jplbrun]

Code Review

Reviewed against the 23-criterion review-pr contract. Verdict: ⚠️ Needs Minor Work — no blocking issues; the one open item is non-blocking by the rules. (Reposted from the PR description and refreshed — several earlier flags are now resolved.)

Resolved since first review

• [A1] ✅ Checklist is now complete. The type-hints box and the security-review box are both ticked; the remaining unticked boxes are the non-applicable "Type of Change" alternatives and the correctly-N/A new-module e2e item.
• [C1 — SDK review gate] ✅ "SDK review label present" is now passing (the label is applied).

Still open

• [A3] ⚠️ (non-blocking) No linked issue (Closes #N). Fine to merge without one by the review rules; a public issue reference would help traceability if this tracks a finding.
• [C1 — Integration Tests] ➖ (explained, not caused by this PR) The "Integration Tests" check is red, but this is a known environmental issue in the agent-memory integration suite, unrelated to this diff — this PR only touches Print URL-path construction and adds no integration test. A maintainer re-run should clear it; not a reason to hold the change.

Things done well

• [C7] / [D5] No unused code and no dependency changes (quote is stdlib); the three touched methods keep their existing @record_metrics decorators, so telemetry is unaffected.
• The fix is minimal and surgical (one helper, three call sites), the regression suite parametrizes across all three URL sites, and the first test case drives the real HttpClient.request URL-building path via a mocked Session.send rather than asserting on a mock — so it actually proves the prepared URL is correct.