Security, invariant, and test-coverage fixes from a repo gap analysis - #11
Open
thedataengineer wants to merge 10 commits into
Open
Security, invariant, and test-coverage fixes from a repo gap analysis#11thedataengineer wants to merge 10 commits into
thedataengineer wants to merge 10 commits into
Conversation
…rusted input download_artifact extracted with a bare extractall() on Python 3.10/3.11 (no filter= kwarg there), so a malicious or MITM'd server could write outside the dest dir via ../ members, absolute paths, or escaping links. _check_members now resolve-and-contains every member (and link target) against dest and refuses special files, on every Python version; filter="data" stays on 3.12+ as a second layer. Closes open-gitagent#4
…job ids recv_frame allocated whatever the 10-byte header claimed (up to 2**63); _body()/the artifact upload did the same with Content-Length. Both now refuse anything over 512 MiB — a ConnectionError on the socket, a 413 (connection closed) on HTTP, checked before the read either way. A worker also built work-dir paths straight from the hub-supplied job_id in _serve_infer and _run_job; _job_dir now requires a plain path segment, so a rogue or compromised hub can't name paths outside the work root. Part of open-gitagent#5
Both landed with the default umask (0644 world-readable) — settings.json holds the HF token in plaintext. _write_private opens with 0600 and re-chmods pre-existing files. Part of open-gitagent#5
Login was unauthenticated and unlimited — online password guessing at
wire speed. A per-address throttle now answers 429 after 5 consecutive
failures for 60s; success clears the strikes.
Dataset ids came off the URL and went straight into root / f"{id}.json";
rows/meta/delete now require a plain path segment. The static-asset
routes drop their '..' substring denylist for resolve-and-contain.
Part of open-gitagent#5
The optimizer was plain Adam, so every run silently discarded weight_decay (default 0.01) — and the standard trainer paths never seeded, so runs weren't reproducible. All six optimizer sites now build AdamW with the config's weight_decay, and finetune() seeds mx.random before dispatching to any trainer path. The ignored list moves to _ignored_fields() (unit-testable without an mlx install); its optim check now accepts any adamw variant instead of flagging only non-defaults. The unknown-adapter-kind error names the supported kinds and the torch fallback. Closes open-gitagent#7
…rface finetune() checked config.method != 'grpo', which rejects a registered method whose trainer is grpo — the one thing methods.register() exists to allow. It now reads spec.trainer, like every other backend. VERL sees about 7 of TrainConfig's fields; the rest were dropped in silence. _ignored_fields compares against the method's defaults and logs whatever the caller actually set, naming SHADOWLM_VERL_OVERRIDES as the way to reach VERL's own knobs. load() does the same for load_in_4bit/adapter instead of accepting and discarding them. chat() and save() fell through to base.py's generic raises; both now point at backend='torch' like generate() already did. _build_overrides resolves a None learning_rate from the method default rather than raising TypeError on the format string. Closes open-gitagent#6
There was no test job at all — publish.yml only checked versions, built the wheel, and imported it. test.yml now runs tests/ (minus tests/gpu) on 3.10 and 3.12 for every push and PR, plus a compileall; make test runs the same thing locally and a [dev] extra declares pytest, which the suite already imported but nothing installed. New coverage for the cheapest, highest-traffic pure functions: the TrainConfig resolvers and its dict round-trip (the shape the hub sends jobs over the wire), select_backend's five branches and four install messages, and data.py's format detection plus the as_chat/to_texts/split render path a wrong answer would silently mistrain on. Two suite fixes so the whole thing runs without a training backend installed: run_worker used the injected backend_factory for the job but still probed select_backend for the registration name, and the more_plus merge tests imported torch at module scope instead of skipping. The CUDA suite now exits 2 when every test skipped — exiting 0 read as a pass when nothing had run. Closes open-gitagent#3
README told people to run examples/quickstart.py and linked a Colab notebook — neither file is in the repo. Both now point at examples/, which does exist. SHADOWLM_EXTRAS=cli was documented as a lighter UI-only install in README and install.sh, but [cli] resolves to nothing; _cli_entry.py went further and recommended it as the fix for a missing Typer, which it cannot be. eval.py, traces.py, and the worker fleet had no README presence at all while the roadmap listed eval gates and a GPU job queue as future work. They're in the feature table now and the roadmap says what's actually left. CLAUDE.md gains the worker/ws tier (it still described serve.py as a one-job reference server), traces.py, eval.py, the three auth modes and machine tokens, the remote pool, and the two new UI pages. numpy is imported directly by more.py and more_plus.py and was riding in transitively through torch — declared now. models.py's method list was missing more_plus, and six CLI help strings plus load()'s docstring listed only auto|mlx|torch when remote and verl are accepted too. Closes open-gitagent#10
The capture proxy read Content-Length and called model.chat() with no handling, so bad JSON or a failed generation killed the connection. An agent under capture is a real OpenAI client: it retries a JSON error and surfaces the message, but a dropped socket reads as a transport fault and usually takes the run down. Both now return OpenAI-shaped error bodies (400 / 500) and the proxy keeps serving. It also records the OS-assigned port when constructed with port=0, so base_url is right. do_POST's blanket except turned client typos into 500 KeyError: 'model' — /v1/generate, /v1/chat and /v1/prewarm now 422 naming the missing field, and chat validates that messages is a list. torch.has_cuda() swallowed a broken CUDA install and silently ran on CPU; it says so now. The two cuda.empty_cache() swallows in serve.py log what threw, and clear_vram reports the error instead of returning a clean-sweep result it didn't achieve. Closes open-gitagent#8
submit_finetune never sent `name`, so every SDK-submitted run showed up
nameless in the studio's run list, and it had no dataset_id path even
though the server accepts one — you couldn't train on a dataset the
studio already holds. Both are parameters now, with a check that one of
rows/dataset_id is present instead of a server-side 422. The remote
backend passes a name built from the model and method.
pick() ranked on running+pending+gpus and ignored the `workers` count
capacity() reports, so a hub with four idle workers tied with a lone
box. Workers now break the tie ahead of GPU count.
api.ts: getHealth was typed {ok, backend, version} while the server also
returns gpus/running/pending/workers; submitFinetune took `object`, the
one POST body worth typing; login dropped user/expires; MethodInfo's
adapter comment was missing more_plus. Train.tsx narrows model/ds
explicitly — `ready` already guaranteed both, but not to tsc.
Closes open-gitagent#9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3, #4, #5, #6, #7, #8, #9, #10.
Ten commits from the gap analysis behind those issues. Each is self-contained and reviewable on its own; they're stacked because the CI commit needs the suite green first. Happy to split into separate PRs if you'd rather review them apart.
Security
remote.pytar extraction (remote.py: unfiltered tar.extractall on Python 3.10/3.11 allows client-side path traversal #4) —download_artifactfell back to a bareextractall()on Python 3.10/3.11, wherefilter="data"doesn't exist. Sincerequires-python = ">=3.10"that branch is live, and the transport is plain HTTP to whateverSHADOWLM_API_URLnames, so a malicious or MITM'd server could write arbitrary paths on the client. Members are now resolve-and-contained (plus link targets, plus a reject for special files) on every Python version.ws.recv_frameallocated whatever the 10-byte header claimed, up to 2^63;_body()and the artifact upload did the same withContent-Length. Both cap at 512 MiB and refuse before the read (ConnectionErroron the socket, 413 with the connection closed on HTTP).job_id(Worker/hub tier: job_id path traversal, uncapped WS frames and HTTP bodies, auth hardening gaps #5) —_serve_inferand_run_jobbuilt work-dir paths straight from the hub-supplied id._job_dir()requires a plain path segment now. Workers trust the hub to schedule work, not to name paths on their disk.settings.json(plaintext HF token) andtokens.jsonwere written with the default umask. Both are 0600./v1/loginis public and was unlimited. 429 after 5 consecutive failures from an address, 60s window, cleared on success.root / f"{id}.json"unvalidated; the static-asset routes used".."substring denylists. Both are validated/contained now. These were safe in practice becauseBaseHTTPRequestHandlerdoesn't URL-decodeself.path, which is a thin thing to rely on.Invariants the code states but didn't hold
config.method != "grpo"rejects a method registered withtrainer="grpo", which is the one thingmethods.register()exists to allow. Readsspec.trainernow, like every other backend.weight_decay(default 0.01) vanished on every run, and only the more_plus path ever seeded. mlx now builds AdamW with the configured decay and seeds before dispatch; verl logs every field the caller set that VERL won't see. CLAUDE.md's rule is "ignored with a log line, never silently dropped".chat()andsave()fell through to the base class's generic raises. Both point atbackend='torch'now, asgenerate()already did.Errors that went nowhere
model.chat()killed the connection. An agent under capture is a real OpenAI client: it retries a JSON error and surfaces the message, but a dropped socket reads as a transport fault and usually takes the run down. Returns OpenAI-shaped 400/500 bodies and keeps serving. Also records the OS-assigned port when constructed withport=0, sobase_urlis correct.do_POST's blanket except turnedb["model"]into500 KeyError: 'model'./v1/generate,/v1/chat,/v1/prewarmnow 422 naming the field.torch.has_cuda()swallowed a broken CUDA install and quietly ran on CPU. It says so. The twocuda.empty_cache()swallows log what threw, andclear_vramreports the error instead of a clean-sweep result it didn't achieve.Testing
CI ran no tests at all —
publish.ymlchecked versions, built the wheel, imported it. About 20% of the package was never imported by any test, including both training backends and the whole public SDK surface..github/workflows/test.yml: the CPU suite on 3.10 and 3.12, every push and PR, pluscompileall.make testruns the same locally; a[dev]extra declares pytest, which the suite already imported and nothing installed.select_backend's five branches and four install messages, anddata.pyformat detection plus theas_chat/to_texts/splitrender path (a misdetection here silently mistrains rather than erroring).run_workerprobedselect_backendfor the registration name even when handed abackend_factory, and the more_plus merge tests imported torch at module scope.tests/gpu/test_cuda.pyexits 2 when every test skipped. Exiting 0 read as "the GPU suite passed" when nothing had run.81 tests before, 200 after. All green,
tsc -bclean.Protocol and docs
remote.py(Protocol drift: remote.py covers ~11 of ~30 routes; api.ts shapes lag the server #9) —submit_finetunenever sentname(so SDK runs were nameless in the studio) and had nodataset_idpath though the server accepts one.pick()ignored theworkersfieldcapacity()reports, so a hub with four idle workers tied with a lone box.api.ts(Protocol drift: remote.py covers ~11 of ~30 routes; api.ts shapes lag the server #9) —getHealthwas missing four fields the server returns;submitFinetunetookobject, the one POST body worth typing;logindroppeduser/expires;MethodInfo's adapter list was missingmore_plus.examples/quickstart.pyand linked a Colab notebook; neither file is in the repo.SHADOWLM_EXTRAS=cliwas documented as a lighter install in README andinstall.sh, but[cli]resolves to[]— and_cli_entry.pyrecommended it as the fix for a missing Typer, which it cannot be.eval.py,traces.py, and the worker fleet had no README presence while the roadmap listed eval gates and a GPU job queue as future work.numpy(Docs drift: README references missing files, roadmap lists shipped features, numpy undeclared #10) — imported directly bymore.pyandmore_plus.py, riding in transitively through torch. Declared now. This one bit me: a clean venv couldn't collect the test suite.serve.pyas a one-job reference server, with no mention of the worker/ws tier,traces.py,eval.py, the three auth modes and machine tokens, the remote pool, or the two newer UI pages.Left open deliberately
Noted on the issues rather than fixed here:
backends/torch.pyandbackends/mlx.py(CI runs no tests; 20% of package never imported by the test suite #3) — 2,266 lines still verified only by the manual GPU suite.