Block unsafe PyYAML deserialization in Studio tools - #7176
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces AST-based safety checks to detect and block unsafe PyYAML deserialization (such as yaml.load with unsafe loaders or yaml.unsafe_load) while permitting safe loaders. It also adds corresponding unit tests. The reviewer identified a potential security bypass where users could alias the load functions themselves (e.g., my_load = yaml.load) to evade detection. To address this, the reviewer suggested updating visit_Assign to track load function aliases and adding test cases to verify that these aliased calls are correctly blocked.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
studio/backend/core/inference/tools.py (4547-4554)
The current implementation of 'visit_Assign' only tracks aliases for safe loaders (e.g., 'Safe = yaml.SafeLoader'). It does not track assignments of the load functions themselves (e.g., 'my_load = yaml.load' or 'my_unsafe_load = yaml.unsafe_load'). This allows users to bypass the safety check by aliasing the load functions before calling them. We should update 'visit_Assign' to also track assignments of 'load' and 'unsafe_load' attributes.
def visit_Assign(self, node):
if _pyyaml_loader_is_safe(
node.value, self.yaml_aliases, self.yaml_safe_loader_aliases
):
for target in node.targets:
if isinstance(target, ast.Name):
self.yaml_safe_loader_aliases.add(target.id)
if isinstance(node.value, ast.Attribute) and isinstance(node.value.value, ast.Name):
if node.value.value.id in self.yaml_aliases:
if node.value.attr == "load":
for target in node.targets:
if isinstance(target, ast.Name):
self.yaml_load_aliases.add(target.id)
elif node.value.attr == "unsafe_load":
for target in node.targets:
if isinstance(target, ast.Name):
self.yaml_unsafe_load_aliases.add(target.id)
self.generic_visit(node)
studio/backend/tests/test_sandbox_tools.py (30-57)
Add test cases to verify that aliased 'yaml.load' and 'yaml.unsafe_load' calls are correctly blocked by the safety check.
@pytest.mark.parametrize(
"code",
[
(
"import yaml\n"
"yaml.load("
"'!!python/object/apply:os.system [\"echo pwned\"]', "
"Loader=yaml.Loader"
")"
),
(
"import yaml as y\n"
"y.load("
"'!!python/object/apply:os.system [\"echo pwned\"]', "
"Loader=y.Loader"
")"
),
(
"from yaml import load, Loader\n"
"load('!!python/object/apply:os.system [\"echo pwned\"]', Loader=Loader)"
),
(
"import yaml\n"
"yaml.unsafe_load('!!python/object/apply:os.system [\"echo pwned\"]')"
),
"import yaml\nloader = get_loader()\nyaml.load('a: 1', Loader=loader)",
(
"import yaml\n"
"my_load = yaml.load\n"
"my_load('!!python/object/apply:os.system [\"echo pwned\"]')"
),
(
"import yaml\n"
"my_unsafe_load = yaml.unsafe_load\n"
"my_unsafe_load('!!python/object/apply:os.system [\"echo pwned\"]')"
),
],
)
* Harden desktop release token permissions * Specify UTF-8 for workflow permission tests
…menu opt-in (unslothai#7171) * Studio: make sidebar settings cog clickable, opens settings directly * Studio: truncate long profile names so the settings cog stays visible * Studio: tighten spacing between profile name and settings cog * Studio: render settings cog as a sibling button instead of nesting it in the account trigger * Studio: cap very long names in the welcome greeting * Studio: hide the Canvas chat menu item by default behind a settings opt-in * Studio: keep Canvas in the chat menu settings list as the visibility toggle * Studio: drop the Canvas row description in chat menu settings * Studio: keep Canvas visible for profiles that pinned it before the visibility flag
…nslothai#7162) The permission-levels feature defaults an unset permission_mode to ask on streaming requests, so the headless smoke probes hang at the approval prompt until the job timeout. Declare permission_mode full in the tool probe bodies; the gate itself is covered by unit tests.
…unslothai#7158) * Make the Inkling reasoning-effort coercion a module-level helper so duck-typed engine stand-ins keep working * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Align Inkling minimal reasoning effort with the reference implementation (0.1) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
unslothai#7152) --------- Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>
for more information, see https://pre-commit.ci
…ror (unslothai#7193) * fix(save): unsloth_push_to_hub_gguf(save_method="lora") raises NameError unsloth_push_to_hub_gguf reads is_main_process at save.py:3181 but never declares it. Its twin unsloth_save_pretrained_gguf declares it (2783) and uses it the same way (2839) -- the LoRA branch was copied between the twins, the parameter it depends on was not. There is no module-level global, so the name resolves as a global load and the branch raises NameError 100% of the time. save_pretrained_gguf(save_method="lora", push_to_hub=True) raises a ValueError that tells users to "use .push_to_hub_gguf(save_method='lora') instead" -- the documented escape hatch is the broken call. Add is_main_process to the signature, positioned as in the twin, and forward it to unsloth_save_pretrained_gguf on the merged path so the parameter is not silently ignored there. Default stays True, so nothing changes for existing callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(save): preserve GGUF push compatibility --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Etherll <61019402+Etherll@users.noreply.github.com>
* refactor(studio): move chat model picker into features/model-picker
Relocate model-selector + its support files from components/assistant-ui
into a self-contained features/model-picker feature (own barrel), mirroring
the modular Hub layout. Pure move + import repoint; no behaviour change.
* feat(model-picker): add per-model config persistence layer
Superset PerModelConfig (customContextLength, kvCacheDtype, speculativeType,
specDraftNMax, tensorParallel, chatTemplateOverride, trustRemoteCode) persisted
to localStorage (unsloth_model_configs) with schema versioning + LRU budget.
KV-dtype and speculative value sets match main's sidebar (no q4_0/ngram-simple).
Reuses features/hub/lib/model-identity for normalization; adds storage-key layer
and applyPerModelConfigToRuntime (sets tensorParallel, which the old PR omitted).
* feat(picker): modular backend for chat-template validate + default fetch
New studio/backend/picker package (schemas/service/routes) mounted at /api/picker:
- POST /api/picker/validate-chat-template (Jinja syntax validation, no false positives)
- GET /api/picker/chat-template/{model_name} (default template from tokenizer_config.json,
reusing get_cache_path/resolve_cached_repo_id_case; graceful null, no model-code exec)
Frontend api/templates.ts client + hooks/use-model-defaults lazy cache. No backend
changes to the existing inference load route (per-model load fields already supported).
* feat(model-picker): bind picker on-device list to shared hub inventory
Picker now sources cached + local models from useHubInventory (the Hub's shared
store) via a thin adapter, replacing its own /api/models/* fetchers + module
caches. Hub, download manager, and picker now share one source of truth, so
completed downloads reflect in the picker automatically. Partial/live-download
rows are filtered from the cached lists (unchanged rendering). Local naming/search
preserved via additive LocalInventoryRow modelId/displayName. Variant expander,
scan-folder management, recommended-fit, search, external providers untouched.
Known minor: cached 'Downloaded date' sort tiebreak degrades to alphabetical
(hub cached rows carry no mtime); default 'recent' (load-time) sort preserved.
* feat(model-picker): per-model config step inside the picker
Picking a (non-external) model now opens an in-picker config view built from
main's current load controls (context length, KV cache dtype, speculative
decoding, draft tokens, tensor parallel) plus a chat-template editor backed by
the picker validate/default endpoints. 'Remember for this model' persists the
config per model+variant; Run forwards the config to the existing load flow via
meta.config. External models bypass the step. Two-view orchestration lives in
model-selector (single interception point); pickers.tsx call sites untouched.
trustRemoteCode dropped from PerModelConfig to preserve main's per-load consent.
* feat(chat): apply/persist per-model config through the load flow
handleCheckpointChange threads meta.config into the selection; stageOrLoad and
the autoload/Hub-run paths now apply the picker config (explicit pick or saved
remembered config) via applyPerModelConfigToRuntime before staging/loading, with
keepSpeculative set so a remembered speculative mode survives the model switch.
Replaces the old remembered-load-settings seeding (resolveInitialConfig now the
single source). SelectedModelInput carries config.
* refactor(chat): remove per-model load config from the right sidebar
The load knobs (context, KV cache, speculative, draft tokens, tensor parallel)
and the chat-template editor now live only in the picker config step. The sheet's
Model section keeps the staged Load/Cancel flow (config is applied at pick time);
sampling params, system prompt, and RAG are unchanged. Deletes the superseded
remembered-load-settings module + the store's applyRememberedLoadSettings action,
removes the now-dead sheet state/imports, and points the settings reset at
unsloth_model_configs. Delete-cleanup deferred (stale config is LRU-capped).
* fix(model-picker): remove leftover sidebar-staging cogwheel + empty Model section
The downloaded-variant gear (ModelLoadSettingsAction) staged a model straight
into the right-sidebar Run-settings flow -- the old 'configure before load' path
now fully replaced by the in-picker config step. Removed the gear + its component.
Also gate the sheet's 'Model' section to staged picks only (pendingSelection):
after the load-knob strip its content is staged-only, so it was rendering an
empty section header whenever a model was merely loaded.
* chore(chat): remove dead per-model-config setters + modelControlsDisabled
After the load-config UI moved into the picker, the store's per-model setters
(setKvCacheDtype/setSpeculativeType/setSpecDraftNMax/setTensorParallel/
setCustomContextLength/setChatTemplateOverride) had zero callers
(applyPerModelConfigToRuntime writes via setState), and the sheet's
modelControlsDisabled was unreferenced. Verified dead across the whole tree.
* fix(chat): config-step Load actually loads (ignore Load-on-selection)
Root cause: with Settings > Chat > 'Load on selection' turned OFF, the config
step's load went down the deferred-staging path -- opening the right sidebar with
'<model> is staged, not loaded yet / Choose Load model'. The in-picker config step
IS the deliberate load action, so its Load now loads immediately (or downloads +
auto-loads when not cached) regardless of the toggle. Renamed the button
'Run model' -> 'Load model' to match. Native/dropped picks still honor the toggle.
* refactor(chat,hub): retire 'Load on selection' — config step is the only load flow
The in-picker config step (and the Hub Run button) now fully supersede the old
stage-to-sidebar flow, so the Load-on-selection toggle is removed everywhere:
- chat stageOrLoad: every pick loads immediately, or downloads + auto-loads when
not cached (the previous default behaviour, now universal).
- hub Run: drops the stage branch; downloaded GGUFs load directly with their saved
per-model config (no collision with the chat config step — both end at selectModel).
- store: removed loadOnSelection field/setter/key/default; Settings>Chat toggle and
its settings-reset entry removed.
- staged sidebar section is now a download-progress view (auto-loads on completion).
No manual staging remains; stageModel is used only for background auto-load downloads.
* feat(model-picker): default chat template from GGUF + thread variant through config flow
Read the embedded tokenizer.chat_template from GGUF files (read_gguf_chat_template
in gguf_metadata) and use it as the per-model default. Plumb gguf_variant through
the picker service, /api/picker/chat-template route, frontend templates API, and
use-model-defaults so the right variant's template is fetched.
Also refine the picker config-page/model-selector wiring, drop the dead
ggufNativeContextLength runtime path, and add the per-model-config storage keys to
the settings prefs export.
* feat(model-picker): read safetensors chat template + hide editor where it has no effect
Resolve the default chat template for safetensors models: prefer the modern
chat_template.jinja, fall back to the tokenizer_config.json chat_template field,
then chat_template.json (multimodal processor), then the GGUF embedded template.
Applied to local dirs, the HF cache snapshot scan, and the HF remote fetch.
Hide the chat-template editor in the picker for safetensors models — the override
is only applied at load by the GGUF/llama.cpp backend, so editing it on safetensors
currently has no effect. GGUF keeps the editor. Nothing removed; the dialog stays
for when the safetensors apply path is wired up in a later branch.
* fix(model-picker): set legacy-migration flag only after the write succeeds
Set unsloth_model_configs_migrated only once writeMap confirms the migrated
map persisted, so a quota/storage failure no longer marks migration done and
silently drops the user's pre-existing remembered settings — the next load retries.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* MVP model picker fixes
* MVP picker config fix
* MVP safetensors config
* MVP max seq config
* MVP max seq fix
* Fix static max tokens cap ignoring model context
* Fix picker GGUF scan parity
* fix(studio): harden model picker config loading
Apply remembered per-model configs consistently from picker and Hub loads, keep default configs from overriding standing speculative settings, add config access for direct local GGUF files, and support saving or forgetting active model settings without a reload.
* Fix model picker config flow
* Fix model picker config loads
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Avoid recursive per-model config migration reads
* Apply the displayed context length when loading a GGUF
* Fix template validation, cached template lookup, and failed load rollback
- Validate chat templates with the loopcontrols extension so templates
that use break or continue tags pass the picker validator, matching the
inference renderer that already accepts them.
- Read the default chat template from the newest cache snapshot rather than
an arbitrary iterdir order, so an older cached revision no longer prefills
a stale template.
- Capture the runtime per-model config before a load and reapply it when the
load fails, so a failed switch leaves the active model context, KV cache,
template, and speculative settings as they were.
* Make chat template view only for safetensors models
Custom chat template overrides are applied at inference only for GGUF
models, which pass the template to llama-server. The safetensors backend
renders with the model built-in template and ignores the override, so
editing it would save a value that never loads. For safetensors the
config page now opens the template as a read-only preview with a note
that editing is not available yet. This can become editable once
inference support for custom safetensors templates lands in main.
* Fix model picker config edge cases
- Restore prior runtime config when a load no-ops for the active model
- Cap the picker validator request body via the protected prefixes
- Keep the GGUF context slider max above the loaded context
- Fetch subfolder chat templates for uncached Hub repos
- Show the compare side config when reopening the picker
* Keep saved GGUF context above the fallback ceiling
* Show the model config in the run settings sidebar
* Fix model config sidebar reset and context slider
- Stack the remember toggle and action buttons in the sidebar
- Reset the config to defaults instead of the loaded values
- Fetch the native context so the slider max is not the loaded value
* Fix model picker config and download regressions
- Run picker chat template routes off the event loop
- Depth and root guard local template directory scans
- Restore download manager flow for uncached hub picks
- Apply per model context length on reload
- Import model picker symbols from the feature barrel
* Fix model picker config and cached download sorting
- Restore load settings when a Hub run is rejected mid load
- Reuse one NumericValueInput instead of a duplicate copy
- Fix double decode of the model name in the template route
- Remove the unused reset-to-loaded settings action
- Fix cached model download sorting
* Fix model picker per-model config edge cases
Honor a saved or typed max seq length above the model's native context so
RoPE extended values are no longer clamped and silently overwritten. Allow
typing past native while the slider keeps native as a soft ceiling.
Guard the fetch success paths in use-model-defaults against an aborted
signal, and refetch when the HF token changes.
Hash the chat template content in the sidebar remount key instead of its
length. Enable reset for a GGUF whose native context is unknown, and floor
the context slider max so it can never fall below the min.
* Fix GGUF context auto-fit and gated model config token
Stop forcing a 32768 context when a GGUF native context is unknown so the backend auto-fits to VRAM again, while still honoring an explicit context edit.
Send the HF token as a query param so gated safetensors models resolve their max position embeddings.
Derive model default state during render to drop the set-state-in-effect calls.
* Fix native GGUF context ceiling and guard picker template reads
Restore the native context store field so the sidebar slider keeps the
full ceiling for drag and drop GGUFs. Limit local chat template reads to
the browse allowlist, skip malformed repo ids, and drop unused model
picker exports.
* Fix model picker lint boundaries
* Fix model picker review findings
Chat template editor never seeded its draft. Radix only calls onOpenChange
from internal events, so the seed in the nextOpen branch was dead and a model
with a saved override opened empty. Saving then cleared the override. Drop the
dead branch, treat draft as an untouched sentinel, and reset it on every close.
Uncached Hub picks could auto load a model after the user left the chat. Main
detached the staged pick on route exit and on chat context change. Carry the
context key on the pending pick and skip the load when it no longer matches.
Also clear configTarget when the picker closes, restore the onUpdated ref so
variant rows stop resubscribing on every parent render, skip the LRU write when
the entry is already most recent, import NumericValueInput relatively, and drop
the unused ModelUpdateAction barrel export.
* Preserve GGUF context on active reload
* Fix model picker per-model config regressions
- Stop reloading the already loaded model on re-pick
- Hide infra models from the chat picker
- Detect vision support on cached GGUF repos
- Honor saved maxSeqLength on auto load
- Restore default chat template for local GGUFs
- Warn on save failure and revert config on cancel
- Refetch picker inventory on open
- Persist read only per model config safely
* Fix stale model auto load
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Fix model picker numeric input sizing and constraints
Size value inputs to their content so long context lengths are not clipped,
restrict them to numeric characters, and stop the speculative decoding label
from truncating in the sidebar.
* Fix picker CI tests and harden chat template resolution for PR unslothai#6647
- tests: point the descender guard at the moved model-selector.tsx path
- tests: exclude the disabled Reload model button from the regenerate locator so .first targets the real Regenerate
- picker/service.py: reject symlinked template/gguf leaves that resolve outside the browse allowlist (HF cache reads unchanged)
- compare mode: resolve each pane's own remembered chat template instead of inheriting the other pane's from the store
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Protect future-schema per-model configs from deletion for PR unslothai#6647
savePerModelConfig already refuses to overwrite a stored config whose schema version is newer than this client understands, but deletePerModelConfig did not. Unchecking Remember on an older client therefore silently destroyed a newer client's saved config. Apply the same guard on delete and surface the blocked case through the existing saveFailed toast.
* Protect future-schema per-model configs from quota eviction for PR unslothai#6647
The save and delete guards already refuse to touch a stored config whose schema version is newer than this client understands, but the quota-eviction path did not, so a full store on an older client could still evict a newer client's config. Skip future-schema entries when evicting and fail the save if the budget cannot be met without them.
* Fix GGUF context persistence, compare context, and rollback settings for PR unslothai#6647
Persist a GGUF context override from the user's intent instead of collapsing it against the loaded context, which reintroduced the context-reset (f483878 reverted the native-baseline fix). model-config-page now collapses the saved value against native, and use-chat-model-runtime and chat-adapter retain the requested context on load so re-saving another setting keeps the override; a null request stays null so a VRAM auto-fit never becomes a stored override.
shared-composer: a compare pane with no explicit GGUF context now loads at native (0) like single-view, not the session maxSeqLength that silently shrank the shown context.
use-chat-model-runtime: restore the previous model's KV cache dtype and chat template on a failed-load rollback so it runs as it was, not with backend defaults.
* Preserve native path token when reloading the active model for PR unslothai#6647
handleReloadActiveModel rebuilt the selection without the store's activeNativePathToken, so reloading a file-picked GGUF after a settings change validated the display label as a repo/path and failed. Thread the active native token through the reload selection so native-loaded models reopen correctly.
* Make picker template validation resilient and accept HF generation tags for PR unslothai#6647
Import Jinja lazily inside validate_chat_template so a backend without the optional jinja2 package (GGUF-only installs) still starts instead of raising ModuleNotFoundError at import time. Register a no-op extension for the Transformers {% generation %} assistant-mask tag so pasting a valid HF chat template validates, matching the renderer, rather than being rejected as an unknown tag.
* Honor remembered compare config and parse processor chat_template.json for PR unslothai#6647
* Fix failed-load rollback context and processor template map fallback for PR unslothai#6647
* Restore speculative decoding config on failed-switch rollback
When a model switch fails after the previous model was unloaded, the
rollback reload restored tensor_parallel, KV cache dtype and the chat
template override, but omitted speculative_type and spec_draft_n_max and
cleared their loaded shadows to null. The previous model therefore came
back running at backend defaults (speculation off) while the UI still
showed it enabled, and the status resync confirmed the off state. Resend
the previous model's speculative settings in the rollback load and keep
the store's active and loaded speculative fields in sync with them.
* Reset max sequence length when a model has no saved config
applyPerModelConfigToRuntime reset every per-model field except
maxSeqLength, which it only wrote when the incoming config had one.
maxSeqLength is the sole field carried on store.params, so selecting a
model with no remembered config left the previous model's value in place
and later loaded the new model at that leaked length. Fall back to the
standing default so an unremembered model loads at its own default.
* Surface a message when a variant update cannot start
startManagedUpdate handled the conflict and error start outcomes but let
busy fall through as if the update began, so the confirm dialog closed
with no job created and the cached variant stayed stale. Show an info
message when the repo is busy with a sibling transfer so the click is
not silently dropped.
* Keep per-model speculative choices out of the global default
A staged load with a per-model or one-off config sets keepSpeculative,
which already skips reading the global speculative preference. The
matching save still ran unconditionally, so the model-specific choice was
written to the global unsloth_chat_speculative_type and a later model with
no saved config started from it instead of Auto. Skip saveSpeculativeType
when keepSpeculative so the per-model choice stays isolated.
* Seed non-active model settings from the app default max length
The Run settings page captured initialMaxSeqLength from the loaded
model's runtime params and fell back to it for a model with no saved
config. Opening settings for a different, unloaded model and clicking
Load then sent the active model's context (for example 64k) instead of
the 4096 default, risking validation failures or OOMs. Seed the default
for non-active models and keep the runtime value only for the active one.
* Prefer sidecar tokenizer chat template over the GGUF copy for variants
_chat_template_from_dir returned the embedded GGUF template first when a
variant was selected, reversing the tokenizer-first precedence of the
no-variant path. A model whose chat_template.jinja or tokenizer_config.json
supersedes a stale embedded template then got the wrong template on
variant selection. Keep tokenizer files first regardless of variant; the
variant only picks which GGUF is the fallback. Adds regression tests for
both the tokenizer-wins and gguf-fallback cases.
* Keep per-model speculative choices load-local in autoload and compare
The interactive load path treats a per-model speculative choice as
load-local and skips writing it to the global default. Autoload and
generalized compare still called saveSpeculativeType unconditionally, so a
remembered off or ngram setting leaked into unsloth_chat_speculative_type
and later models with no saved config inherited it. Persist the global
preference only when the value came from the global settings.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Studio: record the compare pane's loaded context in runtime state so the active model's settings and any reload or save use it, not the previous context
* Studio: notify the user when a Hub autoload can't start because another download for the model is already running, instead of silently dropping it
* Studio: drop the merge's orphaned staged-model store helpers and unused alert imports
The main merge left isPendingGguf and pendingSelectionMatches referencing the
removed PendingModelSelection type, and the alert-dialog/alert imports unused
after the permission-mode dropdown replaced the bypass dialog, so tsc -b failed.
* Studio: cache a null default chat template so the viewer stops re-fetching it
A model with no sidecar or embedded template resolves to a terminal null, but
that result was never cached, so reopening the template viewer re-ran the
backend and Hugging Face lookup every time.
* Studio: detect direct-file GGUFs in run settings so Max Tokens uses their context
A GGUF loaded from a local file or custom folder has no variant label, so the
run-settings panel treated it as non-GGUF and clamped Max Tokens to the session
max_seq_length instead of the loaded GGUF context. Detect it via the reported
GGUF context and the .gguf checkpoint suffix, matching the chat page.
* Studio: prompt to re-select a local model file when its lease expired before reload
A file-picked GGUF is reachable only through a native path token that the
desktop host prunes after a TTL. Reloading reused that token blindly, so a
reload long after the initial load failed with an opaque error. Track the
token's expiry and, when it has passed, ask the user to re-select the file
instead of attempting a doomed reload.
* Fix descender-clipping test to tolerate sidebar layout utilities
The sidebar account-block div carries layout utilities (min-w-0, flex-1)
between 'flex' and 'flex-col', so the descender-clipping guard's regex,
which required 'flex' immediately followed by 'flex-col', no longer matched
and the test failed to locate the account-block div. Generalize the prefix
to allow intervening flex utilities while still capturing the leading-*
class before the collapsible visibility utility and asserting leading-tight,
so the guard against clipped glyph descenders is fully preserved.
* Harden picker chat-template resolution
Enforce the 64 KiB chat-template contract at the validate endpoint's request
model so a direct caller cannot submit a template far larger than the frontend
allows (MaxBodyMiddleware only bounds the whole request body, not this field);
oversized templates now return a clean 422.
Apply sidecar-over-GGUF template precedence globally across cached snapshots
instead of per snapshot. A repo with multiple cached revisions previously
returned the first snapshot's template, so a newer GGUF-only revision could
win over an older revision's maintained chat_template.jinja sidecar, which
contradicted the documented intent that sidecars supersede the embedded copy.
* Guard per-model config against future-schema and lossy migration
Two forward-compatibility gaps in the versioned per-model config store:
- The load/apply path returned and normalized a stored record without checking
its schema version, so a record written by a newer client was reinterpreted
under the current schema and applied to a live model load, even though save,
delete and eviction all refuse to touch future-schema records. Reject
future-schema records on load too.
- The one-time legacy migration enforced the storage budget without protecting
the entries it had just migrated and set the completion flag unconditionally.
When storage was already full of future-schema records (which are unevictable
by an older client), the migrated entries were the only evictable ones and
could be dropped while migration was still marked complete. Protect the
migrated keys during eviction and only mark migration complete when they
survive, so it retries once space frees up.
* Discard chat-template validation results after the dialog closes
Server-side template validation is async, but closing or cancelling the editor
did not abort it, so a late-arriving valid response still called onSave and
applied a template the user had already dismissed. Track a validation token
that is bumped on close and ignore any validation result whose token is stale.
* Record native lease expiry when loading a picked GGUF from the chip
The pending-native-model chip loaded via stageOrLoad directly, bypassing
loadNativeModelIntent, so activeNativePathExpiresAtMs was never recorded for a
chip-loaded file. A later reload then either skipped the lease-expiry guard
entirely (expiry left null) or compared against a previously loaded file's
stale expiry, so reload could reuse an already-pruned token or wrongly block a
still-valid one. Route the chip through loadNativeModelIntent, which builds the
same selection and records the expiry.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Prefer sidecar template for a directly selected local GGUF file
A direct .gguf file path read its embedded chat template without checking the
parent directory for a maintained sidecar (chat_template.jinja /
tokenizer_config.json), while directory and variant selections already prefer
the sidecar. That let the config editor preview or save a stale embedded
template for the same model depending on how it was selected. Check the parent
directory sidecars first, then fall back to the embedded copy, and cover both
paths with tests.
* Resolve cached chat template per revision, newest first
The earlier change searched every cached snapshot for a sidecar before
considering any snapshot's embedded GGUF template, which let an obsolete sidecar
from an older revision override the newest revision's template. Restore
per-snapshot resolution (newest first): a revision's sidecar still supersedes
its own embedded GGUF copy, but a newer revision is no longer overridden by an
older revision's sidecar.
* Preserve autoload transport conflicts and surface background busy downloads
- When a Hub autoload hits a transport conflict, keep pendingHubAutoLoad bound
instead of clearing it. Clearing it re-keyed the download surface and its
cleanup cancelled the conflict the toast tells the user to resolve, so the
Hub resume affordance was gone the moment it appeared. Return early on
conflict, mirroring the started branch, so resolving it from the Hub still
auto-loads on completion.
- The background-download branch handled started and conflict but silently
dropped a busy outcome, leaving the user with no feedback when a peer variant
of the same repo was already downloading. Surface the same busy toast the
autoload path uses.
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Han <danielhanchen@gmail.com>
Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com>
Co-authored-by: danielhanchen <michaelhan2050@gmail.com>
This reverts commit 8cbdfbe.
* fix(studio): recover stalled Hub downloads over HTTP * fix(studio): preserve retry generation and progress baseline * fix(studio): keep XET retry handoff nonterminal * fix(studio): preserve retry cancellation on claim failure * fix(studio): make retry failure cancellation atomic * fix(studio): close skipped retry state gaps * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stabilize chat-only export gate detection on Windows * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Retrigger CI on a user-authored head * fix(studio): serialize XET HTTP retry handoff * List XET to HTTP retries that are briefly released from the repo guard as active downloads for PR unslothai#6858 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Settle no-process active downloads on shutdown so a parked XET retry cannot spawn after cleanup for PR unslothai#6858 * Settle exited-error and no-process downloads on shutdown and persist their cancel markers for PR unslothai#6858 * Keep terminal HTTP failures uncancelled and block companion deletion for released retry peers for PR unslothai#6858 * Trim download lifecycle test coverage * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: Etherll <61019402+Etherll@users.noreply.github.com>
--------- Co-authored-by: oobabooga <112222186+oobabooga@users.noreply.github.com>
…on Python 3.14+) (unslothai#7186) * Studio: don't apply nest_asyncio on plain CLI starts (breaks asyncio on Python 3.14+) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Skip nest_asyncio on Python 3.14+ so notebook and embedded Studio starts also work * Tighten the nest_asyncio gate comment --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>
…rs_bias (unslothai#7189) * Propagate fp8 block_size before the early return in get_lora_parameters_bias get_lora_parameters_bias set the fp8 block_size on W/W_quant only after the disable_adapters/merged early return, so on the merged or disabled path (merged inference, DPO reference model) a block-fp8 weight lost its real block_size and downstream fp8 kernels fell back to [128, 128]. The non-bias sibling get_lora_parameters already sets block_size before its early return; move the block so both behave the same. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Guard the fp8 block_size against a missing quant state A decompressed compressed-tensors layer keeps quant_method == "fp8" while its weight is back to bf16, so it has no quant state and get_lora_parameters_bias must still return W_quant None for fast_linear_forward to fall back to a plain matmul. Only attach block_size when a quant state was actually found. * Guard the sibling get_lora_parameters fp8 block_size against a missing quant state Mirror the get_lora_parameters_bias guard so a decompressed compressed-tensors layer (quant_method fp8, bf16 weight, no quant state) does not raise AttributeError on the fused-LoRA path. Add a CPU-only regression test. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
* feat(studio): expose opt-in MCP control plane * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden Studio MCP tools: byte-safe auth, page clamping, forward export/checkpoint fields Follow-up hardening on the opt-in MCP control plane. All changes are additive and backwards compatible. - BearerTokenMiddleware now compares the Authorization header on raw bytes. A non-ASCII bearer value previously reached str-based hmac.compare_digest, which raises TypeError and surfaced as a 500 instead of a clean 401. The constructor also rejects an empty or whitespace-only token so an empty token can never match an empty "Bearer " header. - MCP tools call the route functions directly, which skips FastAPI Query validation. list_training_runs and get_recipe_job_dataset now clamp limit and offset to the same bounds the HTTP routes enforce (a negative SQLite LIMIT otherwise means "no limit"). - export_gguf forwards hf_token (the backend rejects a Hub upload without it), accepts a list of quantization methods, and exposes imatrix / imatrix_path so the IQ low-bit quants are reachable. - load_checkpoint forwards hf_token and approved_remote_code_fingerprint so gated checkpoints and the remote-code approval retry work. Its docstring is corrected: the export backend coexists with training and inference rather than freeing GPU work. - start_training passes via_api_key=False explicitly instead of relying on the unfilled Depends default. Tests: add coverage for non-ASCII and empty-token auth, the correct-token pass through, non-http scope pass through, pagination clamping, and the forwarded export/checkpoint fields. * Harden Studio MCP: cap /mcp request bodies, reject unusable tokens, fix docs Follow-up hardening from a full review pass. All changes are additive and backwards compatible. - Add "/mcp" to _BODY_PROTECTED_PREFIXES so MaxBodyMiddleware enforces the same request-body cap it already applies to every other write endpoint (/api/train, /api/export, /api/data-recipe, ...). The MCP endpoint accepts authenticated POST tool-call bodies; without this an authenticated client could send an unbounded body. The middleware only buffers the request body (not the SSE response), so streaming is unaffected, and the 500MB default cap never affects a real JSON-RPC tool call (verified live). - Reject a non-ASCII UNSLOTH_STUDIO_MCP_TOKEN at construction. HTTP header values are ASCII, so a non-ASCII token cannot be sent by a standard client and would silently lock out the endpoint; fail fast instead. - MCP.md: document the canonical /mcp/ endpoint and note that /mcp redirects to it, so clients that do not follow redirected POSTs still connect. Tests: add non-ASCII token rejection coverage. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten MCP server comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <unslothshared@gmail.com>
…unslothai#7194) * fix(tokenizer): check for tokenizer.model after saving it, not before `fix_sentencepiece_tokenizer` creates its temporary directory, then returns early unless that directory already contains a tokenizer.model: if not os.path.exists(temporary_location): os.makedirs(temporary_location) # fresh, empty if not os.path.isfile(f"{temporary_location}/tokenizer.model"): return new_tokenizer # always true old_tokenizer.save_pretrained(temporary_location) # writes that file The file only appears on the line after the check, so the guard is always true and the body never runs. Nothing else writes that path either -- `convert_to_fast_tokenizer` saves into a per-name subdirectory, not `{temporary_location}/tokenizer.model`. Both call sites are in `get_chat_template` and are commented "Must fix the sentence piece tokenizer since there's no tokenizer.model file!" -- the guard defeats the exact intent the caller states. The effect is silent: the caller still gets a working `new_tokenizer`, but the sentencepiece piece rename is skipped, so the mapped token (e.g. the eos token remapped to `<|im_end|>`) is missing from tokenizer.model and GGUF/llama.cpp exports carry the old piece. `check_if_sentencepiece_model` in save.py does the same probe in the right order -- makedirs, save_pretrained, then isfile. Match it. Tests are added under tests/saving/ next to the existing sentencepiece coverage, and to the two Bucket-A lists in consolidated-tests-ci.yml, since Repo tests (CPU) --ignores tests/saving and these need protobuf. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Clear stale tokenizer.model before the sentencepiece guard The guard now runs after old_tokenizer.save_pretrained, but the default temporary_location is a fixed reusable directory. A fast-only tokenizer writes no tokenizer.model, so a stale file from an earlier sentencepiece call could pass the guard and patch the wrong model (e.g. mixing models in one process, like a long-running server). Remove any existing tokenizer.model first, and add a regression test. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Empty the reusable sentencepiece scratch directory each call The final AutoTokenizer.from_pretrained reloads the whole temporary_location, so removing only a stale tokenizer.model still let other artifacts from a previous tokenizer (added_tokens.json, chat template, etc.) leak into the reload when the default reusable directory is used across models in one process. Recreate the directory instead, and add a regression test for the leaked-artifact case. * Clear only top-level scratch files, keep subdirectories Recreating the whole reusable directory deleted the {name} subtree that convert_to_fast_tokenizer stores a converted tokenizer's source vocab in, so old_tokenizer.save_pretrained could not copy tokenizer.model and the guard returned the tokenizer unpatched for those legacy converted tokenizers. Remove only stale top-level files (all the final reload reads) and leave subdirectories intact. Add a regression test for the converted-source subdirectory. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Keep the current tokenizer's own source vocab when clearing On a repeated get_chat_template(map_eos_token=True) call, the returned tokenizer's vocab_file points back at the top-level tokenizer.model, and the cleanup deleted that source before old_tokenizer.save_pretrained could re-emit it, so the guard returned the tokenizer unpatched. Skip removing the old tokenizer's own source vocab while still clearing stale files from a different tokenizer, and add a regression test. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Use a per-call temporary directory for the sentencepiece fix The scratch directory defaulted to a single shared path, so concurrent or repeated get_chat_template(map_eos_token=True) calls could delete or overwrite each other's tokenizer.model between save and reload (tripping the piece assertion or reloading the wrong model), and stale files from an earlier tokenizer could leak into the reload. Work in a unique per-call subdirectory instead: this isolates every call without deleting anything the caller owns, and replaces the earlier per-file cleanup. Tests updated to read the patched model from the reloaded directory and to cover isolation and source-vocab preservation. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pass only the applied token mappings into the sentencepiece fix get_chat_template mirrors token remaps into tokenizer.model via fix_sentencepiece_tokenizer, but two caller paths passed a mapping that did not match what they wrote to the fast tokenizer JSON, so once the sentencepiece patch runs the model and JSON disagree: - the mapped-token path skipped entries whose target already existed but still passed the full mapping, renaming a piece the JSON never changed; - the EOS-swap path swapped both tokens in the JSON but passed only one direction, leaving two stop_word pieces and no old EOS piece. Pass the applied mapping (and both swap directions) instead. Add regression tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten sentencepiece guard comments * Add SPDX license identifier to sentencepiece guard test * Reclaim the per-call sentencepiece scratch directory The per-call tempfile.mkdtemp fixed the shared-directory race but never cleaned up, so a long-running process leaked one scratch dir per call. The dir cannot be deleted eagerly for sentencepiece tokenizers because the returned tokenizer's vocab_file points into it (a later save_pretrained copies the patched tokenizer.model from there). Reclaim it correctly instead: remove the dir right away on the fast-only path (the returned tokenizer never references it), and attach a weakref.finalize so the sentencepiece dir is removed once its tokenizer is garbage collected. Add regression tests for both. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the scratch-dir reclaim comment --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com>
* Stabilize Studio regression tests Rebuild on current main. Restore the set-membership sidebar account-block matcher (unslothai#6647, which fixed the same order-sensitive regex, was reverted on main, so the guard is failing on main again) and keep the watchdog replacement-race fix, whose blocked-watchdog stub now waits without a timeout so a superseded watchdog stays alive until cleanup regardless of scheduler load. * Tighten the blocked-watchdog stub comment --------- Co-authored-by: Daniel Han <unslothshared@gmail.com>
…nslothai#7195) * fix(dataprep): skip .jsonl lines that are valid JSON but not objects `_read_file_by_format` json.loads each line and hands the result to `_extract_text_from_json`, which assumes a dict: for field in self._TEXT_FIELDS: if field in data and isinstance(data[field], str): A JSON line does not have to be an object -- `"context"`, `["text"]` and `42` are all valid JSON. For those, `field in data` stops being a key lookup and becomes a substring/membership test, so `data[field]` raises: "context" -> "text" in "context" is True (substring!) -> TypeError: string indices must be integers ["text", "foo"] -> TypeError: list indices must be integers 42 -> TypeError: argument of type 'int' is not iterable The TypeError escapes past `except json.JSONDecodeError: continue`, so the whole load dies on one odd line. That except clause is also the tell: a *malformed* line is already skipped gracefully. A *well-formed* line that happens not to be an object should be too -- it carries no text either way. This makes the two agree. Reachable from `unsloth-cli.py:253` (`--dataset foo.jsonl` auto-detect) and `RawTextDataLoader` is exported from `unsloth/__init__.py`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Slim the non-object jsonl regression test and shorten the guard comment --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com>
…nslothai#7230) * fix(studio): equal padding in dataset source segmented control * fix(studio): scope dataset source pill layoutId per component instance
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 180e8994a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isinstance(func, ast.Call) | ||
| and func.args | ||
| and _is_yaml_string(func.args[0]) |
There was a problem hiding this comment.
Because this resolver guard only fires when the resolver call is the immediate callee, code can store the resolved callable first, e.g. f = pydoc.locate('yaml.unsafe_load'); f(payload) or the same with pkgutil.resolve_name, and _check_code_safety returns None for that pattern. That leaves an unsafe PyYAML deserialization path executable in the sandbox despite the new blocklist, so resolver results need to be tracked as unsafe aliases on assignment as well as direct invocation.
Useful? React with 👍 / 👎.
| isinstance(func, ast.Attribute) | ||
| and ( | ||
| func.attr in _PYYAML_SAFE_LOADER_MUTATORS | ||
| or func.attr in {"update", "setdefault", "pop", "clear", "__setitem__"} |
There was a problem hiding this comment.
This branch only recognizes bound SafeLoader mutator calls, so equivalent reflective or unbound dictionary calls are allowed; for example _check_code_safety returns None for getattr(yaml.SafeLoader, 'add_multi_constructor')(...FullConstructor.construct_python_object_apply) followed by yaml.load(..., Loader=yaml.SafeLoader). That re-enables the Python object/apply constructors on SafeLoader and lets the unsafe payload execute under an apparently safe loader.
Useful? React with 👍 / 👎.
| self._merge_pyyaml_scope_states(*exit_states) | ||
| for statement in node.finalbody: | ||
| self.visit(statement) |
There was a problem hiding this comment.
The finalbody is visited after merging only normal/handler exits, so a try/finally that raises before a sanitizing reassignment is analyzed as if the reassignment always happened. For example, _check_code_safety returns None for from yaml import load; try: 1/0; load = print; finally: load(payload), but at runtime the finally block calls the original unsafe PyYAML load before the exception propagates.
Useful? React with 👍 / 👎.
| and isinstance(func.args[0], ast.Name) | ||
| and func.args[0].id == "importlib" |
There was a problem hiding this comment.
This check hard-codes the receiver name to importlib, so aliased import modules and builtins.__import__ lookups with a dynamic attribute name are missed; _check_code_safety returns None for import importlib as il; name='import_module'; getattr(il, name)('yaml').unsafe_load(payload) and likewise for builtins/__import__. Those call paths import PyYAML and invoke the unsafe loader without hitting the new deserialization block.
Useful? React with 👍 / 👎.
| if path and path[-1] in _PYYAML_UNSAFE_LOADERS: | ||
| return path[-1] | ||
| if isinstance(node, ast.Name) and node.id in unsafe_loader_aliases: | ||
| return node.id |
There was a problem hiding this comment.
Unsafe loader detection only catches direct attribute or name references, so reflective access through the PyYAML submodules is missed; _check_code_safety returns None for getattr(yaml.loader, 'Loader')(payload).get_single_data() and for vars(yaml.loader)['Loader'](...). Those constructors are the same unsafe-capable PyYAML loaders that direct yaml.loader.Loader is trying to block, so submodule reflection needs to fail closed too.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Name): | ||
| if node.id in unsafe_load_aliases: | ||
| return "unsafe_load" | ||
| if node.id in load_aliases: | ||
| return "load" |
There was a problem hiding this comment.
This branch only treats a from-imported unsafe PyYAML callable as dangerous when the callee is still a bare Name, so a trivial namespace lookup bypasses it; _check_code_safety returns None for from yaml import unsafe_load; globals()['unsafe_load'](payload) (also with locals()/vars()). That invokes the same unsafe deserializer the import alias tracking is meant to block.
Useful? React with 👍 / 👎.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e2fb73a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return _pyyaml_loader_is_safe( | ||
| current, | ||
| yaml_aliases, |
There was a problem hiding this comment.
When a SafeLoader registry is assigned to a local, e.g. constructors = yaml.SafeLoader.yaml_constructors, this helper later peels constructors['!run'] = run down to the name constructors and returns false because only loader-class aliases are recognized. That lets the new PyYAML guard approve a mutated SafeLoader followed by yaml.load(..., Loader=yaml.SafeLoader), while the direct yaml.SafeLoader.yaml_constructors[...] = ... spelling is blocked, so the registry-mutation bypass remains open.
Useful? React with 👍 / 👎.
| if ( | ||
| isinstance(func, ast.Attribute) | ||
| and func.attr in _PYYAML_SAFE_LOADER_MUTATORS | ||
| and _is_pyyaml_module_expr( |
There was a problem hiding this comment.
This only recognizes module-level registry mutation when the call is spelled directly as yaml.add_constructor(...). If the mutator is first saved, e.g. add = yaml.add_constructor; add('!run', run, Loader=yaml.SafeLoader), the assignment path does not mark add as dangerous and the later yaml.load(..., Loader=yaml.SafeLoader)/yaml.safe_load(...) is approved even though SafeLoader has been extended with arbitrary constructors.
Useful? React with 👍 / 👎.
| self.yaml_load_aliases: set[str] = set() | ||
| self.yaml_unsafe_load_aliases: set[str] = set() | ||
| self.yaml_unsafe_loader_aliases: set[str] = set() | ||
| self.yaml_safe_loader_aliases = set(_PYYAML_SAFE_LOADERS) |
There was a problem hiding this comment.
Seeding SafeLoader, BaseLoader, etc. as proven safe aliases before any PyYAML import makes the assignment visitor classify an ordinary binding to those names as a SafeLoader registry mutation. This rejects harmless code like SafeLoader = object() and even the safe alias spelling SafeLoader = yaml.SafeLoader; yaml.load(..., Loader=SafeLoader), so bare loader names should only become trusted after an actual PyYAML binding proves them.
Useful? React with 👍 / 👎.
| mapping_mutators = {"update", "setdefault", "pop", "clear", "__setitem__"} | ||
| func = call.func | ||
| if isinstance(func, ast.Attribute): | ||
| if ( | ||
| func.attr in (_PYYAML_SAFE_LOADER_MUTATORS | mapping_mutators) |
There was a problem hiding this comment.
The registry mutation check only handles method-style mutations (plus dict.__setitem__), so an unbound helper such as operator.setitem(yaml.SafeLoader.yaml_constructors, '!run', run) is not recorded as unsafe. In that scenario the SafeLoader registry is modified before a later yaml.load(..., Loader=yaml.SafeLoader)/yaml.safe_load(...), and the new policy still approves the code despite the arbitrary constructor.
Useful? React with 👍 / 👎.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ce3d8b40d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(node, ast.Subscript) and _is_dynamic_namespace( | ||
| node.value, dynamic_namespace_aliases | ||
| ): | ||
| key = _subscript_key(node.slice) | ||
| if key in unsafe_load_aliases: | ||
| return "unsafe_load" | ||
| if key in load_aliases: | ||
| return "load" |
There was a problem hiding this comment.
When an unsafe PyYAML callable is imported into the module namespace, this branch only recognizes globals()/locals() lookups whose key is a literal string. For a non-literal key _subscript_key returns None and the call is treated as safe, so code like from yaml import unsafe_load; globals()['unsafe' + '_load'](payload) bypasses _check_code_safety and reaches unsafe deserialization. Dynamic namespace lookups should fail closed whenever unsafe PyYAML aliases are in scope.
Useful? React with 👍 / 👎.
| if not (isinstance(node, ast.Call) and node.args and _is_yaml_string(node.args[0])): | ||
| return None |
There was a problem hiding this comment.
This helper only treats literal resolver strings as PyYAML targets, so pydoc.locate(name)(payload) or resolve_name('yaml.' + 'unsafe_load')(payload) is considered safe even when the runtime value resolves to yaml.unsafe_load. Because these stdlib resolvers can import and return the unsafe PyYAML callable, non-literal resolver arguments need to be blocked rather than ignored.
Useful? React with 👍 / 👎.
| reflected = _reflected_member(func) | ||
| if reflected is None: | ||
| return False |
There was a problem hiding this comment.
When the SafeLoader mutator name is computed, _reflected_member returns None and this path returns False, so calls such as getattr(yaml.SafeLoader, 'add_' + 'constructor')('!run', run) are allowed. That mutates SafeLoader before a later yaml.safe_load(...), reintroducing code-executing constructors through an API this policy is trying to block.
Useful? React with 👍 / 👎.
| self.dynamic_import_aliases = {"__import__", "import_module"} | ||
| self.dynamic_import_module_aliases = {"builtins", "importlib"} |
There was a problem hiding this comment.
In the sandboxed script context, __builtins__ exposes __import__, but this tracked module set only includes builtins and importlib. As a result, getattr(__builtins__, '__' + 'import__')('yaml').unsafe_load(payload) is not caught by the dynamic-import fallback and _check_code_safety returns None, letting unsafe PyYAML deserialization execute.
Useful? React with 👍 / 👎.
| ( | ||
| (isinstance(func, ast.Name) and func.id in {"setattr", "delattr"}) | ||
| or (isinstance(func, ast.Attribute) and func.attr in {"setattr", "delattr"}) | ||
| ) |
There was a problem hiding this comment.
This mutation check only recognizes a direct setattr(...) call (or an attribute literally named .setattr), so aliasing the builtin bypasses it: s = setattr; s(yaml.SafeLoader, 'yaml_constructors', {'!run': run}) is allowed and a later yaml.safe_load('!run') can invoke the injected constructor. The policy needs to track setattr aliases or otherwise fail closed when SafeLoader is passed to an opaque setter.
Useful? React with 👍 / 👎.
|
@codex review |
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0cdaeacf0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isinstance(node.value, ast.Attribute) | ||
| and node.value.attr in _PYYAML_SAFE_LOADER_MUTATORS | ||
| and _pyyaml_loader_is_safe( |
There was a problem hiding this comment.
For assignments, this check only treats direct yaml.SafeLoader.add_constructor attributes as escaped mutators; a reflected lookup such as add = getattr(yaml.SafeLoader, 'add_constructor') or vars(yaml.SafeLoader)['add_constructor'] is neither recorded nor blocked. Calling add('!run', run) then makes yaml.safe_load('!run x') use the registered constructor while _check_code_safety returns OK, so reflected mutator aliases bypass the SafeLoader-mutation protection.
Useful? React with 👍 / 👎.
| if isinstance(target, (ast.Tuple, ast.List)): | ||
| return set().union(*(target_names(elt) for elt in target.elts)) |
There was a problem hiding this comment.
When code destructures a SafeLoader registry out of a tuple/list, e.g. constructors, _ = (yaml.SafeLoader.yaml_constructors, None), flattening the targets here means _update_pyyaml_bindings later tests the entire tuple RHS once and never records constructors as a registry alias. The subsequent constructors['!run'] = run; yaml.safe_load('!run x') is accepted, so the new PyYAML guard can be bypassed by mutating SafeLoader through a destructured alias.
Useful? React with 👍 / 👎.
| (isinstance(node.func, ast.Attribute) and node.func.attr in {"locate", "resolve_name"}) | ||
| or (isinstance(node.func, ast.Name) and node.func.id in resolver_aliases) |
There was a problem hiding this comment.
This resolver predicate only recognizes direct pydoc.locate / pkgutil.resolve_name attributes or from-import aliases, so reflected access such as getattr(pydoc, 'locate')('yaml.unsafe_load')(payload) or vars(pydoc)['locate'](...) is not classified. Those calls still return PyYAML's unsafe callable and _check_code_safety currently returns OK, bypassing the resolver-specific block added for unsafe deserialization.
Useful? React with 👍 / 👎.
| dynamic_namespace_aliases: set[str] = frozenset(), | ||
| ) -> bool: | ||
| """Whether a bound, reflected, or unbound call mutates SafeLoader state.""" | ||
| mapping_mutators = {"update", "setdefault", "pop", "clear", "setitem", "__setitem__"} |
There was a problem hiding this comment.
The registry mutator set omits in-place union helpers. operator.ior(yaml.SafeLoader.yaml_constructors, {'!run': run}) mutates the same mapping just like the tested |= form, but because ior / __ior__ are not in mapping_mutators, the mutation is accepted and a later yaml.safe_load('!run x') can dispatch to the injected constructor.
Useful? React with 👍 / 👎.
| if isinstance(node.func, ast.Name) and node.func.id == "getattr" and len(node.args) >= 2: | ||
| return _subscript_key(node.args[1]) in {"import_module", "__import__"} |
There was a problem hiding this comment.
This helper unwraps direct .__call__ attribute access on known import callables, but not the equivalent reflected form. Code like getattr(__import__, '__call__')('yaml').unsafe_load(payload) resolves the YAML module and executes unsafe_load while _check_code_safety returns OK, so the dynamic-import guard needs to treat reflected __call__ on import callables the same as __import__.__call__.
Useful? React with 👍 / 👎.
| finally: | ||
| self.function_parameter_aliases = parameter_alias_state | ||
| self._restore_pyyaml_scope_state(state) |
There was a problem hiding this comment.
Restoring the whole PyYAML alias state after visiting a function drops assignments that are explicitly declared global or nonlocal. For example, def enable(): global load; from yaml import unsafe_load as load followed by enable(); load(payload) actually installs the unsafe loader in the outer scope, but _check_code_safety returns OK because the import alias is discarded here.
Useful? React with 👍 / 👎.
| elif node.module in {"importlib", "builtins"}: | ||
| for alias in node.names: | ||
| if alias.name in {"import_module", "__import__"}: | ||
| self.dynamic_import_aliases.add(alias.asname or alias.name) |
There was a problem hiding this comment.
This import handling tracks builtins import helpers, but not setattr / delattr aliases. With from builtins import setattr as s, s(yaml.SafeLoader, 'yaml_constructors', {'!run': run}) mutates the SafeLoader registry and a later yaml.safe_load('!run x') is accepted, whereas the equivalent s = setattr assignment is blocked.
Useful? React with 👍 / 👎.
| (isinstance(func, ast.Name) and func.id in self.pyyaml_registry_setter_aliases) | ||
| or (isinstance(func, ast.Attribute) and func.attr in {"setattr", "delattr"}) | ||
| ) |
There was a problem hiding this comment.
The registry setter check only recognizes calls named setattr / delattr, so descriptor forms such as type.__setattr__(yaml.SafeLoader, 'yaml_constructors', {'!run': run}) are missed. That call directly replaces the SafeLoader registry and _check_code_safety still returns OK before yaml.safe_load('!run x') dispatches to the injected constructor.
Useful? React with 👍 / 👎.
| if ( | ||
| isinstance(node, ast.Attribute) | ||
| and node.attr in {"get", "pop", "setdefault", "__getitem__"} | ||
| and _is_dynamic_namespace(node.value, dynamic_namespace_aliases) | ||
| ): | ||
| return True |
There was a problem hiding this comment.
The PyYAML dynamic-namespace helper does not recognize __builtins__.__dict__ / builtins.__dict__, even though the broader auto-safety code treats arbitrary __dict__ lookups as dynamic. As a result, __builtins__.__dict__['__import__']('yaml').unsafe_load(payload) imports YAML and invokes the unsafe loader while _check_code_safety returns OK.
Useful? React with 👍 / 👎.
|
@codex review |
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934dc01b69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _pyyaml_safe_loader_mutator_reference( | ||
| node.value, |
There was a problem hiding this comment.
When the SafeLoader mutator is wrapped before assignment, e.g. add = [yaml.SafeLoader.add_multi_constructor][0], this check only examines the top-level RHS and the generic visit does not record the contained mutator. A user can then call add(...FullConstructor.construct_python_object_apply) and yaml.safe_load an !!python/object/apply payload without _check_code_safety returning an error, so the new PyYAML block can be bypassed by one list/dict indirection.
Useful? React with 👍 / 👎.
| and not (isinstance(func, ast.Name) and func.id in self.function_parameter_aliases) | ||
| and (not node.args or _subscript_key(node.args[0]) is None) |
There was a problem hiding this comment.
Because this condition skips function parameters, a helper like def parse(im, name): return getattr(im(name), "unsafe_load")(payload) is not reported when later called as parse(__import__, "yaml"): the module name is nonliteral so _is_pyyaml_module_expr cannot identify the returned module, and the reflective getattr path never hits the direct .unsafe_load parameter check. This leaves an unsafe PyYAML load executable under the sandbox.
Useful? React with 👍 / 👎.
| return None | ||
|
|
||
| visitor = DeclarationVisitor() | ||
| for statement in getattr(node, "body", ()): |
There was a problem hiding this comment.
For code that contains any lambda, visit_Lambda now calls _function_scope_declarations, but Lambda.body is a single expression rather than an iterable statement list, so this loop raises TypeError before execution. That means benign snippets like f = lambda x: x fail the sandbox checker instead of running normally.
Useful? React with 👍 / 👎.
| resolver_target = ( | ||
| _pyyaml_resolver_target(value, self.pyyaml_resolver_aliases) | ||
| if value is not None | ||
| else None |
There was a problem hiding this comment.
When pydoc.locate or pkgutil.resolve_name is first assigned to a local, e.g. loc = pydoc.locate; loc('yaml.unsafe_load')(payload), this binding logic only records names assigned the result of a resolver call and never records the resolver callable itself. The later call is therefore treated as an ordinary function call, allowing PyYAML unsafe_load to be invoked by the sandbox.
Useful? React with 👍 / 👎.
| reflected = _reflected_member(node) | ||
| if reflected is not None: | ||
| base, member = reflected | ||
| return member in _PYYAML_SAFE_LOADER_REGISTRIES and _pyyaml_loader_is_safe( |
There was a problem hiding this comment.
When the registry name is computed, e.g. getattr(yaml.SafeLoader, name)['!run'] = run with name = 'yaml_multi_constructors', _reflected_member returns None and this check falls through instead of treating the target as a possible SafeLoader registry. That lets code install a constructor and then call yaml.safe_load(...) without _check_code_safety reporting the mutation.
Useful? React with 👍 / 👎.
| finally: | ||
| self.loop_depth -= 1 | ||
| body_state = self._pyyaml_scope_state() | ||
| self._merge_pyyaml_scope_states(base_state, body_state) |
There was a problem hiding this comment.
The loop visitor analyzes the body once using only the pre-loop aliases and merges aliases afterward, so loop-carried registry aliases are missed: constructors = {}; for _ in range(2): constructors['...'] = ...; constructors = yaml.SafeLoader.yaml_multi_constructors mutates SafeLoader on the second iteration, yet the later yaml.safe_load is allowed. Loop bodies need a fixpoint or fail-closed handling for aliases created inside the loop.
Useful? React with 👍 / 👎.
| _AUTO_UNSAFE_PY_LOAD_MODULES = frozenset({"torch", "joblib", "cloudpickle"}) | ||
| # PyYAML's safe/base loaders do not construct arbitrary Python objects. Treat | ||
| # yaml.load with any other loader (or a dynamic/missing loader) as unsafe. | ||
| _PYYAML_SAFE_LOADERS = frozenset({"BaseLoader", "CBaseLoader", "SafeLoader", "CSafeLoader"}) |
There was a problem hiding this comment.
SafeLoader inherits constructor registries from yaml.constructor.SafeConstructor, but the protected loader set only covers the loader classes themselves. Code can do yaml.constructor.SafeConstructor.add_constructor('!run', run) or mutate SafeConstructor.yaml_constructors, then yaml.safe_load('!run x') executes the registered constructor while _check_code_safety returns no error.
Useful? React with 👍 / 👎.
| return ( | ||
| isinstance(node, ast.Constant) | ||
| and isinstance(node.value, str) | ||
| and node.value.split(".", 1)[0] == "yaml" |
There was a problem hiding this comment.
pkgutil.resolve_name also accepts package:object names. Because this helper only treats strings whose dot-prefix is yaml as PyYAML targets, resolve_name('yaml:unsafe_load')(payload) is allowed even though it resolves to yaml.unsafe_load at runtime.
Useful? React with 👍 / 👎.
| dynamic_import_alias = value is not None and _is_dynamic_import_callable( | ||
| value, self.dynamic_import_aliases, self.dynamic_namespace_aliases | ||
| ) |
There was a problem hiding this comment.
When __import__ or import_module is wrapped with functools.partial, e.g. im = partial(__import__, 'yaml'); im().unsafe_load(payload), this assignment does not mark im as a dynamic import alias because it only checks whether the outer partial(...) call itself is an import callable. The later zero-argument call is not recognized as a PyYAML module, so the unsafe load runs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48171b00cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| isinstance(func, ast.Call) | ||
| and isinstance(func.func, ast.Name) | ||
| and func.func.id == "getattr" |
There was a problem hiding this comment.
When a SafeLoader mutator is invoked through functools.partial, call.func is an ast.Call, but this new branch only handles getattr(...) calls and then falls through to False. A sandboxed snippet like partial(yaml.SafeLoader.add_constructor, '!x', run)(); yaml.safe_load('!x v') is therefore approved while still mutating the global SafeLoader registry, so the PyYAML deserialization gate can be bypassed without confirmation.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Subscript): | ||
| return _is_dynamic_namespace(node.value, dynamic_namespace_aliases) and _subscript_key( | ||
| node.slice | ||
| ) in {"import_module", "__import__"} |
There was a problem hiding this comment.
If the import callable is moved through a literal container and immediately indexed, this subscript branch returns False because it only accepts dynamic namespaces. For example, [__import__][0]('yaml').unsafe_load(payload) and [import_module][0]('yaml').unsafe_load(payload) pass _check_code_safety even though they reach the same unsafe PyYAML loader path as direct dynamic imports, so the new deserialization block can be bypassed.
Useful? React with 👍 / 👎.
| and ( | ||
| (isinstance(node.func, ast.Attribute) and node.func.attr in {"locate", "resolve_name"}) | ||
| or (isinstance(node.func, ast.Name) and node.func.id in resolver_aliases) | ||
| or (reflected is not None and reflected[1] in {"locate", "resolve_name"}) | ||
| ) |
There was a problem hiding this comment.
This resolver check only recognizes direct pydoc.locate / pkgutil.resolve_name references, so putting the resolver in a literal container bypasses it. For example, [pydoc.locate][0]('yaml.unsafe_load')(payload) and [pkgutil.resolve_name][0]('yaml:unsafe_load')(payload) are currently approved while resolving and calling the unsafe PyYAML loader.
Useful? React with 👍 / 👎.
| if isinstance(func, ast.Attribute): | ||
| if func.attr in ( | ||
| _PYYAML_SAFE_LOADER_MUTATORS | mapping_mutators | ||
| ) and _pyyaml_safe_loader_registry_mutation( | ||
| func.value, |
There was a problem hiding this comment.
When the mutator is reached through super(yaml.SafeLoader, yaml.SafeLoader), func.value is a super(...) call, so this registry-mutation check does not recognize that add_constructor is still bound to yaml.SafeLoader. A snippet such as super(yaml.SafeLoader, yaml.SafeLoader).add_constructor('!x', run); yaml.safe_load('!x v') passes the sandbox check while modifying the global SafeLoader constructor table.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bf31dedad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isinstance(lookup_func, ast.Attribute) | ||
| and lookup_func.attr in {"get", "pop", "setdefault", "__getitem__"} | ||
| and _is_dynamic_namespace(lookup_func.value, dynamic_namespace_aliases) | ||
| ): |
There was a problem hiding this comment.
Because this only accepts bound namespace methods, equivalent unbound lookups are not classified as PyYAML module expressions. In sandboxed Python, import yaml; dict.__getitem__(globals(), 'yaml').unsafe_load(payload) or operator.getitem(globals(), 'yaml') returns the same module, but _check_code_safety currently returns None, so auto mode can execute the unsafe loader this patch is meant to block. Please treat unbound dict/operator lookups with a dynamic namespace first argument the same way as bound lookups.
Useful? React with 👍 / 👎.
| elif ( | ||
| isinstance(func, ast.Name) | ||
| and func.id in mapping_mutators | ||
| and call.args |
There was a problem hiding this comment.
This branch only recognizes registry mutators when the callee name is literally setitem/__setitem__, so aliases or container-selected operator.setitem are missed. For example s = operator.setitem; s(yaml.SafeLoader.yaml_constructors, '!run', run); yaml.safe_load('!run x') passes _check_code_safety even though it mutates SafeLoader before safe_load, bypassing the new SafeLoader-registry guard. Track aliases/literal containers for these mutators as with the other PyYAML callables.
Useful? React with 👍 / 👎.
| isinstance(node, ast.Call) | ||
| and isinstance(node.func, ast.Name) | ||
| and node.func.id == "getattr" |
There was a problem hiding this comment.
This helper only recognizes reflection when the callee is the literal builtin name getattr. In sandboxed Python, g = getattr; g(importlib, 'import_module')('yaml').unsafe_load(payload) and g(yaml.SafeLoader, 'add_constructor')('!x', run); yaml.safe_load('!x') currently return None from _check_code_safety, so aliasing the builtin bypasses the new dynamic-import and SafeLoader-mutator protections. Track aliases to getattr anywhere _reflected_member is used.
Useful? React with 👍 / 👎.
| if ( | ||
| isinstance(node, ast.Attribute) | ||
| and node.attr == "__dict__" | ||
| and _is_dynamic_namespace(node.value, dynamic_namespace_aliases) | ||
| ): | ||
| return True | ||
| if isinstance(node, ast.Attribute) and node.attr == "modules": |
There was a problem hiding this comment.
Because __dict__ is considered a dynamic namespace only when its base is already a dynamic namespace, the imported importlib module's dictionary is ignored. import importlib; importlib.__dict__['import_module']('yaml').unsafe_load(payload) reaches PyYAML's unsafe loader while _check_code_safety returns None, bypassing the dynamic-import guard without using globals() or sys.modules. Include dynamic_import_module_aliases such as importlib when recognizing __dict__ namespace lookups.
Useful? React with 👍 / 👎.
| isinstance(value, ast.Attribute) | ||
| and value.attr in {"setattr", "delattr"} | ||
| and isinstance(value.value, ast.Name) | ||
| and value.value.id in self.dynamic_import_module_aliases | ||
| ) |
There was a problem hiding this comment.
This alias propagation recognizes setattr/delattr and builtins.setattr, but not equivalent class-level setters that the direct call path later blocks. In sandboxed Python, s = type.__setattr__; s(yaml.SafeLoader, 'yaml_constructors', dict(yaml.SafeLoader.yaml_constructors, **{'!run': run})); yaml.safe_load('!run x') is accepted and lets a custom SafeLoader constructor run, so assigning the setter to a name bypasses the SafeLoader-registry mutation guard. Track aliases for type.__setattr__/__delattr__ as registry setters too.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Subscript) and _is_dynamic_namespace( | ||
| node.value, dynamic_namespace_aliases | ||
| ): | ||
| key = _subscript_key(node.slice) |
There was a problem hiding this comment.
This only treats subscripted dynamic namespaces as references to imported PyYAML callables, so the equivalent globals().get/locals().get path is missed. In sandboxed Python, from yaml import unsafe_load as u; globals().get('u')(payload) returns the unsafe loader while _check_code_safety returns None, bypassing the alias protections added here. Handle bound namespace methods such as get, pop, and setdefault for load_aliases and unsafe_load_aliases as well.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Subscript): | ||
| member = _subscript_key(node.slice) | ||
| if member is None: | ||
| return None | ||
| namespace = node.value |
There was a problem hiding this comment.
_reflected_member handles vars(obj)['member'] and obj.__dict__['member'], but not the same lookup through .get. That leaves bypasses such as pydoc.__dict__.get('locate')('yaml.unsafe_load')(payload) and vars(yaml.constructor.SafeConstructor).get('yaml_constructors')['!run'] = run; yaml.safe_load('!run x'), both of which currently return None from _check_code_safety. Treat mapping .get/.pop on reflected dictionaries like subscripts so resolver and registry lookups cannot escape inspection.
Useful? React with 👍 / 👎.
| if ( | ||
| _is_dynamic_import_callable( | ||
| func, | ||
| self.dynamic_import_aliases, | ||
| self.dynamic_namespace_aliases, |
There was a problem hiding this comment.
The dynamic-import guard only recognizes the import callable when it is invoked directly, so higher-order helpers can materialize the PyYAML module without being classified. For example next(map(__import__, ['yaml'])).unsafe_load(payload) and list(map(__import__, ['yaml']))[0].unsafe_load(payload) both pass _check_code_safety while reaching the unsafe loader. Treat higher-order invokers that call __import__/import_module with a YAML literal as opaque unsafe PyYAML module escapes.
Useful? React with 👍 / 👎.
| if _pyyaml_load_call_is_unsafe( | ||
| node, | ||
| self.yaml_aliases, | ||
| self.yaml_load_aliases, | ||
| self.yaml_unsafe_load_aliases, |
There was a problem hiding this comment.
This check only inspects statically visible call nodes, so dynamic execution can build the exact unsafe PyYAML call after the policy has approved the source. In the sandboxed execution path, exec("import yaml\nyaml.unsafe_load(payload)") and import yaml; eval('yaml.unsafe_load')(payload) both make _check_code_safety return None, even though the direct spelling is now rejected. Block exec/eval/compile in the execution-time safety check or recursively analyze literal code strings before allowing them.
Useful? React with 👍 / 👎.
20 participants
Summary
safe_load,safe_load_all, andload/load_allwith a proven safe/base loaderWhy this is a real issue
Before this PR,
_check_code_safety(...)returnedNonefor a harmless proof payload usingyaml.load(..., Loader=yaml.Loader), and_python_exec(...)executed its Python-object constructor. The submitted first pass blocked the direct spelling but could still be bypassed through callable aliases,load_all, unsafe loader classes, conditional rebinding, an escaped module, aliased dynamic imports,yaml.__dict__,sys.modules, andglobals()/locals().After the final fix, those forms require approval and are refused by the safe-execution path. Normal safe loaders and benign name shadowing remain allowed.
Validation
PYYAML_EXECUTEDmarker; the hardened policy rejected the payload before executiongit diff --check: passCompatibility
This changes only Studio's static approval classification. It does not add or alter a CPU/GPU/hardware path. Existing code using
yaml.safe_load,yaml.safe_load_all, or a directly provable Safe/Base loader continues to auto-approve; deliberately dynamic or reflective PyYAML use now requires manual review.