borgbase / vorta

Desktop Backup Client for Borg Backup
https://vorta.borgbase.com
GNU General Public License v3.0
1.98k stars 130 forks source link

Fix freeze when opening repo on unsupported URL scheme. #1631

Open real-yfprojects opened 1 year ago

real-yfprojects commented 1 year ago

It seems like not all file explorers respect the supported scheme property, that's why Vorta shouldn't rely on that. However currently Vorta will freeze when opening an unsupported scheme like smb or sftp. This should be fixed. A small error message should be displayed in the dialog instead.


You can open smb:// and other URLs when adding a repository although borg only supports normal paths (file scheme) and the ssh scheme. This is because vorta is missing to set the supported schemes attribute.

See https://doc.qt.io/qt-5/qfiledialog.html#supportedSchemes-prop

Originally posted by @ThomasWaldmann in https://github.com/borgbase/vorta/issues/1628#issuecomment-1459041597

i1sm3ky commented 1 year ago

@ThomasWaldmann Thanks for the guide, I'll check the borg code for the url validator and the unit tests. Sorry for the inconvenience, I just looked past the fact that borg also does a url check, I don't know what was I thinking of :/ I'll see the borg code and implement it and share it with you soon. Could you please help me locate the files which may contain the url validator and the unit tests for borg as I haven't been much involved in the borg code base.

i1sm3ky commented 1 year ago

Found the REs and the location_validator on borg.

real-yfprojects commented 1 year ago

Edit: In the PSF checklist they have mentioned Your code doesn't have to be accepted and merged, but it does have to be visible to the public and it does have to be your own work so a link to the issues where the code have been discussed should work right?

Opening a PR would be even better!

i1sm3ky commented 1 year ago

Opening a PR would be even better!

Ya that's why I'm trying to get it done before 4th so that I can include it as a PR in the proposal.

real-yfprojects commented 1 year ago

You can also open a draft PR so we can better see and comment on your current progress.

i1sm3ky commented 1 year ago

Yeah sure! I'll do that.

real-yfprojects commented 1 year ago

Why does vorta freeze in the first place?

i1sm3ky commented 1 year ago

Sorry for the delayed reply. I think its because selected file URL type isn't supported and doesn't get processed at some point and Vorta doesn't get a result out of it so it freezes waiting for a result to work upon.

real-yfprojects commented 1 year ago

Sorry for the delayed reply.

Your reply isn't late at all.

I think its because selected file URL type isn't supported and doesn't get processed at some point and Vorta doesn't get a result out of it so it freezes waiting for a result to work upon.

Can you triage which lines of code cause the freeze?

i1sm3ky commented 1 year ago

Can you triage which lines of code cause the freeze?

I'll test it tomorrow, I'll get my friend's laptop to test on -x86_64 linux.

I'm not able to install vorta on arm linux, PyQt5 to be precise. Any solution for installing it? I'm on M1 Mac.

ThomasWaldmann commented 1 year ago

@i1sm3ky https://github.com/borgbase/vorta/issues/727#issuecomment-915156613 that was how I got it working on macOS ARM (M1). Maybe Linux ARM solution is similar?

i1sm3ky commented 1 year ago

@ThomasWaldmann Thanks for the suggestion but I was able to install vorta and borg using emulated -x86_64 architecture on Rosetta 2. But I can't find a way to run amd64 arch on any arm64 linux distro.

I think PyQt6 compiles natively for arm as well, So I think after we upgrade to PyQt6, It'll be usable on arm distros of linux.

ThomasWaldmann commented 1 year ago

What I meant is to try a corresponding approach on linux arm64.

i1sm3ky commented 1 year ago

I got what you meant, but I was unable to find a way of doing that on arm64 linux. It doesn't have any efficient way of using x86 architecture on arm64 linux. And an update on PyQt5 broke the support for arm. Previously it run without any issue on arm but now due to some dependencies it doesn't work anymore. Currently only way of using PyQt5 on arm is to install it using its -x86_64 architecture which isn't currently possible in the arm64 linux through any means.

real-yfprojects commented 1 year ago

You could also use a VM.

i1sm3ky commented 1 year ago

You could also use a VM.

I use Parallels for VMs and it only supports arm ISOs.

TheLazron commented 6 months ago

Hey @real-yfprojects, @ThomasWaldmann I looked into this issue, and after following the previous discussions, looked into how borg validates the URLs. I was able to find the code for that here

I tested the logic in a separate file, had to make slight change to one of the multiple regex patterns, to mark smb:// as invalid. Here is the pastebin url for the same: https://pastebin.com/MPgYbX3r

Can you please have a look

ThomasWaldmann commented 6 months ago

@TheLazron Thanks for looking at this issue, but wasn't the point of this issue that the GUI dialogue must not let you select smb:// stuff as we already know borg can't use that?

About changes to borg: please file an issue in the borgbackup issue tracker about that and either do a PR or at least provide a diff (not: full file).

TheLazron commented 6 months ago

@ThomasWaldmann I used the code from borg without changes, and it wasnt invalidating the smb urls, even http urls were being treated as valid urls

In borg code to validate these urls, the url is first checked against a ssh_re regex pattern, then a file_re for file paths , then a socket_re to check it against socket regex pattern and in the end there is this local_re pattern which is almost being used as a fallback. Here is the snippet for the same:

# path must not contain :: (it ends at :: or string end), but may contain single colons.
    # to avoid ambiguities with other regexes, it must also not start with ":" nor with "//" nor with "ssh://".
    local_path_re = r"""
        (?!(:|//|ssh://|socket://))                         # not starting with ":" or // or ssh:// or socket://
        (?P<path>([^:]|(:(?!:)))+)                          # any chars, but no "::"
        """

And when I tested the above code for a smb:// url, it would mark it as a valid url with output like so:

The URL 'smb://local-server-name/share-name' is a valid location.
Parsed Details:
proto='file', user=None, host=None, port=None, path='smb:/local-server-name/share-name'

So I had to make changes to this local_re pattern::

    local_path_re = r"""
        (?!(:|//|ssh://|socket://|http://|smb://))                         # not starting with ":" or // or ssh:// or socket://
        (?P<path>([^:]|(:(?!:)))+)                          # any chars, but no "::"
        """

which started to invalidate any smb:// urls

ThomasWaldmann commented 6 months ago

About changes to borg: please file an issue in the borgbackup issue tracker about that and either do a PR or at least provide a diff (not: full file).

TheLazron commented 6 months ago

@ThomasWaldmann sorry but I wanted some clarification here. Is borg not invalidating smb urls an issue, since you asked me to file an issue there.

Second for the changes(difference) I did to the existing borg code for parsing the repository Url is here: #

# path must not contain :: (it ends at :: or string end), but may contain single colons.
    # to avoid ambiguities with other regexes, it must also not start with ":" nor with "//" nor with "ssh://".
    local_path_re = r"""
        (?!(:|//|ssh://|socket://))                         # not starting with ":" or // or ssh:// or socket://
        (?P<path>([^:]|(:(?!:)))+)                          # any chars, but no "::"
        """

Difference ( added |smb://)

    local_path_re = r"""
        (?!(:|//|ssh://|socket://|http://|smb://))                         # not starting with ":" or // or ssh:// or socket://
        (?P<path>([^:]|(:(?!:)))+)                          # any chars, but no "::"
        """

I have made draft PR for the same, so if you can take a look at that Thanks for the help

ThomasWaldmann commented 6 months ago

@TheLazron discussion about borg changes is off-topic here. I will only discuss this in the borg issue tracker.