Skip to content

Aafeedback changes tsfm tools - #633

Merged
ShuxinLin merged 6 commits into
aafeedback_changesfrom
aafeedback_changes_tsfm_tools
Oct 15, 2026
Merged

ShuxinLin merged 6 commits into
aafeedback_changesfrom
aafeedback_changes_tsfm_tools

Conversation

@DhavalRepo18

@DhavalRepo18 DhavalRepo18 commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Closes the security issue raised in review: the TSFM recipe tools imported any Python class named in sktime_class, so an agent could run arbitrary code inside the MCP container. In one run GLM-5.3 passed "sktime_class": "subprocess.Popen" with a shell command and read the output back through tsfm__data_quality, exposing raw data, the scorer and the DB password.

This PR keeps the recipe tools (the TSFM scenarios measure model selection and use) and removes every path by which the server runs agent-supplied code or reads files outside its data folders.

Routes closed

Route Fix
sktime_class / nested _target_ imported any dotted path. Reached from run_recipe, run_tabular_recipe, evaluate, run_plan, the finetune block, tabular transforms, catalog cards rewritten via update_model, and param_space.introspect Exact-match allowlist of 30 estimator classes in substrate/resolver.py, checked before import. All routes go through this one function.
register_feature / new_feature_version ran feature-card code with exec() feature_runner.load_program now refuses; both tools removed from the MCP surface. The code-carrying transform cards were removed from the catalog (v1 snapshot kept).
MOIRAIForecaster unpickles model.ckpt (weights_only=False) from any non-Salesforce Hub repo an agent names TSFM server defaults to HF_HUB_OFFLINE=1; weights load only from the preloaded image cache.
Read-back: any dataset_path read any file (e.g. /proc/self/environ), fetched http:// URLs, followed symlinks out; plan step ids path-traversed on write; save_to wrote anywhere io/refs._path confines file pointers to the data folders (TSFM workdir, temp dir, /workspace-share, artifacts/output), resolves symlinks first, refuses URLs. Output names sanitized; save_to confined.

Files

  • substrate/resolver.py: ALLOWED_CLASSES + check in _import_target
  • reasoning/param_space.py: routes its import through the same check
  • engine/feature_runner.py: load_program refuses instead of exec
  • main.py: unregister register_feature / new_feature_version; default HF_HUB_OFFLINE=1
  • io/refs.py, engine/composition.py: file-pointer confinement
  • tests/test_no_code_exec.py (new), tests/conftest.py, tests/test_feature_catalog_store.py
  • feature catalog seed: transform card removed, feature_catalog_orig.json kept as the v1 snapshot

Testing

  • pytest servers/tsfm/tests: 133 passed, 4 skipped.
  • test_no_code_exec.py replays the GLM-5.3 Popen payload down each route and asserts both an error and that a canary file was never written. Against the pre-fix code these tests fail and the payload executes.
  • Smoke suites (smoke_forecast.py, smoke_tasks.py) on runtime:dev with --network none and the full private catalog (59 cards): same coverage as the 2026-10-01 baseline (38 forecast, 11 task cards), no allowlist refusals, task suite unchanged. Moirai 2.0 reproduces byte for byte. Two cards moved PASS to WARN at the 1.0 boundary (chronos-t5-mini 1.01, moirai-1.1-R-small 1.00), from sampling noise in those models, not this change.

Operator notes

  • New estimator class for a catalog card: add its exact path to ALLOWED_CLASSES, or set TSFM_EXTRA_ESTIMATOR_CLASSES (comma-separated).
  • Local runs without the preloaded weight cache: set HF_HUB_OFFLINE=0.
  • stirrup-agent --workspace-dir outside the temp dir: add that folder to TSFM_DATA_ROOTS (os.pathsep-separated; replaces the default list).

Out of scope

  • Write-time validation of sktime_class on catalog cards (the runtime check already blocks it).
  • Raising the torch floor to >=2.6 (the lock already pins 2.10).

…1 snapshot

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
- resolver: allowlist the estimator classes importable from a dotted path
- param_space: route introspect through the same allowlist
- feature_runner: refuse instead of exec(); unregister register_feature / new_feature_version
- main: default HF_HUB_OFFLINE=1 so weights load only from the preloaded cache
- tests: test_no_code_exec.py; update feature catalog tests

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
Data confinment

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
@ShuxinLin
ShuxinLin self-requested a review October 14, 2026 17:24
@DhavalRepo18

DhavalRepo18 commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

@ShuxinLin, this PR is confined to the request.

  • Please run 15 scenarios on a single model to see that the performance does not deviate.

@DhavalRepo18

Copy link
Copy Markdown
Collaborator Author

On myside, I always do smoke test to load the model in real and do an inference.
image
All works.

@ShuxinLin ShuxinLin left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for closing the code-execution routes. An import allowlist plus confining file pointers in refs._path is the right approach. Three changes needed before merge (details inline):

  1. The allowlist blocks features the server still advertises (resolver.py:33): transforms, tabular sktime_class transforms and tsfm_regression now always fail.
  2. _path can still be bypassed (refs.py:46): it checks a name and the file is opened later, so a swapped symlink or a FIFO gets through.
  3. Writes can land outside the data roots (composition.py:209): finetune.training_args.output_dir isn't confined.

I have a few smaller follow-ups and will raise them once these land.

Comment thread src/servers/tsfm/substrate/resolver.py
"""
if "://" in ref and not ref.startswith("file://"):
raise ValueError("only local file pointers are supported")
p = os.path.realpath(ref[7:] if ref.startswith("file://") else ref)

@ShuxinLin ShuxinLin Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confinement can still be bypassed. realpath() checks the path here, but callers open it by name later (pd.read_csv(p)). /workspace-share is writable from the code sandbox, so a symlink swapped in between the check and the open escapes the roots. Non-regular files also pass, so a FIFO named x.csv makes read_csv block forever.

Suggested fix: open first, then validate what was opened. os.open(p, os.O_RDONLY | os.O_NONBLOCK), require stat.S_ISREG(os.fstat(fd).st_mode), check the fd's real path (os.readlink(f"/proc/self/fd/{fd}")) against the roots, and pass the open file to pandas. Every file-reading tool goes through _path, so this covers them all.

path = os.path.abspath(os.path.expanduser(path))
from ..io import refs

path = refs._path(str(path)) # confined to the data roots

@ShuxinLin ShuxinLin Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Writes are only confined for save_to. _with_finetune (line 91) merges the finetune block into the estimator params, and TinyTimeMixer/PatchTST pass training_args straight to transformers.TrainingArguments. So finetune: {training_args: {output_dir: ...}} makes the Trainer write checkpoints, including the pickled training_args.bin, to any writable path.

Suggest overriding output_dir with a server-chosen directory under WORKDIR and rejecting any other path-valued training args.

…then-verify reads, confine training_args)

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
…s, open-then-verify reads, confine training_args)"

This reverts commit 08e817b.

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
…ols advertise

Signed-off-by: Dhaval Patel <pateldha@us.ibm.com>
"""
ns: Dict[str, Any] = {}
try:
exec(code, ns) # noqa: S102 - intentional; sandbox in production

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ShuxinLin this is a main thing we disabled.

@DhavalRepo18 DhavalRepo18 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hi

@ShuxinLin
ShuxinLin merged commit 73460e6 into aafeedback_changes Oct 15, 2026
6 checks passed
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