-
-
Notifications
You must be signed in to change notification settings - Fork 556
Fixing matching issues in URL plugin #4191
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: dev
Are you sure you want to change the base?
Changes from all commits
be630f1
e9a68d2
d0a274a
77f81cf
a8e0d65
1872755
4942eab
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,73 @@ | ||||||||||||||
| using NUnit.Framework; | ||||||||||||||
| using NUnit.Framework.Legacy; | ||||||||||||||
| using Flow.Launcher.Plugin.Url; | ||||||||||||||
| using System.Reflection; | ||||||||||||||
|
|
||||||||||||||
| namespace Flow.Launcher.Test.Plugins | ||||||||||||||
| { | ||||||||||||||
| [TestFixture] | ||||||||||||||
| public class UrlPluginTest | ||||||||||||||
| { | ||||||||||||||
| [Test] | ||||||||||||||
| public void URLMatchTest() | ||||||||||||||
| private static Main plugin; | ||||||||||||||
|
|
||||||||||||||
| [OneTimeSetUp] | ||||||||||||||
| public void OneTimeSetup() | ||||||||||||||
| { | ||||||||||||||
| 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")); | ||||||||||||||
| var settingsProperty = typeof(Main).GetProperty("Settings", BindingFlags.NonPublic | BindingFlags.Static); | ||||||||||||||
| settingsProperty?.SetValue(null, new Settings()); | ||||||||||||||
|
|
||||||||||||||
| plugin = new Main(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| [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 26 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("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")] | ||||||||||||||
VictoriousRaptor marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| [TestCase("::1")] | ||||||||||||||
|
Comment on lines
+45
to
+47
|
||||||||||||||
| [TestCase("HTTP://EXAMPLE.COM")] | ||||||||||||||
| [TestCase("HTTPS://EXAMPLE.COM")] | ||||||||||||||
| [TestCase("EXAMPLE.COM")] | ||||||||||||||
| [TestCase("LOCALHOST")] | ||||||||||||||
|
||||||||||||||
| [TestCase("LOCALHOST")] | |
| [TestCase("LOCALHOST")] | |
| [TestCase("Http://Example.Com")] | |
| [TestCase("hTTps://ExAmPlE.CoM")] | |
| [TestCase("Example.Com")] | |
| [TestCase("LocalHost")] |
Copilot
AI
Feb 25, 2026
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.
The test cases should include scenarios with query parameters and fragments to ensure the URL validation handles them correctly. For example, add test cases like "192.168.1.1?query=value", "localhost:8080?test=123", "example.com#fragment", etc. These are common URL patterns that should be supported.
Copilot
AI
Feb 25, 2026
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 URLs with paths. The current test cases don't include scenarios like "example.com/path", "192.168.1.1/path/to/resource", or "localhost:8080/api/endpoint". These are common patterns that should be validated to ensure the path handling works correctly in both the validation logic and the Action method.
Copilot
AI
Feb 25, 2026
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.
While the comment states "Pattern excludes 0.0.0.0", it would be more accurate to say "Implementation excludes 0.0.0.0" since the validation is no longer pattern-based (regex) but rather uses IPAddress.Any comparison.
| [TestCase("0.0.0.0")] // Pattern excludes 0.0.0.0 | |
| [TestCase("0.0.0.0")] // Implementation excludes 0.0.0.0 via IPAddress.Any |
Copilot
AI
Feb 25, 2026
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.
The test cases for invalid URLs should include more edge cases to ensure robustness. Consider adding tests for: URLs with spaces (e.g., "example .com"), URLs with invalid characters, extremely long TLDs, domains starting with dots (e.g., "..example.com"), multiple consecutive dots (e.g., "example..com"), and malformed IPv6 addresses (e.g., "2001:db8:::1" with three colons).
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,56 +1,28 @@ | ||||||||||||||
| using System; | ||||||||||||||
| using System; | ||||||||||||||
| using System.Collections.Generic; | ||||||||||||||
| using System.Text.RegularExpressions; | ||||||||||||||
| using System.Linq; | ||||||||||||||
| using System.Net; | ||||||||||||||
| using System.Windows.Controls; | ||||||||||||||
| using Flow.Launcher.Plugin.SharedCommands; | ||||||||||||||
|
|
||||||||||||||
| namespace Flow.Launcher.Plugin.Url | ||||||||||||||
| { | ||||||||||||||
| public class Main : IPlugin, IPluginI18n, ISettingProvider | ||||||||||||||
| { | ||||||||||||||
| //based on https://gist.github.com/dperini/729294 | ||||||||||||||
| private const string UrlPattern = "^" + | ||||||||||||||
| // protocol identifier | ||||||||||||||
| "(?:(?:https?|ftp)://|)" + | ||||||||||||||
| // user:pass authentication | ||||||||||||||
| "(?:\\S+(?::\\S*)?@)?" + | ||||||||||||||
| "(?:" + | ||||||||||||||
| // 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]))" + | ||||||||||||||
| "|" + | ||||||||||||||
| // 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,}))" + | ||||||||||||||
| ")" + | ||||||||||||||
| // port number | ||||||||||||||
| "(?::\\d{2,5})?" + | ||||||||||||||
| // resource path | ||||||||||||||
| "(?:/\\S*)?" + | ||||||||||||||
| "$"; | ||||||||||||||
| 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 []; | ||||||||||||||
| } | ||||||||||||||
VictoriousRaptor marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
|
||||||||||||||
| return | ||||||||||||||
| [ | ||||||||||||||
| new() | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -60,7 +32,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))) | ||||||||||||||
Jack251970 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| { | ||||||||||||||
| raw = GetHttpPreference() + "://" + raw; | ||||||||||||||
VictoriousRaptor marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+35
to
39
|
||||||||||||||
|
|
@@ -92,9 +65,6 @@ public List<Result> Query(Query query) | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| ]; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return []; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private static string GetHttpPreference() | ||||||||||||||
|
|
@@ -104,19 +74,50 @@ private static string GetHttpPreference() | |||||||||||||
|
|
||||||||||||||
| 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(); | ||||||||||||||
|
|
||||||||||||||
| if (raw == "localhost" || raw.StartsWith("localhost:") || | ||||||||||||||
| raw == "http://localhost" || raw.StartsWith("http://localhost:") || | ||||||||||||||
| raw == "https://localhost" || raw.StartsWith("https://localhost:") | ||||||||||||||
| ) | ||||||||||||||
| { | ||||||||||||||
| // Exclude numbers (e.g. 1.2345) | ||||||||||||||
| if (decimal.TryParse(input, out _)) | ||||||||||||||
|
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. P1: Prompt for AI agents
Suggested change
|
||||||||||||||
| return false; | ||||||||||||||
|
Comment on lines
+82
to
+84
|
||||||||||||||
|
|
||||||||||||||
| // Check if it's a bare IP address with optional port and path | ||||||||||||||
| var ipPart = input.Split('/')[0]; // Remove path | ||||||||||||||
|
Comment on lines
+86
to
+87
|
||||||||||||||
| // Check if it's a bare IP address with optional port and path | |
| var ipPart = input.Split('/')[0]; // Remove path | |
| // Check if it's a bare IP address with optional port, path, query, or fragment | |
| var ipPart = input.Split('/', '?', '#')[0]; // Remove path, query, and fragment |
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.
P2: IPv6 "any" address (::) is not excluded, unlike its IPv4 counterpart 0.0.0.0. Typing :: would be incorrectly recognized as a valid URL. Add a check for IPAddress.IPv6Any for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Url/Main.cs, line 88:
<comment>IPv6 "any" address (`::`) is not excluded, unlike its IPv4 counterpart `0.0.0.0`. Typing `::` would be incorrectly recognized as a valid URL. Add a check for `IPAddress.IPv6Any` for consistency.</comment>
<file context>
@@ -104,19 +74,50 @@ private static string GetHttpPreference()
+
+ // Check if it's a bare IP address with optional port and path
+ var ipPart = input.Split('/')[0]; // Remove path
+ if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any))
return true;
- }
</file context>
| if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any)) | |
| if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any) && !endpoint.Address.Equals(IPAddress.IPv6Any)) |
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.
decimal.TryParse is culture-sensitive — may fail to filter plain numbers in non-dot-decimal locales.
decimal.TryParse(input, out _) uses CultureInfo.CurrentCulture. In locales where the decimal separator is a comma (e.g., de-DE), an input like "1.2345" won't parse as a decimal, will fall through to IPEndPoint.TryParse, and .NET will interpret it as a valid IP address (octet shorthand), returning true unexpectedly.
Use CultureInfo.InvariantCulture to ensure the guard works consistently:
Proposed fix
- if (decimal.TryParse(input, out _))
+ if (decimal.TryParse(input, System.Globalization.NumberStyles.Any, System.Globalization.CultureInfo.InvariantCulture, out _))
return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs` around lines 75 - 89, In IsURL, the
decimal parsing guard uses decimal.TryParse(input, out _) which is
culture-sensitive; update the call to use an invariant culture (e.g.,
decimal.TryParse with CultureInfo.InvariantCulture and appropriate NumberStyles)
so inputs like "1.2345" are parsed the same regardless of locale; modify the
decimal.TryParse call in the IsURL method to include NumberStyles.Number and
CultureInfo.InvariantCulture (retain the out discard) to ensure consistent
numeric detection before the IPEndPoint.TryParse 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.
IPv6 "any" address (::) is not excluded, unlike IPv4 0.0.0.0.
Line 88 only checks IPAddress.Any (i.e., 0.0.0.0). The IPv6 equivalent IPAddress.IPv6Any (::) is not excluded, so typing :: would be recognized as a valid URL. The same gap exists on line 111. Consider adding an IPv6Any check for symmetry:
Proposed fix
- if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any))
+ if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any) && !endpoint.Address.Equals(IPAddress.IPv6Any))
return true;- if (IPAddress.TryParse(host, out var hostIp))
- return !hostIp.Equals(IPAddress.Any);
+ if (IPAddress.TryParse(host, out var hostIp))
+ return !hostIp.Equals(IPAddress.Any) && !hostIp.Equals(IPAddress.IPv6Any);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs` around lines 86 - 89, The code
currently treats an endpoint as invalid only when endpoint.Address equals
IPAddress.Any (0.0.0.0); update the validation in the IPEndPoint.TryParse
branches (the logic using ipPart, IPEndPoint.TryParse and endpoint.Address) to
also exclude IPAddress.IPv6Any (::). In other words, change the check that
compares endpoint.Address to IPAddress.Any so it rejects both IPAddress.Any and
IPAddress.IPv6Any (e.g., endpoint.Address.Equals(IPAddress.Any) ||
endpoint.Address.Equals(IPAddress.IPv6Any)), and apply the same modification to
the similar check later in the file (the other IPEndPoint.TryParse usage).
Copilot
AI
Feb 25, 2026
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.
The validation accepts bare IPv6 addresses without brackets (e.g., "2001:db8::1", "::1") which is problematic because when these are passed to the Action method, they will be prepended with "http://" or "https://" resulting in invalid URLs like "http://2001:db8::1". IPv6 addresses in URLs must be enclosed in brackets. The Action method at line 38 simply prepends the protocol without adding brackets, which will create malformed URLs that browsers cannot open.
Copilot
AI
Feb 25, 2026
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.
The validation logic checks for IPv6 addresses with IPAddress.TryParse in two places: once for bare IP addresses (line 88) and again for hosts extracted from URIs (line 110). However, the check at line 110 excludes IPAddress.Any (0.0.0.0 or ::), but the check at line 88 also excludes IPAddress.Any. This means "::" would be rejected, but test case at line 47 expects "::1" (loopback) to be accepted. The logic appears correct for "::1" but be aware that "::" without brackets might not parse correctly in all contexts.
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.
P2: Same issue as the IPEndPoint check above: the IPv6 "any" address (::) is not excluded here either. IPAddress.IPv6Any should also be rejected to maintain symmetry with the 0.0.0.0 exclusion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Url/Main.cs, line 111:
<comment>Same issue as the `IPEndPoint` check above: the IPv6 "any" address (`::`) is not excluded here either. `IPAddress.IPv6Any` should also be rejected to maintain symmetry with the `0.0.0.0` exclusion.</comment>
<file context>
@@ -104,19 +74,50 @@ private static string GetHttpPreference()
+
+ // Valid IP address (excluding 0.0.0.0)
+ if (IPAddress.TryParse(host, out var hostIp))
+ return !hostIp.Equals(IPAddress.Any);
+
+ // Domain must have valid format with TLD
</file context>
| return !hostIp.Equals(IPAddress.Any); | |
| return !hostIp.Equals(IPAddress.Any) && !hostIp.Equals(IPAddress.IPv6Any); |
Copilot
AI
Feb 25, 2026
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.
The TLD validation logic requires that the TLD contains only letters and is at least 2 characters long. However, this will incorrectly reject valid domains with numeric TLDs or newer generic TLDs. While pure numeric TLDs are rare, some valid domains might have alphanumeric characters in their TLD, and this validation is too strict. Consider using a more permissive approach or at minimum allowing alphanumeric TLDs rather than only letters.
| // TLD must be at least 2 letters | |
| var tld = parts[^1]; | |
| return tld.Length >= 2 && tld.All(char.IsLetter); | |
| // TLD must be at least 2 characters, allowing letters and digits | |
| var tld = parts[^1]; | |
| return tld.Length >= 2 && tld.All(char.IsLetterOrDigit); |
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.
The removal of the NUnit.Framework.Legacy import and the switch from ClassicAssert to Assert.That deviates from the established testing conventions in this codebase. Other test files in the repository consistently use ClassicAssert (e.g., CalculatorTest.cs, FuzzyMatcherTest.cs, ExplorerTest.cs). Please restore the NUnit.Framework.Legacy import and use ClassicAssert.IsTrue and ClassicAssert.IsFalse instead of Assert.That for consistency.