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

Dart sdk missing full (absolute) file path to define ownership rules #347

Closed
3 of 11 tasks
lukesterlee opened this issue Mar 10, 2021 · 29 comments
Closed
3 of 11 tasks

Comments

@lukesterlee
Copy link

Platform:

  • Dart
  • Flutter Android or iOS
  • Flutter Web

IDE:

  • VSCode
  • IntelliJ/AS
  • XCode
  • Other, which one?

split-debug-info and obfuscate (Flutter Android or iOS) or CanvasKit (Flutter Web):

  • Enabled
  • Disabled

Platform installed with:

  • pub.dev
  • GitHub

Output of the command flutter doctor -v below:

[✓] Flutter (Channel unknown, 1.22.5, on Mac OS X 10.15.7 19H524 darwin-x64, locale en)
• Flutter version 1.22.5 at /Users/lukelee/fvm/versions/1.22.5
• Framework revision 7891006299 (3 months ago), 2020-12-10 11:54:40 -0800
• Engine revision ae90085a84
• Dart version 2.10.4

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.0-rc1)
• Android SDK at /Users/lukelee/Library/Android/sdk
• Platform android-30, build-tools 30.0.0-rc1
• Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
• Java version OpenJDK Runtime Environment (build 1.8.0_212-release-1586-b4-5784211)
• All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 12.4)
• Xcode at /Applications/Xcode.app/Contents/Developer
• Xcode 12.4, Build version 12D4e
• CocoaPods version 1.10.0

[✓] Android Studio (version 3.6)
• Android Studio at /Applications/Android Studio.app/Contents
• Flutter plugin version 45.1.1
• Dart plugin version 192.8052
• Java version OpenJDK Runtime Environment (build 1.8.0_212-release-1586-b4-5784211)

[✓] Connected device (2 available)
• Android SDK built for x86 64 (mobile) • emulator-5554 • android-x64 • Android 10 (API 29)
(emulator)
• macOS (desktop) • macos • darwin-x64 • Mac OS X 10.15.7 19H524
darwin-x64

• No issues found!
The version of the SDK (See pubspec.lock):
4.0.4


I have the following issue:

Dart sdk is missing a full (absolute) file path to define ownership rules.
The data is included in JSON files as abs_filepath but the path is not popping up in the ownership rules section.

Steps to reproduce:

  • Step 1: trigger an exception in a flutter app.
  • Step 2: go to sentry and find the exception you triggered.
  • Step 3: click Create Ownership Rule on the right panel.
  • Step 4: the full absolute file path is not popping up so you can't utilize the ownership rule feature to define the paths to assign to a specific team.

Screen Shot 2021-03-10 at 1 24 23 PM

Actual result:
When we use flutter sdk, we don't get the full path.

  • Actual

Screen Shot 2021-03-03 at 3 51 43 PM

Expected result:
When we log to Android SDK via method channel, we get this full path.

  • Result

Screen Shot 2021-03-10 at 1 11 08 PM

@marandaneto
Copy link
Contributor

marandaneto commented Mar 11, 2021

@lukesterlee thanks for reporting this, the reason is:

https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/sentry_stack_trace_factory.dart#L127-L143

does it mean that using the Flutter SDK works but the pure Dart SDK does not?

eg:
file:///Users/myuser/Github/sentry-dart/dart/example/main.dart so we have PII here but there's also no easy way to know when the project actually starts which would be sentry-dart.
Ideas on how to do it?

I was not aware this would affect ownership rules, good catch

@marandaneto
Copy link
Contributor

marandaneto commented Mar 11, 2021

I can confirm that, Flutter SDK would return package:sentry_flutter_example/main.dart instead of the file absolute path.

