Visitar URL original
fix(security): reject path-traversal inputs in secret resolver by tiagoek · Pull Request #369 · SAP/cloud-sdk-python · GitHub
Skip to content

fix(security): reject path-traversal inputs in secret resolver - #369

Open
tiagoek wants to merge 3 commits into
mainfrom
fix/secret-path-traversal-validation
Open

tiagoek wants to merge 3 commits into
mainfrom
fix/secret-path-traversal-validation

Conversation

@tiagoek

@tiagoek tiagoek commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a path-traversal vulnerability in the secret resolver and AI Core config
module. Both built filesystem paths from unvalidated module/instance/
instance_name inputs. A crafted value (../default, /etc/passwd) could
select credentials outside the intended service binding — a potential
tenant-isolation bypass in multitenant agents.

Changes (SDK only — no consumer code changes required)

core/secret_resolver/resolver.py

  • _validate_path_component(): rejects separators, absolute/UNC paths, ./..,
    NUL/control chars, >255-char values. Runs before any path assembly or
    env-var fallback — rejected values read no files.
  • _assert_within_base(): canonical-path confinement (Path.resolve) as
    defense-in-depth against symlink/TOCTOU escape.
  • Both wired into _validate_inputs() and both mount attempts
    (_load_from_mount + flat-path branch).

aicore/__init__.py

  • _validate_path_component() + _assert_within_base() called at the start of
    _get_secret(), _get_aicore_base_url(), and _get_secret_dir_mtime()
    before the f-string path assembly.

Tests

  • Full parametrized suite: absolute/UNC/relative traversal, separators,
    NUL/control chars, dot components, overlong values, symlink escape.
  • Proof that a rejected value reads no files and attempts no env fallback.

Docs

  • Security/validation section added to secret_resolver, aicore, and
    agent_memory user-guides.

No consumer changes required

This is a library-level control. Every agent or app using the SDK is protected
automatically on the next version upgrade. All observed production instance
values (default, aicore-instance, hr-advisor-destination-instance,
BTP tenant subdomains) are valid single-component identifiers and continue to
work unchanged.

The only behavioural change: a value that previously silently resolved to a
different binding now raises ValueError (fail-closed — correct behaviour).

Test plan

  • pytest tests/core/unit/secret_resolver/ -v — 62 passed
  • pytest tests/aicore/unit/test_aicore.py -v — 103 passed
  • pytest tests/agent_memory/unit/ -v — 231 passed
  • pytest tests/ -q --ignore=tests/aicore/integration — 3579 passed
  • ruff check resolver.py aicore/__init__.py — all checks passed

@tiagoek
tiagoek marked this pull request as ready for review October 7, 2026 22:08
@tiagoek
tiagoek requested a review from a team as a code owner October 7, 2026 22:09
module/instance values used as filesystem path components were not validated,
allowing traversal sequences (../default), absolute paths (/etc/passwd), and
other escape forms to select credentials outside the intended binding directory.

Adds _validate_path_component() (allowlist: single non-traversal segment) and
_assert_within_base() (canonical-path confinement via Path.resolve) to
resolver.py; wires both into _validate_inputs(), _load_from_mount(), and the
flat-path branch. Extends the same guard to aicore/__init__.py (_get_secret,
_get_aicore_base_url, _get_secret_dir_mtime). Protection is automatic for all
SDK consumers — no code changes required in agent or application code.

Parametrized regression tests cover all attack classes: relative traversal,
absolute POSIX/Windows paths, UNC paths, embedded separators, dot components,
NUL/control characters, overlong values, and symlink escape. Proof that a
rejected value reads no files and attempts no env-var fallback is included.

Documentation updated in secret_resolver, aicore, and agent_memory user-guides.
@tiagoek
tiagoek force-pushed the fix/secret-path-traversal-validation branch from d055bfd to c802f3c Compare October 7, 2026 22:12

@GAMAURER GAMAURER left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks good, missing tests in agent memory, + I am usually against expected security behavior being in the user guide. We could, and likely should look into having a consolidated security guide for the SDK. But scattering expected behavior onto multiple user guides seems to be both too verbose and not the right place for it.

Comment thread src/sap_cloud_sdk/agent_memory/user-guide.md Outdated
Comment thread src/sap_cloud_sdk/aicore/user-guide.md Outdated
Comment thread src/sap_cloud_sdk/agent_memory/user-guide.md Outdated
…s, add agent_memory path-traversal regression tests

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants