Skip to content
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.

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

Remove authority from URLs sent to Sentry #2365

Merged
merged 21 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
03c0f27
Added a UrlPiiSanitizer that can be used to strip auth details from URLs
jamescrosswell May 11, 2023
36c04ff
Urls in the Message or Data of Breadcrumbs are now sanitized
jamescrosswell May 12, 2023
3ec26ff
Transaction Description sanitized when the transaction is captured by…
jamescrosswell May 12, 2023
98e826c
Moved URL redaction logic to SentryClient, just before envelope creation
jamescrosswell May 15, 2023
1bf388d
Renamed Sanitize to Redact in unit tests and comments for consistency
jamescrosswell May 15, 2023
3ad89e7
Transaction.Spans are now redacted before they are sent to Sentry
jamescrosswell May 15, 2023
4b5772d
Merge branch 'main' into feat/sanitize-urls-2078
jamescrosswell May 15, 2023
6fd5004
Update sentry-cocoa
jamescrosswell May 15, 2023
e3457f3
Update src/Sentry/SentryClient.cs
jamescrosswell May 16, 2023
7b15caf
Update CHANGELOG.md
jamescrosswell May 16, 2023
caadabb
Update src/Sentry/Internal/PiiExtensions.cs
jamescrosswell May 16, 2023
ee9c4b2
Update src/Sentry/Request.cs
jamescrosswell May 16, 2023
b3a0416
Fixed nullability errors after changing signature on RedactUrl extension
jamescrosswell May 16, 2023
52fe34a
Made Regex instances private static readonly
jamescrosswell May 16, 2023
80dc7b2
Removed unecessary null forgiving operator from Breadcrumb.Data
jamescrosswell May 16, 2023
d05f5bd
Strip UserInfo from the Request.Url before capturing Failed Requests
jamescrosswell May 17, 2023
3b6f4bb
Merge branch 'main' into feat/sanitize-urls-2078
mattjohnsonpint May 17, 2023
ab6d224
Undo submodule update
mattjohnsonpint May 17, 2023
1e2cf7e
Replaced bespoke code with Uri.GetComponents
jamescrosswell May 17, 2023
7e706af
Merge branch 'feat/sanitize-urls-2078' of https://github.com/getsentr…
jamescrosswell May 17, 2023
97f104a
Fixed missing arg in test
jamescrosswell May 17, 2023
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
### Features

