amadan

carlos / platform Public

runs: store helpers refuse an account or app the repo does not accept; key builders say what they trust

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 139429cec05b55f6dd74dbae9c6f9abd2dcfbecd parent f774b15
4 files changed, +71 −16
  • internal/runs/check.go +1 −0
  • internal/runs/runs.go +6 −1
  • internal/runs/runs_test.go +38 −0
  • internal/runs/store.go +26 −15

CI passed — run details

diff --git a/internal/runs/check.go b/internal/runs/check.go
index 52dfcda8e..607646205 100644
--- a/internal/runs/check.go
+++ b/internal/runs/check.go
@@ -9,6 +9,7 @@ import (
)
var (
+ ErrBadApp = errors.New("runs: not an account and app")
ErrBadID = errors.New("runs: not a run id")
ErrNoArgs = errors.New("runs: no arguments")
ErrTooManyArgs = errors.New("runs: more than 256 arguments")
diff --git a/internal/runs/runs.go b/internal/runs/runs.go
index db0f1f2f7..03f29b45a 100644
--- a/internal/runs/runs.go
+++ b/internal/runs/runs.go
@@ -103,10 +103,15 @@ func ResultKey(acct, app, id string) string { return Prefix(acct, app, id) + "r
func AuditedKey(acct, app, id string) string { return Prefix(acct, app, id) + "audited" }
func OutPrefix(acct, app, id string) string { return Prefix(acct, app, id) + "out/" }
+// InputKey is where an uploaded file lives. name must have passed PlainName
+// (CheckFiles does that); InputKey does not check it, and a name with a
+// slash or ".." would leave in/.
func InputKey(acct, app, id, name string) string { return Prefix(acct, app, id) + "in/" + name }
// ChunkKey is out/<seq>-<stream>, the seq zero-padded so keys sort in the
-// order the agent read the output.
+// order the agent read the output. stream must be StreamOut or StreamErr
+// (ChunkKey does not check it), and seq at most 99,999,999, the 8-digit
+// format; the 64 MB output cap in 256 KB chunks never gets near it.
func ChunkKey(acct, app, id string, seq int64, stream string) string {
return OutPrefix(acct, app, id) + fmt.Sprintf("%08d-%s", seq, stream)
}
diff --git a/internal/runs/runs_test.go b/internal/runs/runs_test.go
index f30acfafc..20b82e926 100644
--- a/internal/runs/runs_test.go
+++ b/internal/runs/runs_test.go
@@ -218,3 +218,41 @@ func TestStoreHelpersRefuseAnIdThatIsNotOne(t *testing.T) {
t.Fatalf("a refused delete touched the store: %q", left)
}
}
+
+// acct and app are concatenated into every key, so a slash or ".." in either
+// would leave the app's subtree. Nothing may be read or written for them.
+func TestStoreHelpersRefuseAnAccountOrAppThatIsNotOne(t *testing.T) {
+ ctx := context.Background()
+ dir := t.TempDir()
+ s := release.NewDirStore(dir)
+ id := "run_abcdefghijklmnopqrstuvwxyz"
+ victim := RequestKey("bab", "x", id)
+ if _, err := s.Put(ctx, victim, []byte(`{}`), ""); err != nil {
+ t.Fatal(err)
+ }
+ before, _ := s.List(ctx, "")
+ for _, c := range [][2]string{{"bab/titogo", "x"}, {"bab", "../x"}, {"bab", "../../y"}, {"", "titogo"}, {"bab", ""}, {"bab", "a/b"}, {"..", "x"}, {"BAB", "x"}, {"bab", "x\n"}} {
+ acct, app := c[0], c[1]
+ if _, err := ReadRequest(ctx, s, acct, app, id); !errors.Is(err, ErrBadApp) {
+ t.Errorf("ReadRequest(%q, %q) = %v", acct, app, err)
+ }
+ if _, err := ReadResult(ctx, s, acct, app, id); !errors.Is(err, ErrBadApp) {
+ t.Errorf("ReadResult(%q, %q) = %v", acct, app, err)
+ }
+ if err := WriteResult(ctx, s, acct, app, id, Result{}); !errors.Is(err, ErrBadApp) {
+ t.Errorf("WriteResult(%q, %q) = %v", acct, app, err)
+ }
+ if _, err := ListChunks(ctx, s, acct, app, id, 0, 0); !errors.Is(err, ErrBadApp) {
+ t.Errorf("ListChunks(%q, %q) = %v", acct, app, err)
+ }
+ if _, err := LastChunkSeq(ctx, s, acct, app, id); !errors.Is(err, ErrBadApp) {
+ t.Errorf("LastChunkSeq(%q, %q) = %v", acct, app, err)
+ }
+ if err := DeleteRun(ctx, s, acct, app, id); !errors.Is(err, ErrBadApp) {
+ t.Errorf("DeleteRun(%q, %q) = %v", acct, app, err)
+ }
+ }
+ if after, _ := s.List(ctx, ""); strings.Join(after, "\n") != strings.Join(before, "\n") {
+ t.Fatalf("a refused call changed the store: %q -> %q", before, after)
+ }
+}
diff --git a/internal/runs/store.go b/internal/runs/store.go
index 22d727f1c..139837bab 100644
--- a/internal/runs/store.go
+++ b/internal/runs/store.go
@@ -10,14 +10,25 @@ import (
"github.com/carlosframework/platform/internal/release"
)
-// Every helper refuses an id that is not a run id (ErrBadID) before it builds
-// a key, so no caller can reach another run's objects, or a whole app's runs,
-// through a crafted one.
+// checkRun is what every helper below does first. It refuses an account or
+// app that release does not accept (ErrBadApp) and an id that is not a run
+// id (ErrBadID) before it builds a key, so no caller can reach another run's objects, another app's runs, or
+// a key with a ".." in it, through a crafted value. Nothing is read or written
+// for a refused call.
+func checkRun(acct, app, id string) error {
+ if !release.ValidAccount(acct) || !release.ValidApp(app) {
+ return ErrBadApp
+ }
+ if !ValidID(id) {
+ return ErrBadID
+ }
+ return nil
+}
// ReadRequest reads request.json; absent is os.ErrNotExist.
func ReadRequest(ctx context.Context, s release.Store, acct, app, id string) (*Request, error) {
- if !ValidID(id) {
- return nil, ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return nil, err
}
raw, _, err := s.Get(ctx, RequestKey(acct, app, id))
if err != nil {
@@ -32,8 +43,8 @@ func ReadRequest(ctx context.Context, s release.Store, acct, app, id string) (*R
// ReadResult reads result.json; absent is (nil, nil).
func ReadResult(ctx context.Context, s release.Store, acct, app, id string) (*Result, error) {
- if !ValidID(id) {
- return nil, ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return nil, err
}
raw, _, err := s.Get(ctx, ResultKey(acct, app, id))
if errors.Is(err, os.ErrNotExist) {
@@ -52,8 +63,8 @@ func ReadResult(ctx context.Context, s release.Store, acct, app, id string) (*Re
// WriteResult writes result.json create-only: the first result wins, and a
// later writer (the sweep's lost result racing the box's) changes nothing.
func WriteResult(ctx context.Context, s release.Store, acct, app, id string, r Result) error {
- if !ValidID(id) {
- return ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return err
}
body, err := json.Marshal(r)
if err != nil {
@@ -67,8 +78,8 @@ func WriteResult(ctx context.Context, s release.Store, acct, app, id string, r R
// ListChunks lists chunk keys after seq, sorted, at most limit (0: all).
func ListChunks(ctx context.Context, s release.Store, acct, app, id string, after int64, limit int) ([]string, error) {
- if !ValidID(id) {
- return nil, ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return nil, err
}
prefix := OutPrefix(acct, app, id)
startAfter := prefix
@@ -98,8 +109,8 @@ func ListChunks(ctx context.Context, s release.Store, acct, app, id string, afte
// LastChunkSeq is the highest seq written, 0 when none.
func LastChunkSeq(ctx context.Context, s release.Store, acct, app, id string) (int64, error) {
- if !ValidID(id) {
- return 0, ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return 0, err
}
keys, err := ListChunks(ctx, s, acct, app, id, 0, 0)
if err != nil {
@@ -116,8 +127,8 @@ func LastChunkSeq(ctx context.Context, s release.Store, acct, app, id string) (i
// DeleteRun deletes every object of the run.
func DeleteRun(ctx context.Context, s release.Store, acct, app, id string) error {
- if !ValidID(id) {
- return ErrBadID
+ if err := checkRun(acct, app, id); err != nil {
+ return err
}
keys, err := s.List(ctx, Prefix(acct, app, id))
if err != nil {