T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/cutout.py:95
- Finding
- Silent Overwrite of Existing Processed Image Assets<![CDATA[ ## Vulnerability Details **File Location**: `scripts/cutout.py`, lines 95–103 **Vulnerability Type**: Unsafe file handling and silent artifact overwrite **Risk Level**: Medium ### Complete Code Snippet ```python def safe_save_path(path): """Return a non-existing path: bump _v2/_v3... suffix instead of overwriting.""" base, ext = os.path.splitext(path) cand, n = path, 1 while os.path.exists(cand): n += 1 cand = "%s_v%d%s" % (base, n, ext) return cand ``` The vulnerable output operation does not use that helper: ```python rgba = Image.fromarray( np.dstack([ np.clip(a, 0, 255).astype(np.uint8), (alpha * 255).astype(np.uint8) ]), "RGBA" ) bbox = rgba.getchannel("A").point(lambda v: 255 if v > 8 else 0).getbbox() if bbox: rgba = rgba.crop(bbox) rgba.save(os.path.join(OUT, name + ".png")) ``` The associated comments claim that existing outputs are protected: ```python # Input/output dirs: override with env vars CUTOUT_IN / CUTOUT_OUT. # Defaults are relative to this script; existing outputs are never overwritten # (auto version bump via safe_save_path below). ``` ### Technical Analysis The script processes ten fixed asset names and saves each result under a deterministic filename, such as `hero.png`, `flow.png`, or `robot.png`. Pillow's `Image.save()` replaces an existing file when the destination path already exists. Although the script provides `safe_save_path()`, the helper is not applied to the processed asset outputs. It is only used later for the verification sheet. This contradicts both the comments in this script and the Skill's documented guarantee that existing artifacts are never silently overwritten. The output directory is controlled through the `CUTOUT_OUT` environment variable. Consequently, a caller can direct the script to any writable directory. Files matching one of the ten fixed output names will then be replaced without confirmation or backup. This is a destr ...[truncated 1437 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Apply `safe_save_path()` to every processed asset before saving: ```python output_path = safe_save_path(os.path.join(OUT, name + ".png")) rgba.save(output_path) ``` Because later code currently reopens fixed filenames, retain the resolved output paths: ```python generated = {} def process(name): # Existing processing logic... output_path = safe_save_path(os.path.join(OUT, name + ".png")) rgba.save(output_path) generated[name] = output_path return output_path for n in NAMES: process(n) tiles = [ (name, Image.open(generated[name]).convert("RGBA")) for name in NAMES ] ``` Additional hardening should include: 1. Resolve `CUTOUT_OUT` to an absolute path and display it before processing. 2. Add an explicit opt-in overwrite flag if replacement is ever required. 3. Refuse to overwrite by default using exclusive file creation or a pre-save existence check. 4. Create a unique per-run output directory to keep each processing run isolated. 5. Add automated tests verifying that two consecutive runs preserve the first run's files. 6. Update documentation so its non-overwrite guarantee matches actual behavior. ]]>