Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,10 @@ async def retrieve(self, reference: str) -> tuple[bytes, str]:
"""Retrieve content from an S3 object.

Accepts both ``s3://`` URIs (as returned by ``store()``) and raw
S3 keys for backward compatibility.
S3 keys for backward compatibility. References are constrained to the
configured ``bucket`` and ``prefix``: a reference that resolves to a key
outside the prefix (or to a different bucket) is rejected, mirroring the
scope that ``store()`` enforces.

Args:
reference: The S3 URI or object key returned by store().
Expand All @@ -523,14 +526,17 @@ async def retrieve(self, reference: str) -> tuple[bytes, str]:
A tuple of (content bytes, content type).

Raises:
KeyError: If the object does not exist.
KeyError: If the object does not exist or the reference resolves
outside the configured bucket and prefix.
"""
s3_key = reference
if reference.startswith("s3://"):
expected_prefix = f"s3://{self._bucket}/"
if not reference.startswith(expected_prefix):
raise KeyError(f"Reference not found: {reference}")
s3_key = reference[len(expected_prefix) :]
if self._prefix and not s3_key.startswith(self._prefix):
raise KeyError(f"Reference not found: {reference}")
try:
response = self._client.get_object(Bucket=self._bucket, Key=s3_key)
content: bytes = response["Body"].read()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,30 @@ async def test_retrieve_rejects_wrong_bucket_uri(self, storage):
with pytest.raises(KeyError, match="Reference not found"):
await storage.retrieve("s3://wrong-bucket/artifacts/some_key")

@pytest.mark.asyncio
async def test_retrieve_rejects_raw_key_outside_prefix(self, storage, mock_s3_client):
# Regression for report 3858663: a raw key pointing outside the configured
# prefix must not be readable, even when the object exists in the same bucket.
mock_s3_client.put_object(Bucket="test-bucket", Key="private/secret.txt", Body=b"SECRET")
with pytest.raises(KeyError, match="Reference not found"):
await storage.retrieve("private/secret.txt")
Comment thread
opieter-aws marked this conversation as resolved.
Resolved

@pytest.mark.asyncio
async def test_retrieve_rejects_s3_uri_outside_prefix(self, storage, mock_s3_client):
# Regression for report 3858663: a same-bucket s3:// URI outside the configured
# prefix must be rejected, not silently read.
mock_s3_client.put_object(Bucket="test-bucket", Key="private/secret.txt", Body=b"SECRET")
with pytest.raises(KeyError, match="Reference not found"):
await storage.retrieve("s3://test-bucket/private/secret.txt")

@pytest.mark.asyncio
async def test_retrieve_rejects_sibling_prefix_key(self, storage, mock_s3_client):
# Regression for report 3858663: the trailing-slash normalization must make
# prefix "artifacts/" reject "artifactsX/...", not just fully-disjoint keys.
mock_s3_client.put_object(Bucket="test-bucket", Key="artifactsX/secret.txt", Body=b"SECRET")
with pytest.raises(KeyError, match="Reference not found"):
await storage.retrieve("artifactsX/secret.txt")

@pytest.mark.asyncio
async def test_put_object_called_with_correct_params(self, storage, mock_s3_client):
await storage.store("key_1", b"test content", "application/json")
Expand All @@ -462,7 +486,7 @@ async def test_non_nosuchkey_error_propagates(self, storage, mock_s3_client):
mock_s3_client.get_object.side_effect = ClientError(error_response, "GetObject")

with pytest.raises(ClientError, match="Forbidden"):
await storage.retrieve("some_key")
await storage.retrieve("artifacts/some_key")


class TestFileStorageWithSandbox:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,68 @@ describe('S3Storage', () => {
expect(command.input.Key).toBe('some/raw/key')
})

it('round-trips a stored reference under a configured prefix', async () => {
mockSend.mockResolvedValueOnce({}).mockResolvedValueOnce({
Body: { transformToByteArray: () => Promise.resolve(new TextEncoder().encode('hello')) },
ContentType: 'text/plain',
})

const storage = new S3Storage('b', { prefix: 'artifacts', s3Client: mockS3Client as never })
const ref = await storage.store('key1', new TextEncoder().encode('hello'))
const result = await storage.retrieve(ref)

expect(new TextDecoder().decode(result.content)).toBe('hello')
})

it('throws on bucket mismatch', async () => {
const storage = new S3Storage('my-bucket', { s3Client: mockS3Client as never })

await expect(storage.retrieve('s3://wrong-bucket/key')).rejects.toThrow('bucket mismatch')
})

