Skip to content

S3 DeleteObjects Key Validation Fix #3832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

S3 DeleteObjects Key Validation Fix #3832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions generator/.DevConfigs/a5cb7525-8e50-4583-ae7b-36180442d7e3.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"services": [
{
"serviceName": "S3",
"type": "patch",
"changeLogMessages": [
"Fixed a bug in DeleteObjects where valid objects would not be deleted when the request included invalid keys"
]
}
]
}
91 changes: 90 additions & 1 deletion sdk/src/Services/S3/Custom/Model/DeleteObjectsRequest.Extensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
Expand All @@ -20,6 +20,7 @@

using Amazon.Runtime;
using Amazon.Runtime.Internal;
using Amazon.Runtime.Internal.Util;

namespace Amazon.S3.Model
{
Expand All @@ -30,13 +31,69 @@ namespace Amazon.S3.Model
/// </summary>
public partial class DeleteObjectsRequest : AmazonWebServiceRequest
{
private List<DeleteError> validationErrors;

/// <summary>
/// Gets the list of keys that were skipped during client-side validation.
/// </summary>
public List<DeleteError> ValidationErrors
{
get
{
if (validationErrors == null)
{
validationErrors = new List<DeleteError>();
}
return validationErrors;
}
}

private static Logger Logger
{
get
{
return Logger.GetLogger(typeof(DeleteObjectsRequest));
}
}

/// <summary>
/// Add a key to the set of keys of objects to be deleted.
/// </summary>
/// <param name="key">Object key</param>
/// <remarks>
/// Invalid keys (null, empty, or longer than 1024 characters) are skipped with an INFO-level log message.
/// These validation errors are also tracked in the ValidationErrors collection.
/// This allows the DeleteObjects operation to proceed with valid keys even when some keys are invalid.
/// </remarks>
public void AddKey(string key)
{
// Skip invalid keys instead of throwing exceptions
// This allows the DeleteObjects operation to proceed with valid keys
if (string.IsNullOrEmpty(key))
{
string message = "Skipping null or empty key in DeleteObjects request";
Logger.InfoFormat(message);

ValidationErrors.Add(new DeleteError {
Key = key ?? "(null)",
Code = "InvalidKey",
Message = "Key cannot be null or empty"
});
return;
}
if (key.Length > 1024)
Copy link

@grsw92 grsw92 May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexDaines I understand this change just skips invalid keys and just sends valid ones with the request to the server. I'm wondering shouldn't this behavior be handled on the server side and proper DeleteObjectsResponse be returned from the server in that case?

{
string message = string.Format("Skipping key in DeleteObjects request: key length {0} exceeds maximum of 1024 characters", key.Length);
Logger.InfoFormat(message);

ValidationErrors.Add(new DeleteError {
Key = key.Substring(0, Math.Min(key.Length, 128)) + (key.Length > 128 ? "..." : ""),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Min here is not truly required, also does it make sense to trim it it 128 when 1024 length is legit it can be issue when you have common prefix of 128 chars length

Code = "KeyTooLongError",
Message = string.Format("Key length {0} exceeds maximum of 1024 characters", key.Length)
});
return;
}

AddKey(new KeyVersion { Key = key });
}

Expand All @@ -45,8 +102,40 @@ public void AddKey(string key)
/// </summary>
/// <param name="key">Key of the object to be deleted.</param>
/// <param name="version">Version of the object to be deleted.</param>
/// <remarks>
/// Invalid keys (null, empty, or longer than 1024 characters) are skipped with an INFO-level log message.
/// These validation errors are also tracked in the ValidationErrors collection.
/// This allows the DeleteObjects operation to proceed with valid keys even when some keys are invalid.
/// </remarks>
public void AddKey(string key, string version)
{
// Skip invalid keys instead of throwing exceptions
// This allows the DeleteObjects operation to proceed with valid keys
if (string.IsNullOrEmpty(key))
{
string message = "Skipping null or empty key in DeleteObjects request";
Logger.InfoFormat(message);

ValidationErrors.Add(new DeleteError {
Key = key ?? "(null)",
Code = "InvalidKey",
Message = "Key cannot be null or empty"
});
return;
}
if (key.Length > 1024)
{
string message = string.Format("Skipping key in DeleteObjects request: key length {0} exceeds maximum of 1024 characters", key.Length);
Logger.InfoFormat(message);

ValidationErrors.Add(new DeleteError {
Key = key.Substring(0, Math.Min(key.Length, 128)) + (key.Length > 128 ? "..." : ""),
Code = "KeyTooLongError",
Message = string.Format("Key length {0} exceeds maximum of 1024 characters", key.Length)
});
return;
}

AddKey(new KeyVersion { Key = key, VersionId = version });
}

Expand Down
68 changes: 42 additions & 26 deletions ...rvices/S3/Custom/Model/Internal/MarshallTransformations/DeleteObjectsRequestMarshaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,36 +68,52 @@ public IRequest Marshall(DeleteObjectsRequest deleteObjectsRequest)
xmlWriter.WriteStartElement("Delete", S3Constants.S3RequestXmlNamespace);

var deleteDeleteobjectsList = deleteObjectsRequest.Objects;
if (deleteDeleteobjectsList != null && deleteDeleteobjectsList.Count > 0)
if (deleteDeleteobjectsList == null || deleteDeleteobjectsList.Count == 0)
{
foreach (var deleteDeleteobjectsListValue in deleteDeleteobjectsList)
if (deleteObjectsRequest.ValidationErrors?.Count > 0)
{
xmlWriter.WriteStartElement("Object", "");
if (deleteDeleteobjectsListValue.IsSetKey())
{
xmlWriter.WriteElementString("Key", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.Key));
}
if (deleteDeleteobjectsListValue.IsSetVersionId())
{
xmlWriter.WriteElementString("VersionId", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.VersionId));
}

if (deleteDeleteobjectsListValue.IsSetETag())
{
xmlWriter.WriteElementString("ETag", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.ETag));
}
if (deleteDeleteobjectsListValue.IsSetLastModifiedTime())
{
xmlWriter.WriteElementString("LastModifiedTime", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.LastModifiedTime.Value));
}
if (deleteDeleteobjectsListValue.IsSetSize())
{
xmlWriter.WriteElementString("Size", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.Size.Value));
}

xmlWriter.WriteEndElement();
// All keys were invalid - provide clear error with validation details
throw new AmazonS3Exception(
$"DeleteObjects operation failed: All {deleteObjectsRequest.ValidationErrors.Count} keys were invalid. See ValidationErrors for details."
);
}
else
{
// No keys were provided
throw new AmazonS3Exception(
"DeleteObjects operation requires at least one valid key, but none were provided."
);
}
}

foreach (var deleteDeleteobjectsListValue in deleteDeleteobjectsList)
{
xmlWriter.WriteStartElement("Object", "");
if (deleteDeleteobjectsListValue.IsSetKey())
{
xmlWriter.WriteElementString("Key", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.Key));
}
if (deleteDeleteobjectsListValue.IsSetVersionId())
{
xmlWriter.WriteElementString("VersionId", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.VersionId));
}

if (deleteDeleteobjectsListValue.IsSetETag())
{
xmlWriter.WriteElementString("ETag", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.ETag));
}
if (deleteDeleteobjectsListValue.IsSetLastModifiedTime())
{
xmlWriter.WriteElementString("LastModifiedTime", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.LastModifiedTime.Value));
}
if (deleteDeleteobjectsListValue.IsSetSize())
{
xmlWriter.WriteElementString("Size", "", S3Transforms.ToXmlStringValue(deleteDeleteobjectsListValue.Size.Value));
}

xmlWriter.WriteEndElement();
}

if (deleteObjectsRequest.IsSetQuiet())
{
xmlWriter.WriteElementString("Quiet", "", S3Transforms.ToXmlStringValue(deleteObjectsRequest.Quiet.Value));
Expand Down