fix(artifacts): Port artifact reference scoping and path-segment validation to v1 - #6794
Open
GWeale wants to merge 3 commits into
Open
fix(artifacts): Port artifact reference scoping and path-segment validation to v1#6794GWeale wants to merge 3 commits into
GWeale wants to merge 3 commits into
Conversation
InMemoryArtifactService accepted a caller-supplied artifact:// reference and, on load, dereferenced whatever app, user and session were embedded in that URI without ever comparing them to the caller's own. A caller could store a reference naming another app or another user and read back bytes belonging to that scope. The service now validates a parsed reference against the caller's app, user and session, both before storing it on save and before following it on load. A reference that stays inside the same session still resolves, and a user-scoped reference (one with no session in the URI) is still readable from any session of the same user. Behaviour change: storing or loading a reference that names a different app, user or session now raises InputValidationError instead of silently resolving. On the artifact save endpoint that surfaces as HTTP 400. The GcsArtifactService half of the upstream change is not ported. On this branch that service raises NotImplementedError for file_data on save and never dereferences references on load, so it has no reference support to constrain. Port of the upstream fix to the v1 branch.
…ces (v1) InMemoryArtifactService and GcsArtifactService built their storage key by interpolating app_name, user_id and session_id straight into a string, with no check on any of them. An identifier that was empty, held a null byte, started with a separator, was drive-qualified, or contained a ".." segment produced an odd key rather than an error. A shared validate_path_segment now lives in artifact_util.py and both services call it before building a key, so the three artifact services agree on which identifiers are acceptable and a future path-backed backend inherits the check. The helper is copied from the upstream branch unchanged, including its isinstance(value, str) guards: callers outside the annotation do reach it, and the accompanying test pins that a non-string value passes rather than raising a TypeError. This is defence in depth, not a cross-tenant fix. Both services key on a flat string, so ".." never traverses anything, and an embedded separator remains accepted here exactly as it is upstream. FileArtifactService keeps its own stricter private validator, which also rejects embedded path separators. The upstream consolidation onto the shared helper is deliberately not ported, because on this branch it would relax that check and start accepting a user_id such as "has/slash" on the one service that writes real filesystem paths. Behaviour changes: an app_name, user_id or session_id that is empty, holds a null byte, starts with a separator, is drive-qualified, or contains a ".." segment now raises InputValidationError on the in-memory and GCS services. On the artifact save endpoint that is HTTP 400; the load, list, delete and version endpoints have no handler for it, so there it becomes HTTP 500. Adding those handlers is left out of this change. Separately, an empty-string session_id passed to FileArtifactService is now an error rather than being treated as no session at all. Port of two upstream commits to the v1 branch, landed as one helper so the weaker intermediate form is never on this branch.
…t filenames (v1) FileArtifactService decided whether a caller-supplied filename was safe by joining it under the scope root, resolving the result, and checking that the resolved path was still inside the root. Two shapes got through. A drive-qualified name such as "C:\Windows\evil.txt" was converted to the relative "C:/Windows/evil.txt", which is not absolute, so it passed the guard; on Windows, joining it replaces the drive and the write lands outside the root. And "folder/../alias.txt" resolved back inside the root and was accepted, so two different filenames addressed one artifact. _resolve_scoped_artifact_path now rejects rooted, drive-qualified and parent-referencing filenames up front, before any joining or resolving. The whole function is replaced rather than patched, which also brings in the Windows path normalization that landed upstream separately; the two are entangled in the same few lines and splitting them would leave a form that is on neither branch. The file service's own private identifier validator gains the same drive-qualified check. It rejected "C:/x" only as a side effect of its separator rule and accepted a bare "C:evil", which on Windows escapes the root directory by the same join. That guard is not in the upstream commit; upstream had already deleted this validator in favour of the shared one, which this branch deliberately does not do. Behaviour change: a filename such as "folder/../alias.txt" that resolves back inside the scope is now rejected rather than silently aliased to "alias.txt". Anyone relying on that aliasing gets an InputValidationError. Port of the upstream fix to the v1 branch.
GWeale
changed the title
xuanyang15
approved these changes
Aug 19, 2026
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
3 participants
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.
Ports three artifact-service fixes from
maintov1. Stored artifacts stay reachable; the input rules below have no flag and no opt-out, so a caller sending a rejected value changes the value.Scope artifact references to the caller (
f8631500)artifact://reference must name the caller's own app, user and session, checked on save and on load.user:references still resolve across sessions of the same user.v1.Validate path segments in the in-memory and GCS services (
45a77dc5, reland of8718aeffafter961f3e88)app_name,user_idandsession_idmust be non-empty, null-byte free, not rooted, not drive-qualified, and neither.,.., nor containing a..segment; otherwiseInputValidationError.FileArtifactServicekeeps its own stricter validator and treats an emptysession_idas an error.Reject rooted, drive-qualified and parent-referencing filenames (
2716ad55)C:...), or contain a..segment, on either platform's separators.FileArtifactServicealso rejects the drive-relativeC:nameform foruser_idandsession_id.Re-implemented rather than applied:
2716ad55.