fix: reject symlinks in extension upload to prevent arbitrary file exfiltration (CWE-22)

[sebastiondev]

Summary

ExtensionsService.create() and ExtensionsService.update() (in both agb/extension.py and python/agb/extension.py) accept a local_path argument and upload the referenced file to the cloud extensions context. Before this change, the only checks were os.path.exists(local_path) and a .zip extension check on the supplied string. Neither rejects symbolic links, and open(local_path, "rb") transparently follows them.

This means a symlink such as evil.zip -> /etc/shadow (or any other readable file on the host) is happily opened, read, and uploaded to the user-accessible cloud context, where the contents can be retrieved. The .zip suffix gate is bypassed because it inspects the link name, not the target.

• CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') / arbitrary file read via symlink
• Affected files / functions:
  • agb/extension.py → ExtensionsService.create, ExtensionsService.update
  • python/agb/extension.py → ExtensionsService.create, ExtensionsService.update
• Data flow: caller-supplied local_path → os.path.exists (passes for symlinks) → .zip suffix check on the link name → _upload_to_cloud → open(local_path, "rb") follows the symlink → arbitrary file contents uploaded.

Fix

Introduce a small helper, _validate_local_path, called at the top of both create() and update() before any I/O:

1. Reject the path outright if os.path.islink(local_path) is true, with a clear ValueError.
2. Canonicalize via os.path.realpath() to catch symlinks in intermediate path components.
3. Require the canonical path to be a regular file (os.path.isfile), raising FileNotFoundError otherwise.
4. Return the canonical path, which is then used for the rest of the flow (extension check, upload).

The existing FileNotFoundError semantics for non-existent paths are preserved. The change is minimal and contained — no public API changes, no new dependencies.

Tests

Added tests/unit/test_extension_symlink_traversal.py (7 tests, all passing) covering both modules and both methods:

• Symlink to a sensitive file (e.g. /etc/shadow-like target) — rejected with ValueError mentioning "symbolic link", and _upload_to_cloud is never called.
• Symlink with a .zip suffix pointing at a non-zip target — still rejected (verifies the suffix gate isn't the only barrier).
• Regular .zip file — still works as before (regression check).
• Non-existent path — still raises FileNotFoundError (behavior preserved).
• Same coverage applied to update().

tests/unit/test_extension_symlink_traversal.py .......                   [100%]
============================== 7 passed in 0.11s ===============================

Security analysis

The exploit requires:

1. A developer's application passing an attacker-influenced local_path into ExtensionsService.create() / update().
2. The attacker being able to place a symlink at that path on the host.
3. The attacker having access to the same cloud extensions context to retrieve the uploaded file.

In that scenario, the SDK silently exfiltrates any file readable by the process to a location the attacker controls. The fix closes this by refusing symlinks before the file is opened, so the .zip extension gate cannot be bypassed via evil.zip -> /etc/passwd, and intermediate-component symlinks (/uploads/link_to_etc/passwd) are also caught via canonicalization.

Adversarial review

Before submitting, I tried to disprove this. The strongest counter-argument is that if the attacker can already plant symlinks on the host or directly control local_path, they likely have meaningful local access already — so why is this a vulnerability rather than a configuration issue? The reason it still matters: the SDK turns "ability to drop a symlink into a watched directory" (a low-privilege primitive that exists in many shared-tenant or upload-staging setups) into "arbitrary file read by the SDK process, exfiltrated to a remote context the attacker can read back". That's a meaningful privilege step, and the .zip suffix check gives a false sense that only zip files are uploaded. There's no framework or upstream protection here — open() follows symlinks by default — so the fix has to live in the SDK. I'd treat this as a hardening / defense-in-depth fix rather than a critical RCE, but worth landing.

Happy to adjust naming, error messages, or split into per-module PRs if you'd prefer.

cc @lewiswigmore