Skip to content

CSV filename validator: side-effectful CWD default and check-then-write race (TOCTOU) #43

Description

@MarketDataApp

Where

UserUniversalAPIParams.validate_filename and write_file in src/marketdata/input_types/base.py, exercised on every request via _validate_user_universal_params in src/marketdata/resources/base.py (which sets result_data["filename"] = None on each call specifically to force the validator to run).

Problem A — validation has filesystem side effects, defaulting to CWD

@field_validator("filename")
def validate_filename(cls, file_path: str | Path | None) -> Path:
    if file_path is None:
        Path("output").mkdir(parents=True, exist_ok=True)
        file_path = Path(f"output/{datetime.datetime.now().strftime('%Y%m%d_%H%M%S_%f')}.csv")
  1. A Pydantic validator mutates the filesystem. mkdir("output") runs during validation — including for requests that never write a file (JSON/DataFrame/internal output formats), because the validator is re-triggered on every request.
  2. The location is relative to CWD. Wherever the consumer's process runs — a systemd service with CWD /, a cron job in $HOME, a container with a read-only filesystem — the SDK silently creates an output/ directory there. Consequences:
    • Market data gets written to disk in a location the consumer never chose (mild data-spill concern).
    • On read-only filesystems the mkdir fails with a confusing error on requests that had nothing to do with CSV output.

Problem B — TOCTOU between the exists-check and the write

    if file_path.exists():
        raise ValueError(f"Filename already exists: {file_path}")   # check
    ...

def write_file(self, content: str) -> str:
    self.filename.write_text(content)                               # use — no re-check

The exists() check runs at validation time, before the HTTP request is sent; the write happens after the response arrives — hundreds of milliseconds later. In that window:

  • Silent overwrite race: two threads/processes can validate the same path (both pass the exists-check) and both write. The "never overwrite an existing file" guarantee the validator advertises is not actually enforced; last writer wins, silently.
  • Symlink attack (low severity): in a shared/world-writable directory, a local attacker who can predict the path can place a symlink at it after validation. write_text follows symlinks, so the SDK overwrites the symlink's target with API-response content. Requires a local attacker and a predictable path (the timestamped default has microsecond granularity, so mainly relevant for caller-chosen filenames), but it is the classic check-then-write hole.

More checks cannot fix B — the fix is an atomic exclusive write.

Proposed fix (needs maintainer sign-off)

Keep the ./output/ default (deliberate DX choice, consistent with the SDK family) and keep the validator's early checks as a UX courtesy, but:

  1. Move the mkdir out of the validator into write_file so the filesystem is only touched when a file is genuinely written.
  2. Switch write_file to exclusive createopen(path, "x") (O_CREAT|O_EXCL) — so the file is created atomically and the write fails if the path appeared (or was symlinked) between validation and write.

Why this is an issue instead of a PR

Per SECURITY.md this is Tier 2 — every remedy changes observable behavior:

  • Moving the mkdir changes when errors surface (validation-time ValueError → write-time error) and stops creating output/ on non-CSV requests (someone could conceivably depend on that).
  • Exclusive-create changes the race's failure mode from silent overwrite to FileExistsError — which is arguably what the existing exists-check already promises, but it is still a behavior change.

Found during the security sweep that produced #41; related discussion in #42.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions