Skip to content

fix: CaptureFeedback now supports multiple attachments correctly #5077

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

Merged
jamescrosswell merged 3 commits into from
Mar 30, 2026
+43 −16
Merged

fix: CaptureFeedback now supports multiple attachments correctly #5077

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89d2c09
add all attachments
bitsandfoxes Mar 27, 2026
013cf7f
updated test
bitsandfoxes Mar 27, 2026
8a2c0fb
feedback
bitsandfoxes Mar 27, 2026
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
7 changes: 2 additions & 5 deletions src/Sentry/Protocol/Envelopes/Envelope.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,10 @@

if (attachments is { Count: > 0 })
{
if (attachments.Count > 1)
foreach (var attachment in attachments)
{
logger?.LogWarning("Feedback can only contain one attachment. Discarding {0} additional attachments.",
attachments.Count - 1);
AddEnvelopeItemFromAttachment(items, attachment, logger);
}

Check warning on line 319 in src/Sentry/Protocol/Envelopes/Envelope.cs

View check run for this annotation

@sentry/warden / warden: code-review

Missing null attachment safety check in FromFeedback 

The `FromFeedback` method iterates over attachments without checking if individual attachments are null, unlike the `FromEvent` method which includes `if (attachment.IsNull())` with the comment 'Safety check, in case the user forcefully added a null attachment.' If a null attachment is passed, `AddEnvelopeItemFromAttachment` will throw a NullReferenceException when accessing `attachment.Content.GetStream()`, and the catch block will also fail when logging `attachment.FileName`.
Comment thread
sentry-warden[bot] marked this conversation as resolved.

AddEnvelopeItemFromAttachment(items, attachments.First(), logger);
}

if (sessionUpdate is not null)
Expand Down
14 changes: 3 additions & 11 deletions test/Sentry.Tests/Protocol/Envelopes/EnvelopeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ public void FromFeedback_NoFeedbackContext_Throws()
}

[Fact]
public void FromFeedback_MultipleAttachments_LogsWarning()
public void FromFeedback_MultipleAttachments_AddsAll()
{
// Arrange
var feedback = new SentryFeedback(
Expand All @@ -864,23 +864,15 @@ public void FromFeedback_MultipleAttachments_LogsWarning()
Feedback = feedback
}
};
var logger = Substitute.For<IDiagnosticLogger>();
logger.IsEnabled(Arg.Any<SentryLevel>()).Returns(true);

List<SentryAttachment> attachments = [
AttachmentHelper.FakeAttachment("file1.txt"), AttachmentHelper.FakeAttachment("file2.txt")
];

// Act
using var envelope = Envelope.FromFeedback(evt, logger, attachments);
using var envelope = Envelope.FromFeedback(evt, attachments: attachments);

// Assert
logger.Received(1).Log(
SentryLevel.Warning,
Arg.Is<string>(m => m.Contains("Feedback can only contain one attachment")),
null,
Arg.Any<object[]>());
envelope.Items.Should().ContainSingle(item => item.TryGetType() == EnvelopeItem.TypeValueAttachment);
envelope.Items.Count(item => item.TryGetType() == EnvelopeItem.TypeValueAttachment).Should().Be(2);
}

[Fact]
Expand Down
Loading