[FEATURE] Support more url schemes and remove insecure ftp scheme#4537
Draft
DavidGBrett wants to merge 5 commits into
Draft
[FEATURE] Support more url schemes and remove insecure ftp scheme#4537DavidGBrett wants to merge 5 commits into
DavidGBrett wants to merge 5 commits into
Conversation
…brave", "opera", "vivaldi", "edge", "chrome", "chrome-extension", "moz-extension"
…and the previously added chromium schemes)
… Context.API.OpenUrl This ensures that it always opens in the browser - instead of letting the system decide which app should handle the URI This means that non standard schemes like chrome:// will be supported
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs:107">
P2: Bookmark actions now bypass Flow's configured browser launch settings and always force tab opening.</violation>
</file>
<file name="Plugins/Flow.Launcher.Plugin.Url/Main.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Url/Main.cs:146">
P1: Colon-scheme validation incorrectly treats `chrome://` as valid because `"//"` passes the non-empty content check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| var content = input[(colonIndex + 1)..]; | ||
| return content.Length > 0 && !content.Any(char.IsWhiteSpace); |
Contributor
There was a problem hiding this comment.
P1: Colon-scheme validation incorrectly treats chrome:// as valid because "//" passes the non-empty content check.
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 146:
<comment>Colon-scheme validation incorrectly treats `chrome://` as valid because `"//"` passes the non-empty content check.</comment>
<file context>
@@ -116,21 +135,37 @@ public bool IsURL(string raw)
+ if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase)))
+ {
+ var content = input[(colonIndex + 1)..];
+ return content.Length > 0 && !content.Any(char.IsWhiteSpace);
+ }
+ }
</file context>
Suggested change
| return content.Length > 0 && !content.Any(char.IsWhiteSpace); | |
| if (content.Length == 0 || content.Any(char.IsWhiteSpace)) | |
| return false; | |
| if (content.StartsWith("//", StringComparison.Ordinal)) | |
| return ChromiumSchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase)) && content.Length > 2; | |
| return true; |
| Action = _ => | ||
| { | ||
| Context.API.OpenUrl(c.Url); | ||
| SearchWeb.OpenInBrowserTab(c.Url); |
Contributor
There was a problem hiding this comment.
P2: Bookmark actions now bypass Flow's configured browser launch settings and always force tab opening.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs, line 107:
<comment>Bookmark actions now bypass Flow's configured browser launch settings and always force tab opening.</comment>
<file context>
@@ -104,7 +104,7 @@ public List<Result> Query(Query query)
Action = _ =>
{
- Context.API.OpenUrl(c.Url);
+ SearchWeb.OpenInBrowserTab(c.Url);
return true;
</file context>
Suggested change
| SearchWeb.OpenInBrowserTab(c.Url); | |
| Context.API.OpenUrl(c.Url); |
Contributor
📝 WalkthroughWalkthroughThe URL plugin's scheme handling is refactored from a single ChangesURL Plugin Scheme Overhaul and Bookmark Open Change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs`:
- Around line 138-148: In the colon-only schemes validation block within the
Main.cs file, the return statement for validating content after the colon does
not account for double-slash patterns. When checking schemes like `chrome` in
`chrome://settings`, the content extracted is `//settings`, which passes the
current validation (length > 0 and no whitespace). Add an additional check to
exclude content that starts with `//` from this colon-only validation path,
allowing it to fall through to the double-slash validation logic instead. Modify
the return statement to ensure that if content begins with `//`, the method
continues to the next validation path rather than returning true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d537a80-e08e-4b89-a2ff-1b7fb232e310
📒 Files selected for processing (3)
Flow.Launcher.Test/Plugins/UrlPluginTest.csPlugins/Flow.Launcher.Plugin.BrowserBookmark/Main.csPlugins/Flow.Launcher.Plugin.Url/Main.cs
Comment on lines
+138
to
+148
| // Check colon-only schemes (e.g. about:blank, chrome:settings) | ||
| var colonIndex = input.IndexOf(':'); | ||
| if (colonIndex > 0) | ||
| { | ||
| var scheme = input[..colonIndex]; | ||
| if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| var content = input[(colonIndex + 1)..]; | ||
| return content.Length > 0 && !content.Any(char.IsWhiteSpace); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: chrome:// incorrectly accepted as valid via colon-only path.
For Chromium schemes that support both forms (chrome:settings and chrome://settings), this check accepts chrome:// because content "//" passes the length and whitespace validation. The test at line 111 expects chrome:// to be invalid.
When content starts with //, defer to the double-slash validation path instead.
🐛 Proposed fix
// Check colon-only schemes (e.g. about:blank, chrome:settings)
var colonIndex = input.IndexOf(':');
if (colonIndex > 0)
{
var scheme = input[..colonIndex];
if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase)))
{
var content = input[(colonIndex + 1)..];
+ // Defer double-slash forms to URI-based validation below
+ if (!content.StartsWith("//"))
+ {
return content.Length > 0 && !content.Any(char.IsWhiteSpace);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check colon-only schemes (e.g. about:blank, chrome:settings) | |
| var colonIndex = input.IndexOf(':'); | |
| if (colonIndex > 0) | |
| { | |
| var scheme = input[..colonIndex]; | |
| if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase))) | |
| { | |
| var content = input[(colonIndex + 1)..]; | |
| return content.Length > 0 && !content.Any(char.IsWhiteSpace); | |
| } | |
| } | |
| // Check colon-only schemes (e.g. about:blank, chrome:settings) | |
| var colonIndex = input.IndexOf(':'); | |
| if (colonIndex > 0) | |
| { | |
| var scheme = input[..colonIndex]; | |
| if (ColonOnlySchemes.Any(s => scheme.Equals(s, StringComparison.OrdinalIgnoreCase))) | |
| { | |
| var content = input[(colonIndex + 1)..]; | |
| // Defer double-slash forms to URI-based validation below | |
| if (!content.StartsWith("//")) | |
| { | |
| return content.Length > 0 && !content.Any(char.IsWhiteSpace); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs` around lines 138 - 148, In the
colon-only schemes validation block within the Main.cs file, the return
statement for validating content after the colon does not account for
double-slash patterns. When checking schemes like `chrome` in
`chrome://settings`, the content extracted is `//settings`, which passes the
current validation (length > 0 and no whitespace). Add an additional check to
exclude content that starts with `//` from this colon-only validation path,
allowing it to fall through to the double-slash validation logic instead. Modify
the return statement to ensure that if content begins with `//`, the method
continues to the next validation path rather than returning true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #4535 - see it for more context
Also closes #4014
Summary by cubic
Expanded URL support to include browser-specific and colon-only schemes, and removed the insecure ftp scheme. Browser bookmarks now always open in a browser tab to ensure consistent handling.
Summary of changes
Release Note
You can now open browser-specific and file links (like chrome:// and about:blank) directly from Flow Launcher; ftp links are no longer supported.
Written for commit 19c600c. Summary will update on new commits.