Skip to content

fix(attachments): Serialize ID without hyphens #5501

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

Open
jjbayer wants to merge 3 commits into
base: master
Choose a base branch
from
+68 −18
Open

fix(attachments): Serialize ID without hyphens #5501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
def0021
wip
jjbayer Dec 19, 2025
ce383fa
fix(attachments): Write attachment ID without hyphens
jjbayer Dec 19, 2025
6ed804a
fix: cfg test
jjbayer Dec 19, 2025
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
55 changes: 53 additions & 2 deletions relay-event-schema/src/protocol/trace_attachment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, Value};
use std::fmt;
use std::ops::Deref;

use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, SkipSerialization, Value};
use serde::Serializer;

use crate::processor::ProcessValue;
use crate::protocol::{Attributes, Timestamp, TraceId};
Expand All @@ -14,7 +18,7 @@ pub struct TraceAttachmentMeta {

/// Unique identifier for this attachment.
#[metastructure(required = true, nonempty = true, trim = false)]
pub attachment_id: Annotated<Uuid>,
pub attachment_id: Annotated<AttachmentId>,

/// Timestamp when the attachment was created.
#[metastructure(required = true, trim = false)]
Expand All @@ -36,3 +40,50 @@ pub struct TraceAttachmentMeta {
#[metastructure(additional_properties, pii = "maybe")]
pub other: Object<Value>,
}

#[derive(Clone, Default, PartialEq, Empty, FromValue, ProcessValue)]
pub struct AttachmentId(Uuid);
Copy link
Member Author

@jjbayer jjbayer Dec 19, 2025

Choose a reason for hiding this comment

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

This was largely copied from TraceId.


impl AttachmentId {
/// Creates a new attachment ID. Only used in tests.
pub fn random() -> Self {
Self(Uuid::now_v7())
}
}

impl Deref for AttachmentId {
type Target = Uuid;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl fmt::Display for AttachmentId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0.as_simple())
}
}

impl fmt::Debug for AttachmentId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "AttachmentId(\"{}\")", self.0.as_simple())
}
}

impl IntoValue for AttachmentId {
fn into_value(self) -> Value
where
Self: Sized,
{
Value::String(self.to_string())
}

fn serialize_payload<S>(&self, s: S, _behavior: SkipSerialization) -> Result<S::Ok, S::Error>
where
Self: Sized,
S: Serializer,
{
s.collect_str(self)
}
}
7 changes: 3 additions & 4 deletions relay-server/src/processing/trace_attachments/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,10 @@ pub fn scrub_attachment<'a>(

#[cfg(test)]
mod tests {
use relay_event_schema::protocol::TraceAttachmentMeta;
use relay_event_schema::protocol::{AttachmentId, TraceAttachmentMeta};
use relay_pii::{DataScrubbingConfig, PiiConfig};
use relay_protocol::SerializableAnnotated;
use relay_sampling::evaluation::ReservoirCounters;
use uuid::Uuid;

use crate::envelope::ParentId;
use crate::services::projects::project::ProjectInfo;
Expand Down Expand Up @@ -217,7 +216,7 @@ mod tests {
let mut attachment = ExpandedAttachment {
parent_id: None,
meta: Annotated::new(TraceAttachmentMeta {
attachment_id: Annotated::new(Uuid::new_v4()),
attachment_id: Annotated::new(AttachmentId::random()),
filename: Annotated::new("data.txt".to_owned()),
content_type: Annotated::new("text/plain".to_owned()),
..Default::default()
Expand Down Expand Up @@ -253,7 +252,7 @@ mod tests {
let mut attachment = ExpandedAttachment {
parent_id: Some(ParentId::SpanId(None)),
meta: Annotated::new(TraceAttachmentMeta {
attachment_id: Annotated::new(Uuid::new_v4()),
attachment_id: Annotated::new(AttachmentId::random()),
filename: Annotated::new("data.txt".to_owned()),
content_type: Annotated::new("text/plain".to_owned()),
attributes: Annotated::new(attributes),
Expand Down
1 change: 1 addition & 0 deletions relay-server/src/services/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ impl UploadServiceInner {
let key = Uuid::from_slice(&trace_item.trace_item.item_id)
.map_err(Error::from)
.reject(&trace_item)?
.as_simple()
.to_string();

#[cfg(debug_assertions)]
Comment on lines 336 to 342
Copy link

@sentry sentry bot Dec 19, 2025

Choose a reason for hiding this comment

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

Bug: The code attempts to deserialize a 32-byte hex string UUID using Uuid::from_slice(), which expects a 16-byte binary slice, causing attachment uploads to fail.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The serialization of attachment_id was changed to a 32-byte UTF-8 hex string, but the deserialization logic was not updated. In upload.rs, the code attempts to deserialize trace_item.trace_item.item_id using Uuid::from_slice(), which expects a 16-byte raw binary slice. This mismatch causes a slice length error. The error is caught and the item is rejected, causing the attachment upload to fail silently. As a result, trace attachments are never stored or forwarded.

💡 Suggested Fix

Update the deserialization logic in upload.rs. Instead of using Uuid::from_slice() on the byte representation of the string, parse the hex string to a UUID. This can be done by converting the item_id byte slice to a string and then using Uuid::parse_str().

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: relay-server/src/services/upload.rs#L336-L342

Potential issue: The serialization of `attachment_id` was changed to a 32-byte UTF-8 hex
string, but the deserialization logic was not updated. In `upload.rs`, the code attempts
to deserialize `trace_item.trace_item.item_id` using `Uuid::from_slice()`, which expects
a 16-byte raw binary slice. This mismatch causes a slice length error. The error is
caught and the item is rejected, causing the attachment upload to fail silently. As a
result, trace attachments are never stored or forwarded.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7755060

Expand Down
23 changes: 11 additions & 12 deletions tests/integration/test_attachmentsv2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
def create_attachment_metadata():
return {
"trace_id": uuid.uuid4().hex,
"attachment_id": str(uuid.uuid4()),
"attachment_id": uuid.uuid4().hex,
Copy link
Member Author

@jjbayer jjbayer Dec 19, 2025

Choose a reason for hiding this comment

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

The input doesn't need to be hyphen-free, but it makes comparisons with the output easier.

"timestamp": 1760520026.781239,
"filename": "myfile.txt",
"content_type": "text/plain",
Expand All @@ -37,7 +37,6 @@ def create_attachment_metadata():
def create_attachment_envelope(project_config):
return Envelope(
headers={
"event_id": "515539018c9b4260a6f999572f1661ee",
Copy link
Member Author

@jjbayer jjbayer Dec 19, 2025

Choose a reason for hiding this comment

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

Drive-by fix: trace attachment envelopes should not have an event ID.

"trace": {
"trace_id": "5b8efff798038103d269b633813fc60c",
"public_key": project_config["publicKeys"][0]["publicKey"],
Expand Down Expand Up @@ -203,7 +202,7 @@ def test_standalone_attachment_store(
),
pytest.param(
{"meta_length": None},
273,
269,
Copy link
Member Author

@jjbayer jjbayer Dec 19, 2025

Choose a reason for hiding this comment

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

No more hyphens -> saved some chars.

"invalid_trace_attachment",
id="missing_meta_length",
),
Expand Down Expand Up @@ -341,9 +340,9 @@ def test_attachment_with_matching_span(mini_sentry, relay):
assert attachment.payload.bytes == combined_payload
assert attachment.headers == {
"type": "attachment",
"length": 260,
"length": 256,
"content_type": "application/vnd.sentry.trace-attachment",
"meta_length": 237,
"meta_length": 233,
"span_id": span_id,
}

Expand Down Expand Up @@ -544,9 +543,9 @@ def test_two_attachments_mapping_to_same_span(mini_sentry, relay):
assert item.payload.bytes == combined_payload
assert item.headers == {
"type": "attachment",
"length": 260,
"length": 256,
"content_type": "application/vnd.sentry.trace-attachment",
"meta_length": 237,
"meta_length": 233,
"span_id": span_id,
}

Expand Down Expand Up @@ -1256,7 +1255,7 @@ def test_attachment_default_pii_scrubbing_meta(
)

metadata = {
"attachment_id": str(uuid.uuid4()),
"attachment_id": uuid.uuid4().hex,
"timestamp": ts.timestamp(),
"filename": "data.txt",
"content_type": "text/plain",
Expand Down Expand Up @@ -1300,7 +1299,7 @@ def test_attachment_default_pii_scrubbing_meta(
metadata_part = json.loads(payload[:meta_length].decode("utf-8"))

assert metadata_part == {
"attachment_id": mock.ANY,
"attachment_id": metadata["attachment_id"],
"timestamp": time_within_delta(ts),
"filename": "data.txt",
"content_type": "text/plain",
Expand Down Expand Up @@ -1360,7 +1359,7 @@ def test_attachment_pii_scrubbing_meta_attribute(
)

metadata = {
"attachment_id": str(uuid.uuid4()),
"attachment_id": uuid.uuid4().hex,
"timestamp": ts.timestamp(),
"filename": "data.txt",
"content_type": "text/plain",
Expand Down Expand Up @@ -1394,7 +1393,7 @@ def test_attachment_pii_scrubbing_meta_attribute(
metadata_part = json.loads(payload[:meta_length].decode("utf-8"))

assert metadata_part == {
"attachment_id": mock.ANY,
"attachment_id": metadata["attachment_id"],
"timestamp": time_within_delta(ts),
"filename": "data.txt",
"content_type": "text/plain",
Expand Down Expand Up @@ -1451,7 +1450,7 @@ def test_attachment_pii_scrubbing_body(mini_sentry, relay):
)

metadata = {
"attachment_id": str(uuid.uuid4()),
"attachment_id": uuid.uuid4().hex,
"timestamp": ts.timestamp(),
"filename": "log.txt",
"content_type": "text/plain",
Expand Down
Loading