search

gm-vm / openfortivpn-webview

MIT License
87 stars 19 forks source link

Support newer fortigates #34

Closed agouin closed 1 month ago

agouin commented 1 month ago

Newer Fortigate SSLVPNs do not have .html in the url, but have .../sslvpn/portal/ instead. This makes it backwards compatible.

gm-vm commented 1 month ago

Hi, thanks for this. I have some questions and observations.

The title of the PR mentions "NetworkManager-fortisslvpn". Does that really support passing cookies now? Why is this change helping that specifically? The regex change seems to be something anybody using an updated Fortigate might need.

As for the regex, how about something like (/sslvpn/portal/|/sslvpn/portal\\.html), or even a more compact /sslvpn/portal(/|\\.html), maybe with something even more specific for the new path? Is the new path just ../sslvpn/portal/? I known the one you suggested covers both cases, but I wonder if it may end up being a bit too generic and cause the application to print the cookie too early. I do not remember what old Fortigates do, even though I probably used the exact path just in case, but it's been a while since I wrote that.

I would also update the README and remove "If no regular expression is specified, the application will look for URLs containing /sslvpn/portal.html." from there. Mentioning --url-regex should be enough and one can read the default from the help output of the application.

As for the last change, I actually made SVPNCOOKIE= optional in openfortivpn, but I think openconnect still requires it, so this would be a breaking change for its users.

agouin commented 1 month ago

Thank you for the quick review!

I named the PR as such because NetworkManager-fortisslvpn was the use case I targeted for this, see https://gitlab.gnome.org/GNOME/NetworkManager-fortisslvpn/-/merge_requests/34 . I am happy to rename it. "Support newer fortigates" is probably a better title.

I think this can be slimmed to simply updating the regex, I will test that out and update the PR.

agouin commented 1 month ago

It works with the cookie name prefix, so I removed that change. The regex has also been updated to be more strict as suggested.

Is the new path just ../sslvpn/portal/

Yes, it ends with the slash following portal.

I confirmed that the latest commit works with the NetworkManager-fortisslvpn feature also.

gm-vm commented 1 month ago

Thanks!