fix(desktop/ipc): allowlist URL scheme protocols in openURLScheme (#5056)
The 'integration.openURLScheme' IPC method invokes 'shell.openExternal' with a renderer-supplied string after only checking that it contains '://'. Electron's documentation explicitly warns that passing untrusted URLs to 'shell.openExternal' is unsafe: schemes such as 'file://', 'smb://', 'ms-msdt:', 'search-ms:', 'jar:', 'res:', 'javascript:', 'data:' and 'vbscript:' have well-known abuse chains (local file disclosure, NTLM credential theft over SMB on Windows, MSDT/Follina-style RCE, etc.). Because the renderer process can also reach this IPC via any XSS sink in untrusted RSS feed content, the previous validation was not sufficient. Replace the substring check with strict URL parsing plus an allowlist of protocols that match the integration use-cases documented in the UI (Obsidian, Bear, Drafts, Things, Notion, DEVONthink) plus generic http/https/mailto. All other protocols are rejected with a clear error. Adds vitest cases for representative dangerous schemes (verifying that 'shell.openExternal' is never invoked) and for every scheme shipped as a built-in example, so future regressions on either side are caught.
This commit is contained in:
parent
2350884eae
commit
bd91b015ee
|
|
@ -1,8 +1,9 @@
|
|||
import fsp from "node:fs/promises"
|
||||
import os from "node:os"
|
||||
|
||||
import { shell } from "electron"
|
||||
import path from "pathe"
|
||||
import { afterEach, describe, expect, it, vi } from "vitest"
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
|
||||
|
||||
import { IntegrationService } from "./integration"
|
||||
|
||||
|
|
@ -69,4 +70,72 @@ describe("IntegrationService", () => {
|
|||
fsp.stat(path.join(vaultPath, "KAWA DESIGN 少女前线2:追放 索米·雪兔献礼 1")),
|
||||
).rejects.toThrow()
|
||||
})
|
||||
|
||||
describe("openURLScheme", () => {
|
||||
const openExternalMock = vi.mocked(shell.openExternal)
|
||||
|
||||
beforeEach(() => {
|
||||
openExternalMock.mockReset()
|
||||
openExternalMock.mockResolvedValue()
|
||||
})
|
||||
|
||||
it("rejects input that cannot be parsed as a URL", async () => {
|
||||
const service = new IntegrationService()
|
||||
|
||||
await expect(service.openURLScheme("not-a-url")).rejects.toThrow(
|
||||
/Invalid URL scheme/i,
|
||||
)
|
||||
expect(openExternalMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// These are the dangerous protocols that previously slipped through the
|
||||
// "contains ://" guard and reached shell.openExternal verbatim.
|
||||
// shell.openExternal docs explicitly warn that passing untrusted URLs is
|
||||
// unsafe — file://, smb://, search-ms:, ms-msdt:, jar:, res:, etc. have
|
||||
// been used in real-world RCE / NTLM-credential-theft chains.
|
||||
it.each([
|
||||
["file:///etc/passwd"],
|
||||
["FILE:///etc/passwd"],
|
||||
["smb://attacker.example/share"],
|
||||
["jar:http://attacker.example/x.jar!/"],
|
||||
["res://shell32.dll/1"],
|
||||
["ms-msdt:/id PCWDiagnostic"],
|
||||
["search-ms:query=secret"],
|
||||
["javascript:alert(1)"],
|
||||
["data:text/html,<script>alert(1)</script>"],
|
||||
["vbscript:msgbox(1)"],
|
||||
])(
|
||||
"blocks dangerous scheme %s and does not invoke shell.openExternal",
|
||||
async (dangerousScheme) => {
|
||||
const service = new IntegrationService()
|
||||
|
||||
await expect(service.openURLScheme(dangerousScheme)).rejects.toThrow(
|
||||
/not allowed|disallowed|not permitted/i,
|
||||
)
|
||||
expect(openExternalMock).not.toHaveBeenCalled()
|
||||
},
|
||||
)
|
||||
|
||||
// The integration UI ships these schemes as built-in examples
|
||||
// (see url-scheme-handler.ts#getExamples) plus generic web/mail.
|
||||
// They must keep working after the fix.
|
||||
it.each([
|
||||
["https://example.com"],
|
||||
["http://example.com/path?q=1"],
|
||||
["mailto:user@example.com"],
|
||||
["obsidian://new?vault=MyVault&name=Test"],
|
||||
["bear://x-callback-url/create?title=Test"],
|
||||
["things:///add?title=Test"],
|
||||
["notion://new?title=Test"],
|
||||
["x-devonthink://createText?title=Test"],
|
||||
["drafts://x-callback-url/create?text=Test"],
|
||||
])("permits known integration scheme %s", async (allowedScheme) => {
|
||||
const service = new IntegrationService()
|
||||
|
||||
await expect(service.openURLScheme(allowedScheme)).resolves.toEqual({
|
||||
success: true,
|
||||
})
|
||||
expect(openExternalMock).toHaveBeenCalledWith(allowedScheme)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -81,6 +81,29 @@ export async function saveMediaToEagle(input: SaveToEagleInput): Promise<any> {
|
|||
}
|
||||
}
|
||||
|
||||
// Allowlist of URL scheme protocols that `openURLScheme` is permitted to hand
|
||||
// off to `shell.openExternal`. The list intentionally covers the integrations
|
||||
// shipped in the UI (Obsidian, Bear, Drafts, Things, Notion, DEVONthink) plus
|
||||
// generic web/mail schemes, while excluding dangerous protocols such as
|
||||
// `file:`, `smb:`, `ms-msdt:`, `search-ms:`, `jar:`, `res:`, `javascript:`,
|
||||
// `data:`, `vbscript:`, which have known abuse chains when invoked from
|
||||
// untrusted content.
|
||||
const ALLOWED_URL_SCHEME_PROTOCOLS = new Set<string>([
|
||||
"http",
|
||||
"https",
|
||||
"mailto",
|
||||
"obsidian",
|
||||
"bear",
|
||||
"drafts",
|
||||
"things",
|
||||
"notion",
|
||||
"x-devonthink",
|
||||
])
|
||||
|
||||
function isAllowedURLSchemeProtocol(protocol: string): boolean {
|
||||
return ALLOWED_URL_SCHEME_PROTOCOLS.has(protocol)
|
||||
}
|
||||
|
||||
export class IntegrationService extends IpcService {
|
||||
static override readonly groupName = "integration"
|
||||
|
||||
|
|
@ -382,11 +405,32 @@ ${content}
|
|||
const requestId = Math.random().toString(36).slice(2, 8)
|
||||
|
||||
try {
|
||||
// Validate URL scheme format
|
||||
if (!scheme.includes("://")) {
|
||||
// Parse and validate the protocol up-front. `shell.openExternal` will
|
||||
// happily dispatch any scheme the OS has registered a handler for,
|
||||
// including `file://`, `smb://`, `ms-msdt:`, `search-ms:`, `jar:`,
|
||||
// `res:`, etc. Several of those have well-documented exploit chains
|
||||
// (NTLM credential theft over SMB, MSDT/Follina RCE on Windows,
|
||||
// local-file disclosure via file://). The Electron docs explicitly
|
||||
// warn against passing untrusted URLs to `shell.openExternal`, so we
|
||||
// enforce a strict allowlist of schemes that the integrations UI is
|
||||
// intended to support.
|
||||
let protocol: string
|
||||
try {
|
||||
protocol = new URL(scheme).protocol.replace(/:$/, "").toLowerCase()
|
||||
} catch {
|
||||
throw new Error("Invalid URL scheme format. Must include protocol (e.g., 'app://')")
|
||||
}
|
||||
|
||||
if (!protocol) {
|
||||
throw new Error("Invalid URL scheme format. Must include protocol (e.g., 'app://')")
|
||||
}
|
||||
|
||||
if (!isAllowedURLSchemeProtocol(protocol)) {
|
||||
throw new Error(
|
||||
`URL scheme "${protocol}://" is not allowed. Allowed schemes: ${[...ALLOWED_URL_SCHEME_PROTOCOLS].sort().join(", ")}.`,
|
||||
)
|
||||
}
|
||||
|
||||
// Log URL scheme execution (mask sensitive data)
|
||||
const safeScheme = scheme.replaceAll(/(\?|&)([^=]+)=([^&]+)/g, (_, prefix, key, value) =>
|
||||
// Mask potential sensitive query parameters
|
||||
|
|
@ -399,7 +443,7 @@ ${content}
|
|||
|
||||
logger.info(`[URLScheme:${requestId}] Opening URL scheme`, {
|
||||
scheme: safeScheme,
|
||||
protocol: scheme.split("://")[0],
|
||||
protocol,
|
||||
})
|
||||
|
||||
// Use Electron's shell.openExternal to open URL scheme
|
||||
|
|
|
|||
Loading…
Reference in New Issue