the problem is, for a Dart App, the frame.uri.scheme is actually a file scheme and we need to filter that :( not sure how we can go around this.

@bruno-garcia ideas?

@marandaneto
Copy link
Contributor

I only see a way if filtering frame.uri.pathSegments but we'd need to know where to start discarding PII info

@ueman
Copy link
Collaborator

ueman commented Mar 11, 2021

I would guess splitting at the lib folder would be a good starting point as practically every Dart programm uses it.
There would be of course some problems if there are more than one lib folder.

@marandaneto
Copy link
Contributor

yep @ueman that's one of my thoughts, for scripting it'd not work though, but I guess heuristics would be the way to go, or, Dart offering a proper relative path property.
The other way would be getting the user's folder from the OS and substring from there, it'd not avoid PII entirely though.

@ueman
Copy link
Collaborator

ueman commented Mar 11, 2021

There is also https://api.dart.dev/stable/2.12.1/dart-io/Platform-class.html which should help in some cases. I'm pretty sure it doesn't work with Flutter, I would have to double check that however.

@marandaneto
Copy link
Contributor

There is also https://api.dart.dev/stable/2.12.1/dart-io/Platform-class.html which should help in some cases. I'm pretty sure it doesn't work with Flutter, I would have to double check that however.

still returns /Users/myuser/Github/sentry-dart/dart/example/main.dart but yeah I've not googled it yet, might have a reliable workaround somewhere hidden in the Webs :D

@lukesterlee
Copy link
Author

@marandaneto I'm sorry for the confusion, I meant we do get the right data in both android sdk and flutter sdk but flutter sdk does not know how to parse the data in the ownership rules section since they are parsing filename instead of abs_path.

This is from our JSON raw payload:

{
"function":"CheckingCardService.lock",
"package":"betterment_flutter",
"filename":"checking_card_service.dart",
"abs_path":"package:betterment_flutter/features/checking/manage_account/services/checking_card_service.dart",
"lineno":39,
"colno":7,
"in_app":true
}

As you can see in the previous screenshots, it seems like android sdk's ownership rules parse abs_path while flutter sdk's ownership rules parse filename. I think there is no problem with sdk, we just have a problem with pulling the right attribute in ownership rules section on the web?

by the way, thanks for the quick response! I really appreciate it. 🙌

@marandaneto
Copy link
Contributor

@lukesterlee thanks for your reply.

I am a bit lost here.

The data is included in JSON files as abs_filepath but the path is not popping up in the ownership rules section.

this is indeed an issue (Sentry Dart SDK), but unless we use some heuristics, there's no native way to get proper absolute paths without risking PII.

the Sentry Flutter SDK though gets proper absolute file paths, as the tooling doesn't include PII so we send it as it is, so it's all good afaik.

When we log to Android SDK via method channel, we get this full path.

what do you mean by Android SDK? are you talking about the Sentry Flutter SDK or Native SDKs (Java/Kotlin and Swift/ObjC events)? Please review your description so we can get a better understanding :)

@samandmoore
Copy link

samandmoore commented Mar 16, 2021

(i work with @lukesterlee)

this is indeed an issue (Sentry Dart SDK), but unless we use some heuristics, there's no native way to get proper absolute paths without risking PII.

@marandaneto would it be possible to introduce something like inAppIncludes to identify the root of the app within the file system?

List<String> get inAppIncludes => List.unmodifiable(_inAppIncludes);

with something like that, it would be possible to trim all path segments before that one and call that the absolute path, right?

edit: and i think that logic would only want to be applied to files whose scheme is file: right?

@samandmoore
Copy link

@lukesterlee thanks for reporting this, the reason is:

https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/sentry_stack_trace_factory.dart#L127-L143

does it mean that using the Flutter SDK works but the pure Dart SDK does not?

@marandaneto another question, just for clarification, i'm not quite understanding how this code is converting package:betterment_flutter/something.dart into something.dart. i'd expect that since the scheme is package it would not be stripping the path down to just the filename. can you explain why that's happening?

@marandaneto
Copy link
Contributor

(i work with @lukesterlee)

this is indeed an issue (Sentry Dart SDK), but unless we use some heuristics, there's no native way to get proper absolute paths without risking PII.

@marandaneto would it be possible to introduce something like inAppIncludes to identify the root of the app within the file system?

List<String> get inAppIncludes => List.unmodifiable(_inAppIncludes);

with something like that, it would be possible to trim all path segments before that one and call that the absolute path, right?

not sure how that would work as the dir. path would be dynamic depending on the user's folder etc.

edit: and i think that logic would only want to be applied to files whose scheme is file: right?

yes, only to file, when its package or something else, the absolute path is already fine

@marandaneto
Copy link
Contributor

@lukesterlee thanks for reporting this, the reason is:
https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/sentry_stack_trace_factory.dart#L127-L143
does it mean that using the Flutter SDK works but the pure Dart SDK does not?

@marandaneto another question, just for clarification, i'm not quite understanding how this code is converting package:betterment_flutter/something.dart into something.dart. i'd expect that since the scheme is package it would not be stripping the path down to just the filename. can you explain why that's happening?

it's not in such case, only for file:...

@bruno-garcia
Copy link
Member

One proposal we're considering is to disable this PII stripping in the SDK if sendDefaultPii is set to true.

Related: #360

Docs (JS for now until we release for Dart):
https://docs.sentry.io/platforms/javascript/configuration/options/#send-default-pii

@lukesterlee
Copy link
Author

I think mentioning android/dart sdk might have confused you. I will try my best to state it with more clarity!
I'm using Flutter SDK 4.0.4.

Step1: make the flutter part crash, then go to the dashboard.

Screen Shot 2021-03-16 at 11 17 19 PM

As you can see ^ above, I was able to see the full path when I hovered checking_card_service.dart exception. (package:betterment_flutter/features/checking/manage_account/services/checking_card_service.dart)

Step2: click Create Ownership Rule on the right panel in the dashboard of the issue.

Screen Shot 2021-03-17 at 1 12 30 PM

Then, you don't see the full path.

Screen Shot 2021-03-16 at 11 17 41 PM

Therefore, I can't use the ownership rule feature since I can't get the full path.

Let me know if there is any step I missed!

@marandaneto
Copy link
Contributor

@lukesterlee oh yeah indeed that makes sense, please open an issue on https://github.com/getsentry/sentry for that, I'll keep this open here because the pure Dart SDK strip the absolute path off anyway, thanks for reporting.

@marandaneto
Copy link
Contributor

the file path is used for grouping, so if we change that, we'd need to consider bumping a major bump, we could do it now btw before v5

@ueman
Copy link
Collaborator

ueman commented Jun 3, 2021

This line could be related:

return frame.uri.pathSegments.last;

A frame can be relative even if it does not has a scheme.

This line

final fileName = frame.uri.pathSegments.isNotEmpty
just always uses the last segment of the path, even for relativ paths. That would explain the bug which the user described.

I'm guessing that the web path might also be wrong, though I haven't checked that yet.

@marandaneto
Copy link
Contributor

the reasoning for not using the full segment was because sometimes it contains PII and events coming from different machines would not be grouped together, its not a bug but something we should improve.

@ueman ueman added this to the 6.0.0 milestone Jun 15, 2021
@ueman
Copy link
Collaborator

ueman commented Jun 20, 2021

I figured out what's the problem. We're sending just the file name as the file_name property.
The docs and examples however state that we should include the file path relative to the project root.

So given the following folder structure, where app is the project root and app is also the name given in the pubspec.yaml

.
└── app/
    └── lib/
        ├── main.dart
        └── foo_bar/
            └── foo_bar.dart

Current behaviour:

  • app/lib/main.dart is reported as main.dart
  • app/lib/foo_bar/foo_bar.dart is reported as foo_bar.dart

This is what we would get by just parsing the stacktrace

  • app/lib/main.dart is reported as app/main.dart
  • app/lib/foo_bar/foo_bar.dart is reported as app/foo_bar/foo_bar.dart

I'm not quite sure what the expected behaviour should be.
I think it should be what the stacktrace is saying but I'm not sure how the webservice handles it

@samandmoore
Copy link

I'm not quite sure what the expected behaviour should be.
I think it should be what the stacktrace is saying but I'm not sure how the webservice handles it

Nice sleuthing.

I'm not sure what the web service expect either, but it appears to accept 2 params: filename and absPath. Typically, i expect that filename is just the final segment of the filepath, so in your examples: main.dart is the filename. Although, given what I described here, it seems like the web service prioritizes filename over absPath in ownership rules which basically means that if the submitted filename only contains the final segment of the file path then the ownership rules glob matching is pretty limited / nearly useless (context: getsentry/sentry#24525 (comment)).

So, without a change to the server, one thing this library could do is to start submitting filename as the modified file path strings you showed in the "just by parsing the stacktrace" section. Not sure if y'all wanna do that or not though 🤷🏻

@ueman
Copy link
Collaborator

ueman commented Jun 21, 2021

It does make sense to send the full path. Otherwise files with the same name in different folders can't be distinguished.

Though because of the way imports work in Dart, there could be PII in the import path.
For example if you're using relative paths and reference a file in your user folder.

It's quite hard to make it easily usable in every use case :(

I believe package imports should be fine and included by default with its path.

@bruno-garcia
Copy link
Member

Though because of the way imports work in Dart, there could be PII in the import path.

PII in paths are handled by Sentry I believe. Where it can remove C:\Users\[filtered]\ We should send the paths if we can find a way. This problem exists on Android too where Java only gives us the file name without a path. One option there is to have a best effort an convert the package to a path hoping it's the default class loader (io.sentry.test becomes io/sentry/test/File.java)

@marandaneto
Copy link
Contributor

I need to check how ownership rules work, its probably based on file path instead of packages or something else, let me get back to this in a bit.

@marandaneto
Copy link
Contributor

https://github.com/getsentry/sentry/blob/51fffaad802a1f8c4331bf6030fd5f23d8673bff/src/sentry/ownership/grammar.py#L95 is the responsible field (filename) for ownership rules, ideally, the server could work with the "packages" concept too, but that would take longer.
a quick workaround would be what @bruno-garcia said.

io.sentry.test.File becomes io/sentry/test/File.java on Android

https://develop.sentry.dev/sdk/event-payloads/stacktrace/#frame-attributes

filename

The path to the source file relative to the project root directory.

abs_path

The absolute path to the source file.

I'll check if Sentry handles the file path PII automatically or if we should guard against sendDefaultPii

@ueman
Copy link
Collaborator

ueman commented May 3, 2022

I think in Flutter applications there doesn't need to be a check for PII in paths since Flutter is always package based.

@marandaneto
Copy link
Contributor

marandaneto commented May 4, 2022

I think in Flutter applications there doesn't need to be a check for PII in paths since Flutter is always package based.

Nope, only Dart and Web.

@marandaneto marandaneto moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK May 12, 2022
@barkbarkimashark
Copy link

barkbarkimashark commented Jun 30, 2022

I've merged this getsentry/sentry#35273 into the server as an initial attempt at resolving this issue.

In particular, please see this test which should illustrate ownership rules evalutation:
https://github.com/getsentry/sentry/pull/35273/files#diff-4c04f5ce7e6c9e5907ec7f6d6e2d379f2b9ca8cbf2ecf05b09afc6d785e83fc4R332

@marandaneto
Copy link
Contributor

Closing it then, since the work was done on the server, thanks @barkbarkimashark

Repository owner moved this from Backlog to Done in Mobile & Cross Platform SDK Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

7 participants