Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
bernaferrari
force-pushed
the
marketplace
branch
from
c63492e to
0ac8225
Compare
| * Fetch extension settings for the authenticated user/organization. | ||
| * These settings include org-managed Marketplace policy fields. | ||
| */ | ||
| export async function fetchExtensionSettings( |
There was a problem hiding this comment.
I'm not sure where this came from, if it's in the plan or based no the current extension, but we dont actually have org settings in our current marketplace. Roo does, and the code is there in the kilocode extension, but that is actually dead. Can you remove all this organization checking logic?
There was a problem hiding this comment.
Yes, I'll get rid of that
markijbema
left a comment
markijbema
left a comment
There was a problem hiding this comment.
The org stuff shouldnt be there, and those are also the changes that span extension and cli-backend. Can you please revert those? Then there is some small stuff, but I can do that postmerge as well if you want (like adding other translations, which isnt hard but just costs a lot of tokens)
|
Yes, I even did that in a separate commit, but without them the marketplace won't work, is that OK? |
|
Why wouldnt it work, the marketplace is publicly available endpoints like http://app.kilo.ai/api/marketplace/mcps There was functionality in the old extension to disable certain parts of the marketplace for orgs (which never did anything in kilo, but was from roo), but the marketplace should just work fine as is? |
bernaferrari
force-pushed
the
marketplace
branch
from
0ac8225 to
bc046b9
Compare
|
I'm still a noob on this. Got it working. Thanks for your patience. I also fixed translations. I made another PR (much simpler) improving the profile UI while trying to not mess with server. |
bernaferrari
force-pushed
the
marketplace
branch
from
f82f51b to
57ca0d5
Compare
packages/app/src/i18n/ar.ts
Outdated
| "command.permissions.autoaccept.enable": "قبول التعديلات تلقائيًا", | ||
| "command.permissions.autoaccept.disable": "إيقاف قبول التعديلات تلقائيًا", | ||
| "command.workspace.toggle": "تبديل مساحات العمل", | ||
| "command.workspace.toggle.description": "Enable or disable multiple workspaces in the sidebar", |
There was a problem hiding this comment.
the 118n works like a cascade (like css); so we have translations in the extension, fall back to kilo i18n fall back to i18n, but i18n should never differ from upstream, so we dont want to make changes here, only in kilo i18n/vscode i18n.
We probably need to add some checks to make this clearer though as you couldnt've known
| backendBase, | ||
| DEFAULT_API_BASE_URL, | ||
| DEFAULT_BACKEND_BASE_URL, | ||
| ] |
There was a problem hiding this comment.
this seems too complex, but that might be due to our original extension being to complex here. We should avoid this now though
| } | ||
|
|
||
| async removeItem(item: MarketplaceItem, target: "project" | "global"): Promise<void> { | ||
| if (item.managedByOrganization) { |
| throw new Error(`Marketplace request failed for ${route}.${detail}`) | ||
| } | ||
|
|
||
| private parseStructured(text: string): unknown { |
There was a problem hiding this comment.
this seems to complex, just try to parse as json, or give up
| private getProjectKiloDirectoryPath(): string { | ||
| const workspaceDir = this.getWorkspaceDir() | ||
| const kiloDir = path.join(workspaceDir, ".kilocode") | ||
| const legacyDir = path.join(workspaceDir, ".roo") |
There was a problem hiding this comment.
we definitely dont want to keep supporting .roo dirs
| } | ||
| } | ||
| } catch { | ||
| // Fallback to item.id if content is invalid. |
There was a problem hiding this comment.
lets not do that, fail if it is invalid
|
|
||
| const config = { ...(input as Record<string, unknown>) } | ||
|
|
||
| if (config.command !== undefined) { |
There was a problem hiding this comment.
I don't think we need checks like this; kilo maintains the marketplace and this should be checked in the marketplace repo
| @@ -0,0 +1,17 @@ | |||
| declare module "tar-fs" { | |||
There was a problem hiding this comment.
i would expect we can import some types
| vscode.postMessage({ type: "requestMarketplaceData" }) | ||
| } | ||
|
|
||
| let lastOrganizationRefreshKey: string | undefined |
There was a problem hiding this comment.
I would expect this to be redundant now as well
| </Show> | ||
|
|
||
| <Show | ||
| when={!managedByOrganization} |
There was a problem hiding this comment.
this condition should be gone
|
I think it looks good overall, there is quite some notes i made but I'll take a stab at fixing them so we can get this merged |
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }) | ||
|
|
||
| requestData() | ||
| return () => unsubscribe() |
There was a problem hiding this comment.
WARNING: Memory leak — SolidJS onMount does not support cleanup return values
Unlike React's useEffect, SolidJS's onMount ignores the return value. This means () => unsubscribe() is never called and the message listener accumulates on every component mount/unmount cycle.
Use onCleanup instead:
| return () => unsubscribe() | |
| onCleanup(() => unsubscribe()) |
(You'll also need to remove the return and call onCleanup before the closing }) of onMount, or move the subscription setup into a createEffect with onCleanup.)
| type: z.literal("skill"), | ||
| category: z.string(), | ||
| githubUrl: z.string().url(), | ||
| content: z.string().min(1), |
There was a problem hiding this comment.
WARNING: Skill content field used as tarball download URL but not validated as a URL
In installSkill(), item.content is passed directly to fetch(tarballUrl) (marketplace-service.ts:650). However, this schema only validates it as z.string().min(1), not as a valid URL. If the catalog data were ever compromised, this could allow SSRF by pointing to internal network resources.
Consider changing to:
| content: z.string().min(1), | |
| content: z.string().url(), |
| return `${currentOrgId}|${orgNames}` | ||
| }) | ||
|
|
||
| createEffect(() => { |
There was a problem hiding this comment.
SUGGESTION: Telemetry event fires on initial render
This createEffect tracks activeTab() and fires a "Marketplace Tab Viewed" telemetry event. Since SolidJS effects run immediately on creation, this will fire for the default "mcp" tab on every component mount — even before the user has interacted with any tab. Consider guarding against the initial run if that's not intentional:
let initialized = false
createEffect(() => {
const tab = activeTab()
if (!initialized) {
initialized = true
return
}
vscode.postMessage({ ... })
})
Code Review SummaryStatus: No New Issues Found | Recommendation: Address existing comments before merge OverviewThis PR adds a full Marketplace feature to the VS Code extension, including:
The implementation is generally well-structured with good input validation (Zod schemas, safe tar path checking, marketplace ID sanitization) and proper error handling throughout. Existing Comments SummaryThe 20 existing inline comments cover the key issues. Several have already been addressed by subsequent commits (merge from dev). The remaining actionable items are:
Key Issues from Prior Review (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in code added by the merge commit (outside the PR diff range):
Files Reviewed (27 files)
|
Moved translation keys from packages/app/src/i18n/ to packages/kilo-vscode/webview-ui/src/i18n/ where they belong. Reverted app/src/i18n/ to match origin/dev baseline.
SolidJS onMount does not support cleanup return values like React's useEffect. The unsubscribe callback was never called, causing a memory leak. Use onCleanup() inside onMount instead.
The skill content field is used as a tarball download URL passed to fetch(). Validate it as z.string().url() instead of z.string().min(1) to prevent potential SSRF if catalog data were compromised.
SolidJS createEffect runs immediately on creation, which fires a Marketplace Tab Viewed telemetry event for the default tab before any user interaction. Guard the initial run so telemetry only fires on actual tab switches.
| private async installMcp(item: Extract<MarketplaceItem, { type: "mcp" }>, options: MarketplaceInstallOptions): Promise<void> { | ||
| const filePath = this.mcpFilePath(options.target) | ||
| const mcpId = this.sanitizeMarketplaceItemId(item.id, "MCP server") | ||
| const selectedFromParameters = options.parameters?._selectedIndex |
There was a problem hiding this comment.
WARNING: _selectedIndex parameter key collision with user-provided parameters
The fallback options.parameters?._selectedIndex reads from the same parameters object that contains user-provided MCP parameter values. If a marketplace MCP item defines a parameter with key _selectedIndex, the user's value for that parameter would be misinterpreted as an installation method index, silently selecting the wrong installation method.
Consider using a dedicated field on MarketplaceInstallOptions instead of overloading the parameters map, or at minimum filtering out _selectedIndex before iterating allParameters at line 580.
| }), | ||
| ) | ||
| } else { | ||
| setStatusMessage( |
There was a problem hiding this comment.
SUGGESTION: Redundant data re-fetch after successful install/remove
On success, this calls requestData() which sends requestMarketplaceData to the extension. However, the extension handler (handleInstallMarketplaceItem / handleRemoveMarketplaceItem) already calls this.handleRequestMarketplaceData() before posting the marketplaceActionResult message — so the webview has already received fresh marketplaceData by the time this code runs. This triggers a second unnecessary network fetch of the entire catalog.
| throw new Error("Skill item missing tarball URL") | ||
| } | ||
|
|
||
| const response = await fetch(tarballUrl) |
There was a problem hiding this comment.
WARNING: No URL scheme validation before fetching skill tarball
The tarballUrl (from item.content) is fetched without validating that it uses an https:// (or at minimum http://) scheme. While the URL originates from the marketplace API, a compromised or misconfigured catalog could supply file://, data:, or internal network URLs, potentially enabling SSRF or local file exfiltration.
Consider adding a scheme check before the fetch:
const parsed = new URL(tarballUrl)
if (parsed.protocol !== "https:" && parsed.protocol !== "http:") {
throw new Error(`Unsupported URL scheme for skill tarball: ${parsed.protocol}`)
}
| } | ||
|
|
||
| private replaceTemplateToken(content: string, key: string, value: string): string { | ||
| return content.split(`{{${key}}}`).join(value) |
There was a problem hiding this comment.
WARNING: replaceTemplateToken performs naive string substitution without JSON-escaping the user-supplied value.
If the MCP content template is JSON (e.g. {"args": ["{{api_key}}"]}), a user-supplied value containing JSON structural characters (quotes, brackets, etc.) can alter the parsed structure. For example, a value like foo", "injected would break out of the string context and inject additional array elements into args.
While sanitizeMcpServerConfig validates the types of known fields (command, args, env), it doesn't restrict unknown keys, so injected keys would pass through to the written config file.
Consider JSON-escaping the replacement value when the template content is JSON, or performing substitution on the parsed object rather than the raw string.
| await fs.mkdir(destination, { recursive: true }) | ||
|
|
||
| try { | ||
| const bytes = Buffer.from(await response.arrayBuffer()) |
There was a problem hiding this comment.
WARNING: No response size limit on tarball download — the entire response body is loaded into memory via response.arrayBuffer() before being written to disk.
A malformed or malicious catalog entry could point to an arbitrarily large file, causing memory exhaustion. Consider checking Content-Length header and enforcing a maximum size (e.g. 50 MB), or streaming the response directly to disk instead of buffering it entirely in memory.
|
@bernaferrari I tried it out, but i don't think it fully works yet. Can you please iterate on it first and make sure all the paths work? |
2 participants
There are a few minor differences with Kilo current extension:
If you want full fidelity I can do that too. I think project/globally is probably a feature that opencode provides, so not sure we want or not to use it.
The only thing missing: I need to translate some strings into every other language. I just want to be sure you like the PR before I do that.