-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(client/android): Add report button for connection failures with webhook integration #2484
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
base: master
Are you sure you want to change the base?
Conversation
…erverAdded function + Add keyHasWebhook function
…rButtonHandler in showToast function + Add button to html template
|
Thank you for your contribution. Snackbars should have only one action (with an optional dismiss): https://m3.material.io/components/snackbar/guidelines#ae0935b0-a14e-4dec-8c3c-2c0809acffeb. This needs a redesign. |
@techamateur, |
jyyi1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution 👍. Before we review the code, please clean up the translation files, as they are handled automatically by our pipeline.
…of "send_error" in old version
…ey extracter in "onserveradded" function + Improve keyhaswebhook, showErrorCauseDialog functions
…, "disconnectedCallback" and "openWithDetails" functions + define send error properties + add "send error" button to html renderer
|
Please add GitHub Copilot as a reviewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. But before we get into a detailed review, I'd like to pause and align on the overall design, as I have a few questions on the current approach:
-
The current design seems to cover manual connection errors. How would we handle reporting for other cases, like automatic reconnection failures? Also, what if the internet is down and we failed to send the report?
-
The
&webhook=parameter works well for URL-based keys. How would this be configured for other formats, like a YAML key? Could you please provide some example keys? -
To keep our client code simple, the parsing and handling of this reporting logic feels better suited for the Go layer rather than TypeScript. And it's also very hard for TypeScript to correctly parse an access key (considering we already have URL, JSON and YAML key formats).
-
Could you clarify what data would be sent in the report? We need to be careful to avoid sending any private user information and prevent any potential misuse of the webhook URL. What if the error details include user sensitive data?
… to fix Go and test issues, install Android Build Tools 35.0.0, fix TypeScript and Mocha issues, Revert files that shouldn't have changed
|
Most of these questions have been answered in #2484 (comment). I am adding a few more points.
Sample keys are shown in #2036. The following can be a sample YAML key. Adding the webhook as a parameter in the URL rather than adding the webhook to the content of the YAML key has the benefit of being able to report the failure in fetching the key. Therefore, we decided to use this syntax.
As mentioned in the response to your 2nd question, the plan is not to parse the YAML key content and webhook is retrieved from the key URL.
|
client/src/www/messages/am.json
Outdated
| "version": "ቅጂ {appVersion}", | ||
| "yes": "አዎ" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not show any change in this file, but this is still shown in the changes. Can you please revert these changes in a way so that we would not see any changes in the files located in client/src/www/messages/?
|
I have some privacy concerns about this design. It can reveal the user ID (from the key), and the user IP (from the error), which will in turn get logged somewhere. And the user probably has no idea of what information they are leaking. You don't really need that information. For example, the "webhook" could be something like Also, let's rename |
The key provider has numerous methods to get the user IP such as through the VPN server it provides. Whenever the user tries to connect to a VPN server, its IP is revealed; therefore, this reporting mechanism is not the only place that reveals the user IP. The key provider can also use tools that sit in between the user and the Outline server that just forward the traffic while collecting user IP and similar information.
The webhook link used here can be any type of link and does not necessarily need to be a link to webhook.site. Outline just posts a cURL request to that link to send the error information.
The goal of this feature is to send as much error information as possible to better help the provider to come up with a solution for the error message faced.
That is a good suggestion. I guess this is reasonable to use a more universal name such as |
|
We discussed this in a meeting. The main issues are that both the strategy and the error messages can contain PII (e.g. credentials -> user id, user IP address), so BOTH need to be sanitized. For reporting the strategy, I'd favor fixing some sort of strategy ID or tags. (e.g. For the errors, we should come up with a mechanism to ensure they are free of PII. That can be tricky. At a minimum, we should sanitize it to redact any reference to hostnames or IPs (v4 and v6), perhaps host:port. I'd prefer to have the reporting config after the |
|
Thank you for your thoughtful comment.
All PII can be sanitized using the Outline’s existing functions and any new changes that need to be introduced, but the user’s IP cannot be hidden as it is the IP that sends the report.
Yes, this is possible, but as explained later, the VPN provider can use separate report receivers for different servers and easily find out which key a report corresponds to.
Removing the hostname will defeat the purpose of this feature. The main purpose of reporting the error message is to let the VPN provider which VPN server has a problem. If the VPN provider receives only an error message, then it is not useful in most scenarios because the VPN provider does not know which one of its servers has a problem. However, the VPN provider can provide a different reporting address for each of their VPN servers. Therefore, they can identify which VPN server a report corresponds to.
This is a sample key: ssconf://domain.com/password#Key_name
Using either of the 2nd and the 3rd formats changes the Outlines assumption that everything that goes after |
Resolves #2036
What
REPORTbutton next toDETAILSin connection error dialog.Why
How to Test
ss://...?&webhook=https://example.com/report#key).REPORTand verify webhook receives JSON payload:Screenshot