- Add tag filters to SentryLoggingOptions ([#2360](https://github.com/getsentry/sentry-dotnet/pull/2360))
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
- Remove authority from URLs sent to Sentry ([#2366](https://github.com/getsentry/sentry-java/pull/2366))
- Remove authority from URLs sent to Sentry ([#2365](https://github.com/getsentry/sentry-dotnet/pull/2365))

## 3.31.0

Expand Down
25 changes: 23 additions & 2 deletions src/Sentry/Breadcrumb.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Sentry.Extensibility;
using Sentry.Internal;
using Sentry.Internal.Extensions;

namespace Sentry;
Expand All @@ -9,6 +10,12 @@ namespace Sentry;
[DebuggerDisplay("Message: {" + nameof(Message) + "}, Type: {" + nameof(Type) + "}")]
public sealed class Breadcrumb : IJsonSerializable
{
private readonly IReadOnlyDictionary<string, string>? _data;
private readonly string? _message;

private bool _sendDefaultPii = true;
internal void Redact() => _sendDefaultPii = false;

/// <summary>
/// A timestamp representing when the breadcrumb occurred.
/// </summary>
Expand All @@ -21,7 +28,11 @@ public sealed class Breadcrumb : IJsonSerializable
/// If a message is provided, it’s rendered as text and the whitespace is preserved.
/// Very long text might be abbreviated in the UI.
/// </summary>
public string? Message { get; }
public string? Message
{
get => _sendDefaultPii ? _message : _message.RedactUrl();
private init => _message = value;
}

/// <summary>
/// The type of breadcrumb.
Expand All @@ -39,7 +50,17 @@ public sealed class Breadcrumb : IJsonSerializable
/// Contains a sub-object whose contents depend on the breadcrumb type.
/// Additional parameters that are unsupported by the type are rendered as a key/value table.
/// </remarks>
public IReadOnlyDictionary<string, string>? Data { get; }
public IReadOnlyDictionary<string, string>? Data
{
get => _sendDefaultPii
? _data
: _data?.ToDictionary(
x => x.Key,
x => x.Value?.RedactUrl()
)!
mattjohnsonpint marked this conversation as resolved.
Show resolved Hide resolved
;
private init => _data = value;
}

/// <summary>
/// Dotted strings that indicate what the crumb is or where it comes from.
Expand Down
29 changes: 0 additions & 29 deletions src/Sentry/Internal/PiiBreadcrumbSanitizer.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
namespace Sentry.Internal;

/// <summary>
/// Sanitizes data that potentially contains Personally Identifiable Information (PII) before sending it to Sentry.
/// Extensions to help redact data that might contain Personally Identifiable Information (PII) before sending it to
/// Sentry.
/// </summary>
internal static class PiiUrlSanitizer
internal static class PiiExtensions
{
internal const string RedactedText = "[Filtered]";

/// <summary>
/// Searches for URLs in text data and redacts any PII
/// data from these, as required.
/// Searches for URLs in text data and redacts any PII data from these, as required.
/// </summary>
/// <param name="data">The data to be searched</param>
/// <returns>
/// The data if SendDefaultPii is enabled or if the data does not contain any PII.
/// A redacted copy of the data otherwise.
/// The data, if no PII data is present or a copy of the data with PII data redacted otherwise
/// </returns>
public static string? Sanitize(this string? data)
public static string? RedactUrl(this string? data)
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
{
// If the data is empty then we don't need to sanitize anything
if (string.IsNullOrWhiteSpace(data))
Expand All @@ -29,13 +30,13 @@ internal static class PiiUrlSanitizer
var result = authRegex.Replace(data, match =>
{
var matchedUrl = match.Groups[1].Value;
return SanitizeUrl(matchedUrl);
return RedactAuth(matchedUrl);
});

return result;
}

private static string SanitizeUrl(string data)
private static string RedactAuth(string data)
{
// ^ matches the start of the string. (?i)(https?://) gives a case-insensitive matching of the protocol.
// (.*@) matches the username and password (authentication information). (.*)$ matches the rest of the URL.
Expand Down
18 changes: 0 additions & 18 deletions src/Sentry/Internal/PiiTransactionSanitizer.cs

This file was deleted.

6 changes: 6 additions & 0 deletions src/Sentry/Request.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Sentry.Extensibility;
using Sentry.Internal;
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
using Sentry.Internal.Extensions;

namespace Sentry;
Expand Down Expand Up @@ -176,4 +177,9 @@ public static Request FromJson(JsonElement json)
Cookies = cookies
};
}

internal void Redact()
{
Url = Url.RedactUrl();
}
}
5 changes: 0 additions & 5 deletions src/Sentry/Scope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,6 @@ public void AddBreadcrumb(Breadcrumb breadcrumb)
_breadcrumbs.TryDequeue(out _);
}

if (!Options.SendDefaultPii)
{
breadcrumb = breadcrumb.Sanitize();
}

_breadcrumbs.Enqueue(breadcrumb);
if (Options.EnableScopeSync)
{
Expand Down
7 changes: 6 additions & 1 deletion src/Sentry/SentryClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public void CaptureTransaction(Transaction transaction)

if (!_options.SendDefaultPii)
{
processedTransaction = processedTransaction.Sanitize();
processedTransaction.Redact();
}

CaptureEnvelope(Envelope.FromTransaction(processedTransaction));
Expand Down Expand Up @@ -271,6 +271,11 @@ private SentryId DoSendEvent(SentryEvent @event, Scope? scope)
return SentryId.Empty;
}

if (!Options.SendDefaultPii)
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
{
processedEvent.Redact();
}

return CaptureEnvelope(Envelope.FromEvent(processedEvent, _options.DiagnosticLogger, scope.Attachments, scope.SessionUpdate))
? processedEvent.EventId
: SentryId.Empty;
Expand Down
9 changes: 9 additions & 0 deletions src/Sentry/SentryEvent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,15 @@ public void SetTag(string key, string value) =>
public void UnsetTag(string key) =>
(_tags ??= new Dictionary<string, string>()).Remove(key);

internal void Redact()
{
_request?.Redact();
foreach (var breadcrumb in Breadcrumbs)
{
breadcrumb.Redact();
}
}

/// <inheritdoc />
public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger)
{
Expand Down
12 changes: 12 additions & 0 deletions src/Sentry/Transaction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,18 @@ public void SetMeasurement(string name, Measurement measurement) =>
SpanId,
IsSampled);

/// <summary>
/// Redacts PII from the transaction
/// </summary>
internal void Redact()
{
Description = Description.RedactUrl();
foreach (var breadcrumb in Breadcrumbs)
{
breadcrumb.Redact();
}
}

/// <inheritdoc />
public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger)
{
Expand Down
43 changes: 0 additions & 43 deletions test/Sentry.Tests/Internals/PiiBreadcrumbSanitizerTests.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
namespace Sentry.Tests.Internals;

public class PiiUrlSanitizerTests
public class PiiExtensionsTests
{
[Fact]
public void Sanitize_Null()
{
var actual = PiiUrlSanitizer.Sanitize(null);
var actual = PiiExtensions.RedactUrl(null);

Assert.Null(actual);
}
Expand All @@ -16,7 +16,7 @@ public void Sanitize_Null()
[InlineData("htp://user:[email protected]?q=1&s=2&token=secret#top", "doesn't affect malformed http urls")]
public void Sanitize_Data_IsNotNull_WithoutPii(string original, string reason)
{
var actual = PiiUrlSanitizer.Sanitize(original);
var actual = original.RedactUrl();

actual.Should().Be(original, reason);
}
Expand All @@ -31,7 +31,7 @@ public void Sanitize_Data_IsNotNull_WithoutPii(string original, string reason)
[InlineData("GET https://[email protected] for goodness", "GET https://[Filtered]@sentry.io for goodness", "strips user info from URL embedded in text")]
public void Sanitize_Data_IsNotNull_WithPii(string original, string expected, string reason)
{
var actual = PiiUrlSanitizer.Sanitize(original);
var actual = original.RedactUrl();

actual.Should().Be(expected, reason);
}
Expand Down
48 changes: 47 additions & 1 deletion test/Sentry.Tests/Protocol/BreadcrumbTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using FluentAssertions.Execution;

namespace Sentry.Tests.Protocol;

public class BreadcrumbTests : ImmutableTests<Breadcrumb>
public class BreadcrumbTests
{
private readonly IDiagnosticLogger _testOutputLogger;

Expand All @@ -9,6 +11,50 @@ public BreadcrumbTests(ITestOutputHelper output)
_testOutputLogger = new TestOutputDiagnosticLogger(output);
}

[Fact]
public void Redact_Redacts_Urls()
{
// Arrange
var breadcrumbData = new Dictionary<string, string>
{
{"url", "https://[email protected]"},
{"method", "GET"},
{"status_code", "403"}
};
var timestamp = DateTimeOffset.UtcNow;
var message = "message https://[email protected]";
var type = "fake_type";
var data = breadcrumbData;
var category = "fake_category";
var level = BreadcrumbLevel.Error;

var breadcrumb = new Breadcrumb(
timestamp : timestamp,
message : message,
type : type,
data : breadcrumbData,
category : category,
level : level
);

// Act
breadcrumb.Redact();

// Assert
using (new AssertionScope())
{
breadcrumb.Should().NotBeNull();
breadcrumb.Timestamp.Should().Be(timestamp);
breadcrumb.Message.Should().Be("message https://[Filtered]@sentry.io"); // should be sanitized
breadcrumb.Type.Should().Be(type);
breadcrumb.Data?["url"].Should().Be("https://[Filtered]@sentry.io"); // should be sanitized
breadcrumb.Data?["method"].Should().Be(breadcrumb.Data?["method"]);
breadcrumb.Data?["status_code"].Should().Be(breadcrumb.Data?["status_code"]);
breadcrumb.Category.Should().Be(category);
breadcrumb.Level.Should().Be(level);
}
}

[Fact]
public void SerializeObject_ParameterlessConstructor_IncludesTimestamp()
{
Expand Down
Loading