-
-
Notifications
You must be signed in to change notification settings - Fork 544
Update UrlPattern in URL plugin to include private addresses and update test cases #4185
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
Changes from all commits
be630f1
e9a68d2
d0a274a
77f81cf
8971d04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,84 @@ | ||
| using NUnit.Framework; | ||
| using NUnit.Framework.Legacy; | ||
| using System; | ||
| using System.Reflection; | ||
| using Flow.Launcher.Plugin.Url; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace Flow.Launcher.Test.Plugins | ||
| { | ||
| [TestFixture] | ||
| public class UrlPluginTest | ||
| { | ||
| [Test] | ||
| public void URLMatchTest() | ||
| private Settings _settings = new() | ||
| { | ||
| var plugin = new Main(); | ||
| ClassicAssert.IsTrue(plugin.IsURL("http://www.google.com")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("https://www.google.com")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("http://google.com")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("www.google.com")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("google.com")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("http://localhost")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("https://localhost")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("http://localhost:80")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("https://localhost:80")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("http://110.10.10.10")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("110.10.10.10")); | ||
| ClassicAssert.IsTrue(plugin.IsURL("ftp://110.10.10.10")); | ||
| AlwaysOpenWithHttps = false | ||
| }; | ||
|
|
||
| private readonly Main _plugin = new(); | ||
|
|
||
| ClassicAssert.IsFalse(plugin.IsURL("wwww")); | ||
| ClassicAssert.IsFalse(plugin.IsURL("wwww.c")); | ||
| ClassicAssert.IsFalse(plugin.IsURL("wwww.c")); | ||
| public UrlPluginTest() | ||
| { | ||
| // Set the static Settings property before running tests | ||
| var settingProperty = typeof(Main).GetProperty("Settings", BindingFlags.NonPublic | BindingFlags.Static); | ||
| if (settingProperty == null) | ||
| Assert.Fail("Could not find property 'Settings' on Flow.Launcher.Plugin.Url.Main"); | ||
| settingProperty.SetValue(null, _settings); | ||
| } | ||
|
|
||
| [TestCase("http://www.google.com")] | ||
| [TestCase("https://www.google.com")] | ||
| [TestCase("http://google.com")] | ||
| [TestCase("ftp://google.com")] | ||
| [TestCase("www.google.com")] | ||
| [TestCase("google.com")] | ||
|
Check warning on line 32 in Flow.Launcher.Test/Plugins/UrlPluginTest.cs
|
||
| [TestCase("http://localhost")] | ||
| [TestCase("https://localhost")] | ||
| [TestCase("http://localhost:80")] | ||
| [TestCase("https://localhost:80")] | ||
| [TestCase("localhost")] | ||
| [TestCase("localhost:8080")] | ||
| [TestCase("http://110.10.10.10")] | ||
| [TestCase("110.10.10.10")] | ||
| [TestCase("110.10.10.10:8080")] | ||
| [TestCase("192.168.1.1")] | ||
| [TestCase("[email protected]")] | ||
| [TestCase("[email protected]:1080")] | ||
| [TestCase("root:[email protected]:1080")] | ||
| [TestCase("192.168.1.1:3000")] | ||
| [TestCase("ftp://110.10.10.10")] | ||
| [TestCase("[2001:db8::1]")] | ||
| [TestCase("[2001:db8::1]:8080")] | ||
| [TestCase("http://[2001:db8::1]")] | ||
| [TestCase("https://[2001:db8::1]:8080")] | ||
| [TestCase("[::1]")] | ||
| [TestCase("[::1]:8080")] | ||
| [TestCase("2001:db8::1")] | ||
| [TestCase("fe80:1:2::3:4")] | ||
| [TestCase("::1")] | ||
| [TestCase("HTTP://EXAMPLE.COM")] | ||
| [TestCase("HTTPS://EXAMPLE.COM")] | ||
| [TestCase("EXAMPLE.COM")] | ||
| [TestCase("EXAMPLE.COM/index.html")] | ||
| [TestCase("LOCALHOST")] | ||
| public void WhenValidUrlThenIsUrlReturnsTrue(string url) | ||
| { | ||
|
|
||
| Assert.That(_plugin.IsURL(url), Is.True); | ||
| } | ||
|
|
||
| [TestCase("wwww")] | ||
| [TestCase("wwww.c")] | ||
| [TestCase("not a url")] | ||
| [TestCase("just text")] | ||
| [TestCase("http://")] | ||
| [TestCase("://example.com")] | ||
| [TestCase("0.0.0.0")] // Pattern excludes 0.0.0.0 | ||
| [TestCase("256.1.1.1")] // Invalid IPv4 | ||
| [TestCase("example")] // No TLD | ||
| [TestCase(".com")] | ||
| [TestCase("http://.com")] | ||
| public void WhenInvalidUrlThenIsUrlReturnsFalse(string url) | ||
| { | ||
| Assert.That(_plugin.IsURL(url), Is.False); | ||
| } | ||
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||
| using System; | ||||||||||||||
| using System; | ||||||||||||||
| using System.Collections.Generic; | ||||||||||||||
| using System.Linq; | ||||||||||||||
| using System.Text.RegularExpressions; | ||||||||||||||
| using System.Windows.Controls; | ||||||||||||||
| using Flow.Launcher.Plugin.SharedCommands; | ||||||||||||||
|
|
@@ -8,49 +9,48 @@ namespace Flow.Launcher.Plugin.Url | |||||||||||||
| { | ||||||||||||||
| public class Main : IPlugin, IPluginI18n, ISettingProvider | ||||||||||||||
| { | ||||||||||||||
| //based on https://gist.github.com/dperini/729294 | ||||||||||||||
| // Simplified pattern to quickly reject obviously invalid inputs | ||||||||||||||
| // Actual validation is done by Uri.TryCreate which properly handles all URL formats | ||||||||||||||
| private const string UrlPattern = "^" + | ||||||||||||||
| // protocol identifier | ||||||||||||||
| "(?:(?:https?|ftp)://|)" + | ||||||||||||||
| // user:pass authentication | ||||||||||||||
| "(?:\\S+(?::\\S*)?@)?" + | ||||||||||||||
| // Optional protocol | ||||||||||||||
| "(?:(?:https?|ftp)://)?" + | ||||||||||||||
| // Optional user authentication | ||||||||||||||
| "(?:[^@\\s]+@)?" + | ||||||||||||||
| // Must contain at least one of: | ||||||||||||||
| "(?:" + | ||||||||||||||
| // IP address exclusion | ||||||||||||||
| // private & local networks | ||||||||||||||
| "(?!(?:10|127)(?:\\.\\d{1,3}){3})" + | ||||||||||||||
| "(?!(?:169\\.254|192\\.168)(?:\\.\\d{1,3}){2})" + | ||||||||||||||
| "(?!172\\.(?:1[6-9]|2\\d|3[0-1])(?:\\.\\d{1,3}){2})" + | ||||||||||||||
| // IP address dotted notation octets | ||||||||||||||
| // excludes loopback network 0.0.0.0 | ||||||||||||||
| // excludes reserved space >= 224.0.0.0 | ||||||||||||||
| // excludes network & broacast addresses | ||||||||||||||
| // (first & last IP address of each class) | ||||||||||||||
| "(?:[1-9]\\d?|1\\d\\d|2[01]\\d|22[0-3])" + | ||||||||||||||
| "(?:\\.(?:1?\\d{1,2}|2[0-4]\\d|25[0-5])){2}" + | ||||||||||||||
| "(?:\\.(?:[1-9]\\d?|1\\d\\d|2[0-4]\\d|25[0-4]))" + | ||||||||||||||
| // - protocol with something after it | ||||||||||||||
| "(?:(?:https?|ftp)://).+" + | ||||||||||||||
| "|" + | ||||||||||||||
| // host name | ||||||||||||||
| "(?:(?:[a-z\\u00a1-\\uffff0-9]-*)*[a-z\\u00a1-\\uffff0-9]+)" + | ||||||||||||||
| // domain name | ||||||||||||||
| "(?:\\.(?:[a-z\\u00a1-\\uffff0-9]-*)*[a-z\\u00a1-\\uffff0-9]+)*" + | ||||||||||||||
| // TLD identifier | ||||||||||||||
| "(?:\\.(?:[a-z\\u00a1-\\uffff]{2,}))" + | ||||||||||||||
| // - IPv6 address (simplified detection - just look for colons in brackets or multiple colons) | ||||||||||||||
| "(?:\\[[0-9a-fA-F:]+\\]|[0-9a-fA-F]*:[0-9a-fA-F:]+)" + | ||||||||||||||
| "|" + | ||||||||||||||
| // - IPv4 address (basic pattern) | ||||||||||||||
| "(?:[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})" + | ||||||||||||||
| "|" + | ||||||||||||||
| // - localhost | ||||||||||||||
| "localhost" + | ||||||||||||||
| "|" + | ||||||||||||||
| // - domain with TLD (at least one dot with valid characters) | ||||||||||||||
| "(?:[a-z0-9-]+\\.)+[a-z]{2,}" + | ||||||||||||||
| ")" + | ||||||||||||||
| // port number | ||||||||||||||
| "(?::\\d{2,5})?" + | ||||||||||||||
| // resource path | ||||||||||||||
| "(?:/\\S*)?" + | ||||||||||||||
| "$"; | ||||||||||||||
| // Optional port and path | ||||||||||||||
| "(?::[0-9]{1,5})?(?:/.*)?$"; | ||||||||||||||
|
|
||||||||||||||
| private readonly Regex UrlRegex = new(UrlPattern, RegexOptions.Compiled | RegexOptions.IgnoreCase); | ||||||||||||||
| internal static PluginInitContext Context { get; private set; } | ||||||||||||||
| internal static Settings Settings { get; private set; } | ||||||||||||||
|
|
||||||||||||||
| private static readonly string[] UrlSchemes = ["http://", "https://", "ftp://"]; | ||||||||||||||
|
|
||||||||||||||
| public List<Result> Query(Query query) | ||||||||||||||
| { | ||||||||||||||
| var raw = query.Search; | ||||||||||||||
| if (IsURL(raw)) | ||||||||||||||
| if (!IsURL(raw)) | ||||||||||||||
| { | ||||||||||||||
| return | ||||||||||||||
| return []; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return | ||||||||||||||
| [ | ||||||||||||||
| new() | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -60,7 +60,8 @@ public List<Result> Query(Query query) | |||||||||||||
| Score = 8, | ||||||||||||||
| Action = _ => | ||||||||||||||
| { | ||||||||||||||
| if (!raw.StartsWith("http://", StringComparison.OrdinalIgnoreCase) && !raw.StartsWith("https://", StringComparison.OrdinalIgnoreCase)) | ||||||||||||||
| // not a recognized scheme, add preferred http scheme | ||||||||||||||
| if (!UrlSchemes.Any(scheme => raw.StartsWith(scheme, StringComparison.OrdinalIgnoreCase))) | ||||||||||||||
| { | ||||||||||||||
| raw = GetHttpPreference() + "://" + raw; | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+63
to
67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Bare IPv6 addresses won't open correctly. The Action handler adds the scheme to bare IPv6 addresses without wrapping them in brackets. For input The 🔎 Proposed fix {
// not a recognized scheme, add preferred http scheme
if (!UrlSchemes.Any(scheme => raw.StartsWith(scheme, StringComparison.OrdinalIgnoreCase)))
{
- raw = GetHttpPreference() + "://" + raw;
+ // Wrap bare IPv6 addresses in brackets
+ if (raw.Count(c => c == ':') > 1 && !raw.Contains("://") && !raw.Contains('@') && !raw.StartsWith('['))
+ {
+ // Check if there's already a port pattern (]:port)
+ var portMatch = Regex.Match(raw, @"\]:(\d{1,5})$");
+ if (!portMatch.Success)
+ {
+ raw = $"{GetHttpPreference()}://[{raw}]";
+ }
+ else
+ {
+ raw = GetHttpPreference() + "://" + raw;
+ }
+ }
+ else
+ {
+ raw = GetHttpPreference() + "://" + raw;
+ }
}Alternatively, extract the bracket-wrapping logic into a helper method to avoid duplication with 🤖 Prompt for AI Agents |
||||||||||||||
|
|
@@ -92,31 +93,77 @@ public List<Result> Query(Query query) | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| ]; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return []; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private static string GetHttpPreference() | ||||||||||||||
| private string GetHttpPreference() | ||||||||||||||
| { | ||||||||||||||
| return Settings.AlwaysOpenWithHttps ? "https" : "http"; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public bool IsURL(string raw) | ||||||||||||||
| { | ||||||||||||||
| raw = raw.ToLower(); | ||||||||||||||
| if (string.IsNullOrWhiteSpace(raw)) | ||||||||||||||
| return false; | ||||||||||||||
|
|
||||||||||||||
| if (UrlRegex.Match(raw).Value == raw) return true; | ||||||||||||||
| var input = raw.Trim(); | ||||||||||||||
|
|
||||||||||||||
| // Quick pre-filter with regex to reject obviously invalid inputs | ||||||||||||||
| if (!UrlRegex.IsMatch(input)) | ||||||||||||||
| return false; | ||||||||||||||
|
|
||||||||||||||
| if (raw == "localhost" || raw.StartsWith("localhost:") || | ||||||||||||||
| raw == "http://localhost" || raw.StartsWith("http://localhost:") || | ||||||||||||||
| raw == "https://localhost" || raw.StartsWith("https://localhost:") | ||||||||||||||
| ) | ||||||||||||||
| // Pre-validate IPv4 addresses (Uri accepts invalid octets like 256) | ||||||||||||||
| // Match pattern: optional scheme/auth + IPv4 + optional port/path | ||||||||||||||
| var ipv4Match = Regex.Match(input, @"(?:(?:https?|ftp)://)?(?:[^@/]+@)?(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})(?:[:/]|$)"); | ||||||||||||||
| if (ipv4Match.Success) | ||||||||||||||
| { | ||||||||||||||
| return true; | ||||||||||||||
| var octets = ipv4Match.Groups[1].Value.Split('.'); | ||||||||||||||
| foreach (var octet in octets) | ||||||||||||||
| { | ||||||||||||||
| if (!byte.TryParse(octet, out _)) | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (ipv4Match.Groups[1].Value == "0.0.0.0") | ||||||||||||||
| return false; | ||||||||||||||
|
Comment on lines
+126
to
+127
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reconsider excluding 0.0.0.0. The exclusion of 🔎 Proposed fix if 0.0.0.0 should be allowed if (!byte.TryParse(octet, out _))
return false;
}
-
- if (ipv4Match.Groups[1].Value == "0.0.0.0")
- return false;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Prepare URL for Uri.TryCreate validation | ||||||||||||||
| var urlToValidate = input; | ||||||||||||||
|
|
||||||||||||||
| // Add scheme if missing | ||||||||||||||
| if (!UrlSchemes.Any(s => input.StartsWith(s, StringComparison.OrdinalIgnoreCase))) | ||||||||||||||
| { | ||||||||||||||
| // Handle bare IPv6 addresses (multiple colons, no @ for auth, no :// for scheme) | ||||||||||||||
| if (input.Count(c => c == ':') > 1 && !input.Contains("://") && !input.Contains('@')) | ||||||||||||||
| { | ||||||||||||||
| // Add brackets if not already present | ||||||||||||||
| if (!input.StartsWith('[')) | ||||||||||||||
| { | ||||||||||||||
| // Check if there's a port at the end (]:port pattern) | ||||||||||||||
| var portMatch = Regex.Match(input, @"\]:(\d{1,5})$"); | ||||||||||||||
| if (!portMatch.Success) | ||||||||||||||
| { | ||||||||||||||
| urlToValidate = $"{GetHttpPreference()}://[{input}]"; | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| urlToValidate = GetHttpPreference() + "://" + input; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| urlToValidate = GetHttpPreference() + "://" + input; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| urlToValidate = GetHttpPreference() + "://" + input; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return false; | ||||||||||||||
| // Use Uri.TryCreate for comprehensive validation | ||||||||||||||
| return Uri.TryCreate(urlToValidate, UriKind.Absolute, out var uri) | ||||||||||||||
| && (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps || uri.Scheme == Uri.UriSchemeFtp); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public void Init(PluginInitContext context) | ||||||||||||||
|
|
||||||||||||||
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.
Missing test coverage for invalid port numbers. The regex pattern allows port numbers up to 99999 (5 digits), but valid TCP/UDP ports range from 0 to 65535. Consider adding test cases like "example.com:99999" or "192.168.1.1:70000" to verify they are correctly rejected or accepted based on the intended behavior.