Skip to content

Fix: Trim Unicode NULL character for Windows support #420

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

Merged
merged 5 commits into from
Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Fix: Mark `Sentry.currentHub` as deprecated (#406)
* Fix: Use name from pubspec.yaml for release if package id is not available (#411)
* Feat: `SentryHttpClient` tracks the duration which a request takes and logs failed requests (#414)
* Fix: Trim `\u0000` from Windows package info (#420)

# 5.0.0

Expand Down
34 changes: 26 additions & 8 deletions flutter/lib/src/default_integrations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -366,19 +366,31 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
try {
if (options.release == null || options.dist == null) {
final packageInfo = await _packageLoader();
var name = packageInfo.packageName;
var name = _cleanString(packageInfo.packageName);
if (name.isEmpty) {
// Not all platforms have a packageName.
// If no packageName is available, use the appName instead.
name = _cleanAppName(packageInfo.appName);
name = _cleanString(packageInfo.appName);
}

final version = _cleanString(packageInfo.version);
Copy link
Member

Choose a reason for hiding this comment

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

This block is guarded by either release or dist being empty on options. But below we only take the release built in here, if options.release is not empty, we just use that, and same for dist.

So if only options.release is set, we'll build a new release (as per this block) but won't use it at all.
Would it make sense to check if + exists and append the buildNumber we detected in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ?? does the job if I got what you meant eg

options.release = options.release ?? release;

but what we could do is to actually check if we need to build the release or dist only and not execute code that is not necessary, eg, building a new release if we're going to use the options.release anyway as its not empty

Copy link
Member

Choose a reason for hiding this comment

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

yeah it's already using options.release if it's already set, it's just odd that we build a new release value even though it's not used.

And I was trying to think of ways to combine what we built in the block with what was defined but best not mess around with a value that was explicitly set by the user on options.

So the only improvement here is as you said just to avoid doing the work if not needed though it'll make the logic more complicated

final buildNumber = _cleanString(packageInfo.buildNumber);

var release = name;
if (version.isNotEmpty) {
release = '$release@$version';
}
// At least windows sometimes does not have a buildNumber
if (buildNumber.isNotEmpty) {
release = '$release+$buildNumber';
}

final release =
'$name@${packageInfo.version}+${packageInfo.buildNumber}';
options.logger(SentryLevel.debug, 'release: $release');

options.release = options.release ?? release;
options.dist = options.dist ?? packageInfo.buildNumber;
if (buildNumber.isNotEmpty) {
options.dist = options.dist ?? buildNumber;
}
}
} catch (error) {
options.logger(
Expand All @@ -388,15 +400,21 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
options.sdk.addIntegration('loadReleaseIntegration');
}

String _cleanAppName(String appName) {
/// This method cleans the given string from characters which should not be
/// used.
/// For example https://docs.sentry.io/platforms/flutter/configuration/releases/#bind-the-version
/// imposes some requirements. Also Windows uses some characters which
/// should not be used.
String _cleanString(String appName) {
// Replace disallowed chars with an underscore '_'
// https://docs.sentry.io/platforms/flutter/configuration/releases/#bind-the-version
return appName
.replaceAll('/', '_')
.replaceAll('\\', '_')
.replaceAll('\t', '_')
.replaceAll('\r\n', '_')
.replaceAll('\r', '_')
.replaceAll('\n', '_');
.replaceAll('\n', '_')
// replace Unicode NULL character with an empty string
.replaceAll('\u{0000}', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to be sure that this works on Windows, maybe @bruno-garcia can test it?

Copy link
Member

Choose a reason for hiding this comment

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

I dream of integration test on the target platform that will validate these for us. Mainly because of regressions introduced later.

Thoughts? Q2 is the time to plan and book these tasks

}
}
66 changes: 65 additions & 1 deletion flutter/test/default_integrations_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ void main() {
expect(fixture.options.dist, '789');
});

test('release name does not contain ivalid chars', () async {
test('release name does not contain invalid chars defined by Sentry',
() async {
final loader = () {
return Future.value(PackageInfo(
appName: '\\/sentry\tflutter \r\nfoo\nbar\r',
Expand All @@ -283,6 +284,69 @@ void main() {
expect(fixture.options.release, '__sentry_flutter [email protected]+789');
expect(fixture.options.dist, '789');
});

/// See the following issues:
/// - https://github.com/getsentry/sentry-dart/issues/410
/// - https://github.com/fluttercommunity/plus_plugins/issues/182
test('does not send Unicode NULL \\u0000 character in app name or version',
() async {
final loader = () {
return Future.value(PackageInfo(
// As per
// https://api.dart.dev/stable/2.12.4/dart-core/String-class.html
// this is how \u0000 is added to a string in dart
appName: 'sentry_flutter_example\u{0000}',
packageName: '',
version: '1.0.0\u{0000}',
buildNumber: '',
));
};
await fixture
.getIntegration(loader: loader)
.call(MockHub(), fixture.options);

expect(fixture.options.release, '[email protected]');
});

/// See the following issues:
/// - https://github.com/getsentry/sentry-dart/issues/410
/// - https://github.com/fluttercommunity/plus_plugins/issues/182
test(
'does not send Unicode NULL \\u0000 character in package name or build number',
() async {
final loader = () {
return Future.value(PackageInfo(
// As per
// https://api.dart.dev/stable/2.12.4/dart-core/String-class.html
// this is how \u0000 is added to a string in dart
appName: '',
packageName: 'sentry_flutter_example\u{0000}',
version: '',
buildNumber: '123\u{0000}',
));
};
await fixture
.getIntegration(loader: loader)
.call(MockHub(), fixture.options);

expect(fixture.options.release, 'sentry_flutter_example+123');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong, in this case, if there's no version, it should be sentry_flutter_example@123

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll confirm that, let's keep as it is for now

Copy link
Contributor

@marandaneto marandaneto Apr 20, 2021

Choose a reason for hiding this comment

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

this is actually not going to happen because pubspec must have a version or it defaults to 0.0.0 https://dart.dev/tools/pub/pubspec#version
its only possible because we've mocked the values, so no special anything case here.

});

test('dist is null if build number is an empty string', () async {
final loader = () {
return Future.value(PackageInfo(
appName: 'sentry_flutter_example',
packageName: 'a.b.c',
version: '1.0.0',
buildNumber: '',
));
};
await fixture
.getIntegration(loader: loader)
.call(MockHub(), fixture.options);

expect(fixture.options.dist, isNull);
});
});
}

Expand Down