-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
Validate Proxy Url before enable proxy #4470
Conversation
initial_settings.proxy_enabled !== accountSettings.proxy_enabled && | ||
accountSettings.proxy_enabled === '1' | ||
) { | ||
const qr = await BackendRemote.rpc.checkQr( |
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.
are all proxies we support also supported to be read as qr code? Like is the set of supported formats the same on both qr code and the config field?
Also reusing "checkQR" looks like a workaround, what do the other platforms do? Is there a reason core does not have a validate_proxy
or even test_proxy
method?
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.
are all proxies we support also supported to be read as qr code?
well it's in fact just a string containg (possibly) an URL.
we could do some URL validation ourselves, but I assumed it's better to use the same logic as core.
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.
Also this looks like a workaround, what do the other platforms do?
I discussed it with @adbenitez - he proposed the QR code check.
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.
@link2xt is the QR code check sufficient? Is there a better core check available?
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.
QR code check is the best check for proxy, there is no special API. All usable proxies can be put into the QR code and are recognized as proxy type QR code.
packages/frontend/src/components/dialogs/EditAccountAndPasswordDialog.tsx
Outdated
Show resolved
Hide resolved
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.
Works alright, apart from my suggestion. Tested with SOCKS5 proxies: reachable, unreachable, invalid URLs.
The other changes also make sense.
const qr = await BackendRemote.rpc.checkQr( | ||
selectedAccountId(), | ||
accountSettings.proxy_url | ||
) | ||
if (qr.kind !== 'proxy') { | ||
openDialog(AlertDialog, { | ||
message: tx('proxy_invalid'), | ||
}) | ||
return | ||
} |
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.
checkQr
can throw e.g. for the string socks5:
const qr = await BackendRemote.rpc.checkQr( | |
selectedAccountId(), | |
accountSettings.proxy_url | |
) | |
if (qr.kind !== 'proxy') { | |
openDialog(AlertDialog, { | |
message: tx('proxy_invalid'), | |
}) | |
return | |
} | |
let qr: T.Qr | undefined = undefined | |
try { | |
qr = await BackendRemote.rpc.checkQr( | |
selectedAccountId(), | |
accountSettings.proxy_url | |
) | |
} catch (error) { | |
const errorStr = | |
error instanceof Error ? error.message : JSON.stringify(error) | |
openDialog(AlertDialog, { | |
message: tx('proxy_invalid') + '\n' + errorStr, | |
}) | |
return | |
} | |
if (qr.kind !== 'proxy') { | |
openDialog(AlertDialog, { | |
message: tx('proxy_invalid'), | |
}) | |
return | |
} |
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.
Oh, and import type { T } from '@deltachat/jsonrpc-client'
would need to be added.
Fixes #4231
Restarting IO didn't help.