-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Comments
@lukesterlee thanks for reporting this, the reason is: does it mean that using the Flutter SDK works but the pure Dart SDK does not? eg: I was not aware this would affect ownership rules, good catch |
I can confirm that, Flutter SDK would return the problem is, for a Dart App, the @bruno-garcia ideas? |
I only see a way if filtering |
I would guess splitting at the |
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. |
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 |
@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 This is from our JSON raw payload:
As you can see in the previous screenshots, it seems like android sdk's ownership rules parse by the way, thanks for the quick response! I really appreciate it. 🙌 |
@lukesterlee thanks for your reply. I am a bit lost here.
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.
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 :) |
(i work with @lukesterlee)
@marandaneto would it be possible to introduce something like sentry-dart/dart/lib/src/sentry_options.dart Line 123 in ae21d7e
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 |
@marandaneto another question, just for clarification, i'm not quite understanding how this code is converting |
not sure how that would work as the dir. path would be dynamic depending on the user's folder etc.
yes, only to |
it's not in such case, only for |
One proposal we're considering is to disable this PII stripping in the SDK if Related: #360 Docs (JS for now until we release for Dart): |
I think mentioning android/dart sdk might have confused you. I will try my best to state it with more clarity! Step1: make the flutter part crash, then go to the dashboard.As you can see ^ above, I was able to see the full path when I hovered Step2: click
|
@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. |
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 |
This line could be related:
A frame can be relative even if it does not has a scheme. This line
I'm guessing that the web path might also be wrong, though I haven't checked that yet. |
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. |
I figured out what's the problem. We're sending just the file name as the So given the following folder structure, where
Current behaviour:
This is what we would get by just parsing the stacktrace
I'm not quite sure what the expected behaviour should be. |
Nice sleuthing. I'm not sure what the web service expect either, but it appears to accept 2 params: So, without a change to the server, one thing this library could do is to start submitting |
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. 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. |
PII in paths are handled by Sentry I believe. Where it can remove |
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. |
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.
https://develop.sentry.dev/sdk/event-payloads/stacktrace/#frame-attributes
I'll check if Sentry handles the file path PII automatically or if we should guard against |
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. |
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: |
Closing it then, since the work was done on the server, thanks @barkbarkimashark |
Platform:
IDE:
split-debug-info and obfuscate (Flutter Android or iOS) or CanvasKit (Flutter Web):
Platform installed with:
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:
Create Ownership Rule
on the right panel.Actual result:
When we use flutter sdk, we don't get the full path.
Expected result:
When we log to Android SDK via method channel, we get this full path.
The text was updated successfully, but these errors were encountered: