Port of the upstream "Secure and harden FileArtifactService against tampered
metadata and partial writes". Four defects, all in the file service:
The payload location was taken from `canonicalUri` in the on-disk metadata
document whenever the payload file itself was absent. That document lives
inside the artifact tree, so anything able to write there, or to win the race
between a delete and a load, could redirect a read to any file the process
could open. The payload location is now derived only from the storage layout,
and the `canonical_uri` returned to callers is recomputed from that layout
rather than read back from the document.
Saving an artifact named `metadata.json` destroyed it. The payload is stored
under the artifact directory's own name, so it was written first and then
overwritten by the metadata document, leaving a version directory holding
only metadata. Filenames are model-supplied, so this needed no attacker.
The name is now rejected at save time, caselessly, because a
case-insensitive filesystem resolves `Metadata.json` to the same file. The
rejection is on the save path only, so an artifact already stored under that
name stays readable and deletable.
A save that failed partway left the version directory behind, and a version
with a payload but no metadata reads as valid. Serializing `custom_metadata`
is caller-driven and can fail, which was enough to produce one. The whole
version directory is now removed if any step fails.
The metadata document was written in place with `write_text`, so a reader
could see a truncated document. It is now written to a temporary file in the
same directory and renamed over the destination. `tempfile.mkstemp` hardcodes
mode 0600 and `os.replace` carries that mode across, so the mode a normally
created file would get from the umask is restored first; otherwise the
metadata document and the payload beside it end up readable by different
principals.
Behaviour changes an existing 1.x user would notice:
- Saving an artifact named `metadata.json` in any casing now raises
InputValidationError. It previously succeeded and silently destroyed the
artifact it had just written.
- A save that fails partway now leaves nothing behind, where it previously
left a version directory that `list_versions` reported.
- A metadata document naming a `canonicalUri` outside the artifact tree is
ignored rather than followed, so an artifact whose payload is missing now
loads as None.
`_umask_derived_file_mode()` calls `os.umask` twice at import time, which is
a process-global mutation. It is momentary, and sampling per write would race
against concurrent writers instead.
The upstream commit also threads `inline_data.display_name` through the save
and load paths and guards `inline_data.data is None`. Neither is ported:
both come from separate upstream changes that are not on this branch.
George Weale
9d6d555d7a