Allowlist snapshot storage location path segments by aramase · Pull Request #2784 · agent-substrate/substrate

Anish Ramasekar (aramase) · 2026-10-16T23:37:39Z

Snapshots are stored under <location>/atespaces/<atespace>/{actors,tags}/<uid>. A storageLocation that already contained that layout (for example gs://snaps/root/atespaces/team-a/actors/<uid>) put one template's snapshots inside another actor's prefix, grant and cleanup.

Where a location is set (template create, --snapshot-location, ate-setup deploy), each path segment may now contain only ASCII letters, digits, _ and -, and may not be atespaces in any letter case. The bucket is not restricted.

Creating a tag checks the template's location against the same rules and returns FAILED_PRECONDITION on a bad one.
Parsing of recorded snapshot URIs stays lenient, so stored snapshots still resume and get collected.

Breaking change: if the path of your --snapshot-location (SNAPSHOT_LOCATION) has a segment outside ^[A-Za-z0-9_-]+$ (a dot, %, an empty segment, non-ASCII) or equal to atespaces in any case, change it before upgrading. Otherwise ate-api-server refuses to start.

Tests pass (touched packages, hack/verify-all.sh)
Docs updated

AI use: I used Claude Code to implement and review this change, and reviewed the diff myself.

Anish Ramasekar (aramase) · 2026-10-16T23:40:20Z

cc ILLUM1N0X

Abhishek Sharma (abhisheksharma2411)

The layering reads well, and I like that prefix and actorSnapshotURI keep the unvalidated path so recorded URIs still parse and anything already stored under a bad location stays deletable. That's the part that's easy to get wrong when tightening a validator.

I had a poke at the segment matcher. Everything I expected it to catch, it caught: case, surrounding dots, surrounding spaces, %61tespaces, atespaces%2Fteam-a, and a/../atespaces/x are all rejected.

One gap. The trim set is ". ", so non-ASCII spaces go straight through:

gs://b/root/atespaces\u00a0/x   -> allowed   (no-break space)
gs://b/root/atespaces\u2000/x   -> allowed   (en quad)
gs://b/root/atespaces\u3000/x   -> allowed   (ideographic space)

Tab and newline do get rejected, but only because net/url refuses control characters - not by this rule.

Since the doc comment says the trimming is there so "a backend that folds or trims names cannot turn a near-match into the real one", would unicode.IsSpace be a better trim predicate than the literal ". "? Low stakes on GCS, where names are byte-literal, so it needs a backend that normalises - it's just the same class the trim already defends against.

Greg Castle (destijl) · 2026-10-23T00:59:08Z

Thanks, this should go in before GA.

ILLUM1N0X

This needs a rebase. Main moved the proto to api/ateapi/v1 and renamed ateapipb to ateapi, so the proto comment and the tests will conflict.

On GCS, main sends every snapshot transfer through the exchange, so an actor whose snapshot is under a bad location can't resume after this change, because actorscope.FromActor now fails for it. That includes an actor that only borrows the old template's golden snapshot, so repointing it as in step 2 doesn't help either. Could you update the description and migration steps, and return FailedPrecondition from the exchange instead of Internal?

ILLUM1N0X · 2026-10-23T02:15:17Z

+		return nil, fmt.Errorf(`invalid snapshot location %q: must not contain '\'`, location)
 	}
 	for segment := range strings.SplitSeq(u.Path, "/") {
 		if segment != "" && strings.Trim(segment, ". ") == "" {


This check still trims only ASCII spaces, so a segment that is just a no-break space passes. Could it use isDotOrSpace like the atespaces check?

I think this is right as is. parseStorageURI is called on the garbage collection / reading path (ParseSnapshotURI, ParseStoragePrefix, SnapshotOwner.Prefix). If you tighten parseStorageURI to reject Unicode spaces or other non-ASCII characters, any historical snapshot created under a previously legal location that contained non-ASCII characters could never be deleted or collected by the GC workflow. The new restrictions make sure new bad paths can't be set.

ILLUM1N0X · 2026-10-23T02:15:17Z

+// TestParseSnapshotURIUnderAtespacesLocation covers a URI recorded under a
+// location ValidateSnapshotLocation rejects: it is read and its owner's
+// prefix can be collected, but nothing new is built under that location.
+func TestParseSnapshotURIUnderAtespacesLocation(t *testing.T) {


Most of this is already covered by rows in TestParseSnapshotURI, TestNewActorSnapshotURI and TestSnapshotOwnerPrefix. Could the owner prefix check be a wantOwnerPrefix field in TestParseSnapshotURI instead?

Anish Ramasekar (aramase) · 2026-10-23T06:26:21Z

ILLUM1N0X PTAL!

cc Greg Castle (@destijl)

Julian Gutierrez Oschmann (juli4n) · 2026-10-23T19:55:28Z

+		if segment != "" && trimmed == "" {
+			return fmt.Errorf("invalid snapshot location %q: %w: must not contain path segments of only dots and white space", location, ErrInvalidLocationSegment)
+		}
+		if strings.EqualFold(trimmed, atespacesPathSegment) {


I was expecting this PR to be just this line. IIUC, one of the reasons this PR got larger is that ParseStoragePrefix calls ValidateSnapshotLocation, but that seems wrong to me as both are different things? I'm not even convinced StoragePrefix is an abstraction we need to formalize either TBH.

WRT unit tests, I don't think we need to test this at every single layer? I think we should have a test that asserts that ValidateCreateActorTemplateRequest returns an error, which you already have.

I kept the new check in ValidateSnapshotLocation only. ParseStoragePrefix and SnapshotOwner.Prefix check just the URI shape again, so stored snapshots still parse and get cleaned up, and the per-layer tests are gone: the rule is tested in validate_test.go and at template create in actor_template_test.go.

One behavior change follows from checkTagCreatable already calling ValidateSnapshotLocation: creating a tag from a template stored with such a location now fails with FAILED_PRECONDITION, so the existing CreateTag row expects that. Suspend and resume work as on main.

Anish Ramasekar (aramase) · 2026-10-25T23:03:32Z

Julian Gutierrez Oschmann (@juli4n) Greg Castle (@destijl) this is down to the check in ValidateSnapshotLocation plus its tests in validate_test.go and actor_template_test.go (see the reply above). It has one approval and still needs a second. Could one of you take another look before the M3 freeze?

Greg Castle (destijl) · 2026-10-27T21:28:57Z

+	if err != nil {
+		return err
+	}
+	for segment := range strings.SplitSeq(u.Path, "/") {


This denylists near-matches of atespaces, so it has to predict every normalization a backend might apply, and other dot- or space-bearing names still reach the backend. Could we allowlist segments instead (for example ^[A-Za-z0-9_-]+$, plus the case-insensitive atespaces check) where a location is set? That removes the Unicode trimming and most of the test matrix. parseStorageURI should stay lenient so recorded URIs can still be collected.

Agreed, switched to an allowlist. Where a location is set (template create, ate-api-server's --snapshot-location and ate-setup deploy), each path segment now has to match ^[A-Za-z0-9_-]+$ and can't be atespaces in any case. Templates that are already stored keep the narrower atespaces check, so creating a tag from one with, say, a dotted location doesn't start failing. parseStorageURI, ParseStoragePrefix and SnapshotOwner.Prefix stay lenient, so recorded URIs still parse and get collected. The Unicode trimming and its test rows are gone.

Greg Castle (destijl)

Thanks, this looks good to me. CI's controlapi build fails on the new TestTagActorSnapshot_StoredLocationOutsideSegmentRules: ateapi.SnapshotStorageStatus_SNAPSHOT_STORAGE_STATUS_COMPLETED no longer exists on main, so it needs a rebase and a test update.

Greg Castle (destijl) · 2026-10-28T18:04:20Z

+		return nil, fmt.Errorf(`invalid snapshot location %q: must not contain '\'`, location)
 	}
 	for segment := range strings.SplitSeq(u.Path, "/") {
 		if segment != "" && strings.Trim(segment, ". ") == "" {


I think this is right as is. parseStorageURI is called on the garbage collection / reading path (ParseSnapshotURI, ParseStoragePrefix, SnapshotOwner.Prefix). If you tighten parseStorageURI to reject Unicode spaces or other non-ASCII characters, any historical snapshot created under a previously legal location that contained non-ASCII characters could never be deleted or collected by the GC workflow. The new restrictions make sure new bad paths can't be set.

Greg Castle (destijl) · 2026-10-28T20:19:41Z

Adding Dmitry Berkovich (@dberkov) somewhat at random for the "needs code freeze merge approval" decision.

Snapshot objects live under <location>/atespaces/<atespace>/..., so a location that contains an atespaces segment could put one template's snapshots inside another actor's prefix, its grant and its cleanup. Where a location is set, each path segment may now contain only ASCII letters, digits, '_' and '-', and no segment may be atespaces in any letter case. Creating a tag checks the template's location against the same rules and fails with FAILED_PRECONDITION on a bad one. Signed-off-by: Anish Ramasekar <[email protected]>

Anish Ramasekar (aramase) · 2026-10-29T19:10:25Z

Dmitry Berkovich (@dberkov) this is ready for approval and merge.

Signed-off-by: Anish Ramasekar <[email protected]>

Anish Ramasekar (aramase) · 2026-10-30T04:21:09Z

Dmitry Berkovich (@dberkov) pushed changes to address your comments, PTAL!

Signed-off-by: Anish Ramasekar <[email protected]>

Anish Ramasekar (aramase) · 2026-11-01T17:37:52Z

Benjamin Elder (@BenTheElder) this merged after rc.1 was cut and isn't on release-1.0. It changes which snapshot storage paths are accepted (breaking-change), so I'd rather it ship in 1.0.0 than in a 1.0.x patch. Could it get a cherry-pick before the tag? Happy to open the cherry-pick PR if that's easier.

Snapshots are stored under `<location>/atespaces/<atespace>/{actors,tags}/<uid>`. A `storageLocation` that already contained that layout (for example `gs://snaps/root/atespaces/team-a/actors/<uid>`) put one template's snapshots inside another actor's prefix, grant and cleanup. Where a location is set (template create, `--snapshot-location`, `ate-setup deploy`), each path segment may now contain only ASCII letters, digits, `_` and `-`, and may not be `atespaces` in any letter case. The bucket is not restricted. - Creating a tag checks the template's location against the same rules and returns `FAILED_PRECONDITION` on a bad one. - Parsing of recorded snapshot URIs stays lenient, so stored snapshots still resume and get collected. Breaking change: if the path of your `--snapshot-location` (`SNAPSHOT_LOCATION`) has a segment outside `^[A-Za-z0-9_-]+$` (a dot, `%`, an empty segment, non-ASCII) or equal to `atespaces` in any case, change it before upgrading. Otherwise ate-api-server refuses to start. - [x] Tests pass (touched packages, `hack/verify-all.sh`) - [x] Docs updated AI use: I used Claude Code to implement and review this change, and reviewed the diff myself. --------- Signed-off-by: Anish Ramasekar <[email protected]> (cherry picked from commit c53f4b2) Signed-off-by: Eitan Yarmush <[email protected]>

Anish Ramasekar (aramase) marked this pull request as ready for review October 16, 2026 23:59

Abhishek Sharma (abhisheksharma2411) reviewed Oct 17, 2026

View reviewed changes

Bowei Du (bowei) added area/api User-facing API changes area/security Security related issue/pr area/storage kind/bug Something isn't working / bugfixes labels Oct 19, 2026

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 1095b79 to de1baf0 Compare October 20, 2026 03:10

ILLUM1N0X reviewed Oct 23, 2026

View reviewed changes

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from de1baf0 to eb9eb3b Compare October 23, 2026 04:44

Anish Ramasekar (aramase) requested a review from ILLUM1N0X October 23, 2026 06:26

Benjamin Elder (BenTheElder) added the breaking-change PRs with breaking changes label Oct 23, 2026

Julian Gutierrez Oschmann (juli4n) reviewed Oct 23, 2026

View reviewed changes

Anish Ramasekar (aramase) closed this Oct 23, 2026

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 2377124 to ad4e040 Compare October 23, 2026 23:19

Anish Ramasekar (aramase) reopened this Oct 23, 2026

ILLUM1N0X approved these changes Oct 24, 2026

View reviewed changes

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 487f51f to 0d9481d Compare October 26, 2026 22:02

Greg Castle (destijl) reviewed Oct 27, 2026

View reviewed changes

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 0d9481d to 62bde8c Compare October 28, 2026 03:07

Zheng Lammerts (zlammerts-svg) added this to the M3 milestone Oct 28, 2026

Zheng Lammerts (zlammerts-svg) assigned Greg Castle (destijl) Oct 28, 2026

Greg Castle (destijl) reviewed Oct 28, 2026

View reviewed changes

Greg Castle (destijl) requested a review from Dmitry Berkovich (dberkov) October 28, 2026 20:19

Julian Gutierrez Oschmann (juli4n) reviewed Oct 28, 2026

View reviewed changes

Comment thread internal/resources/validate.go Resolved

Greg Castle (destijl) approved these changes Oct 28, 2026

View reviewed changes

Greg Castle (destijl) assigned Julian Gutierrez Oschmann (juli4n) Oct 28, 2026

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 4e922e6 to 4514dd6 Compare October 29, 2026 02:45

Anish Ramasekar (aramase) force-pushed the aramase/i/snapshot_location_nesting branch from 4514dd6 to 4ae8693 Compare October 29, 2026 15:54

Dmitry Berkovich (dberkov) added the milestone-ack label Oct 29, 2026

Dmitry Berkovich (dberkov) assigned Dmitry Berkovich (dberkov) and Eitan Yarmush (EItanya) Oct 29, 2026

Julian Gutierrez Oschmann (juli4n) previously approved these changes Oct 29, 2026

View reviewed changes

Eitan Yarmush (EItanya) previously approved these changes Oct 29, 2026

View reviewed changes

Dmitry Berkovich (dberkov) reviewed Oct 30, 2026

View reviewed changes

Comment thread internal/resources/validate.go Resolved

Comment thread internal/resources/validate.go Resolved

Comment thread internal/resources/snapshot.go Resolved

fix(resources): reject trailing '?' and name storage prefix in errors

1ebfae7

Signed-off-by: Anish Ramasekar <[email protected]>

Anish Ramasekar (aramase) dismissed stale reviews from Eitan Yarmush (EItanya) and Julian Gutierrez Oschmann (juli4n) via 1ebfae7 October 30, 2026 04:07

Anish Ramasekar (aramase) requested review from Eitan Yarmush (EItanya), Dmitry Berkovich (dberkov) and Julian Gutierrez Oschmann (juli4n) October 30, 2026 04:20

Dmitry Berkovich (dberkov) reviewed Oct 30, 2026

View reviewed changes

Comment thread internal/resources/validate.go Resolved

refactor(resources): wrap parseStorageURI errors at the callers

af475f8

Signed-off-by: Anish Ramasekar <[email protected]>

Dmitry Berkovich (dberkov) approved these changes Oct 30, 2026

View reviewed changes

Dmitry Berkovich (dberkov) enabled auto-merge October 30, 2026 04:46

Dmitry Berkovich (dberkov) added this pull request to the merge queue Oct 30, 2026

Merged via the queue into agent-substrate:main with commit c53f4b2 Oct 30, 2026
7 checks passed

Anish Ramasekar (aramase) deleted the aramase/i/snapshot_location_nesting branch October 30, 2026 05:24

Eitan Yarmush (EItanya) mentioned this pull request Nov 2, 2026

[release-1.0] Cherry-pick 23 main changes through 4488a6667 #3536

Merged

1 of 2 tasks

Conversation

Anish Ramasekar (aramase) commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anish Ramasekar (aramase) commented Oct 16, 2026

Uh oh!

Abhishek Sharma (abhisheksharma2411) left a comment

Choose a reason for hiding this comment

Uh oh!

Greg Castle (destijl) commented Oct 23, 2026

Uh oh!

ILLUM1N0X left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anish Ramasekar (aramase) commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anish Ramasekar (aramase) commented Oct 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Greg Castle (destijl) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Greg Castle (destijl) commented Oct 28, 2026

Uh oh!

Uh oh!

Anish Ramasekar (aramase) commented Oct 29, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Anish Ramasekar (aramase) commented Oct 30, 2026

Uh oh!

Uh oh!

Uh oh!

Anish Ramasekar (aramase) commented Nov 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Anish Ramasekar (aramase) commented Oct 16, 2026 •

edited

Loading