it('rejects a raw key outside the configured prefix', async () => {
// Regression for report 3858663: a raw key outside the configured prefix
// must be rejected without reading the object, even in the same bucket.
const storage = new S3Storage('b', { prefix: 'artifacts', s3Client: mockS3Client as never })

await expect(storage.retrieve('private/secret.txt')).rejects.toThrow('Reference not found')
expect(mockSend).not.toHaveBeenCalled()
})

it('rejects a same-bucket s3:// URI outside the configured prefix', async () => {
// Regression for report 3858663: a same-bucket URI outside the configured
// prefix must be rejected, not silently read.
const storage = new S3Storage('b', { prefix: 'artifacts', s3Client: mockS3Client as never })

await expect(storage.retrieve('s3://b/private/secret.txt')).rejects.toThrow('Reference not found')
expect(mockSend).not.toHaveBeenCalled()
})

it('rejects a sibling-prefix key that shares a string prefix but not a path segment', async () => {
// Regression for report 3858663: the trailing-slash normalization must make
// prefix 'artifacts/' reject 'artifactsX/...', not just fully-disjoint keys.
const storage = new S3Storage('b', { prefix: 'artifacts', s3Client: mockS3Client as never })

await expect(storage.retrieve('artifactsX/secret.txt')).rejects.toThrow('Reference not found')
expect(mockSend).not.toHaveBeenCalled()
})

it('normalizes a leading-slash prefix so in-prefix raw keys are accepted', async () => {
mockSend.mockResolvedValue({
Body: { transformToByteArray: () => Promise.resolve(new TextEncoder().encode('hello')) },
ContentType: 'text/plain',
})

// A leading slash on the prefix must be stripped (parity with Python's strip("/")),
// so a raw key under the normalized prefix still resolves.
const storage = new S3Storage('b', { prefix: '/artifacts', s3Client: mockS3Client as never })
const result = await storage.retrieve('artifacts/foo')

expect(new TextDecoder().decode(result.content)).toBe('hello')
const command = mockSend.mock.calls[0]![0]
expect(command.input.Key).toBe('artifacts/foo')
})

it('throws on NoSuchKey error', async () => {
const noSuchKey = new Error('not found')
noSuchKey.name = 'NoSuchKey'
Expand Down
25 changes: 18 additions & 7 deletions strands-ts/src/vended-plugins/context-offloader/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,22 @@ function sanitizeId(rawId: string): string {
}

/**
* Remove any trailing `/` characters from a path-like string.
* Remove any leading and trailing `/` characters from a path-like string.
*
* Implemented as a linear scan rather than a `/\/+$/` replacement so that
* inputs with long runs of slashes are handled in linear time.
* Matches Python's `prefix.strip("/")` so the two SDKs normalize an S3 prefix
* identically. Implemented as a linear scan rather than a `/^\/+|\/+$/g`
* replacement so that inputs with long runs of slashes are handled in linear time.
*/
function stripTrailingSlashes(value: string): string {
function stripSurroundingSlashes(value: string): string {
let start = 0
let end = value.length
while (end > 0 && value[end - 1] === '/') {
while (start < end && value[start] === '/') {
start++
}
while (end > start && value[end - 1] === '/') {
end--
}
return value.slice(0, end)
return value.slice(start, end)
}

/**
Expand Down Expand Up @@ -378,7 +383,8 @@ export class S3Storage implements Storage {
options?: { prefix?: string; region?: string; s3Client?: import('@aws-sdk/client-s3').S3Client }
) {
this._bucket = bucket
this._prefix = options?.prefix ? stripTrailingSlashes(options.prefix) + '/' : ''
const normalizedPrefix = options?.prefix ? stripSurroundingSlashes(options.prefix) : ''
Comment thread
opieter-aws marked this conversation as resolved.
Resolved
this._prefix = normalizedPrefix ? normalizedPrefix + '/' : ''
this._client = options?.s3Client
this._region = options?.region ?? 'us-east-1'
}
Expand Down Expand Up @@ -427,6 +433,11 @@ export class S3Storage implements Storage {
s3Key = uriMatch[2]
}

// Constrain references to the configured prefix, mirroring the scope store() enforces.
if (this._prefix && !s3Key.startsWith(this._prefix)) {
throw new Error(`Reference not found: ${reference}`)
}

try {
const response = await client.send(
new GetObjectCommand({
Expand Down
Loading