mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-10-06 02:47:56 +00:00
fix: implement profile-aware secret storage for VSCode profile isolation (#5442)
- Add profile-specific secret key generation using machineId, appName, and uriScheme - Implement automatic migration from legacy global secrets to profile-specific storage - Ensure Qdrant API keys and other secrets are isolated between VSCode profiles (local, WSL, etc.) - Add comprehensive tests for profile isolation and migration functionality - Maintain backward compatibility with existing secret storage Fixes #5442
This commit is contained in:
Roo Code
parent
ad201cc058
commit
3845f34f26
2 changed files with 226 additions and 17 deletions
|
|
@ -1,5 +1,6 @@
|
|||
import * as vscode from "vscode"
|
||||
import { ZodError } from "zod"
|
||||
import * as crypto from "crypto"
|
||||
|
||||
import {
|
||||
PROVIDER_SETTINGS_KEYS,
|
||||
|
|
@ -39,12 +40,39 @@ export class ContextProxy {
|
|||
private stateCache: GlobalState
|
||||
private secretCache: SecretState
|
||||
private _isInitialized = false
|
||||
private _profileId: string
|
||||
|
||||
constructor(context: vscode.ExtensionContext) {
|
||||
this.originalContext = context
|
||||
this.stateCache = {}
|
||||
this.secretCache = {}
|
||||
this._isInitialized = false
|
||||
this._profileId = this.generateProfileId()
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a unique profile identifier based on VSCode's environment
|
||||
* This ensures secrets are isolated per VSCode profile (local, WSL, etc.)
|
||||
*/
|
||||
private generateProfileId(): string {
|
||||
// Use a combination of machineId and environment-specific identifiers
|
||||
const machineId = vscode.env.machineId
|
||||
const appName = vscode.env.appName
|
||||
const uriScheme = vscode.env.uriScheme
|
||||
|
||||
// Create a hash of these identifiers to create a unique profile ID
|
||||
const profileData = `${machineId}-${appName}-${uriScheme}`
|
||||
const hash = crypto.createHash("sha256").update(profileData).digest("hex")
|
||||
|
||||
// Use first 16 characters for a shorter but still unique identifier
|
||||
return hash.substring(0, 16)
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a profile-specific secret key to ensure secrets are isolated per VSCode profile
|
||||
*/
|
||||
private getProfileSpecificSecretKey(key: SecretStateKey): string {
|
||||
return `${this._profileId}:${key}`
|
||||
}
|
||||
|
||||
public get isInitialized() {
|
||||
|
|
@ -63,7 +91,24 @@ export class ContextProxy {
|
|||
|
||||
const promises = SECRET_STATE_KEYS.map(async (key) => {
|
||||
try {
|
||||
this.secretCache[key] = await this.originalContext.secrets.get(key)
|
||||
// Use profile-specific key for secrets to ensure profile isolation
|
||||
const profileSpecificKey = this.getProfileSpecificSecretKey(key)
|
||||
let secretValue = await this.originalContext.secrets.get(profileSpecificKey)
|
||||
|
||||
// Migration: If profile-specific secret doesn't exist, check for legacy global secret
|
||||
if (!secretValue) {
|
||||
const legacyValue = await this.originalContext.secrets.get(key)
|
||||
if (legacyValue) {
|
||||
// Migrate legacy secret to profile-specific storage
|
||||
await this.originalContext.secrets.store(profileSpecificKey, legacyValue)
|
||||
// Clean up legacy secret to prevent cross-profile leakage
|
||||
await this.originalContext.secrets.delete(key)
|
||||
secretValue = legacyValue
|
||||
logger.info(`Migrated secret ${key} to profile-specific storage`)
|
||||
}
|
||||
}
|
||||
|
||||
this.secretCache[key] = secretValue
|
||||
} catch (error) {
|
||||
logger.error(`Error loading secret ${key}: ${error instanceof Error ? error.message : String(error)}`)
|
||||
}
|
||||
|
|
@ -141,10 +186,13 @@ export class ContextProxy {
|
|||
// Update cache.
|
||||
this.secretCache[key] = value
|
||||
|
||||
// Write directly to context.
|
||||
// Use profile-specific key for secrets to ensure profile isolation
|
||||
const profileSpecificKey = this.getProfileSpecificSecretKey(key)
|
||||
|
||||
// Write directly to context with profile-specific key.
|
||||
return value === undefined
|
||||
? this.originalContext.secrets.delete(key)
|
||||
: this.originalContext.secrets.store(key, value)
|
||||
? this.originalContext.secrets.delete(profileSpecificKey)
|
||||
: this.originalContext.secrets.store(profileSpecificKey, value)
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -154,7 +202,24 @@ export class ContextProxy {
|
|||
async refreshSecrets(): Promise<void> {
|
||||
const promises = SECRET_STATE_KEYS.map(async (key) => {
|
||||
try {
|
||||
this.secretCache[key] = await this.originalContext.secrets.get(key)
|
||||
// Use profile-specific key for secrets to ensure profile isolation
|
||||
const profileSpecificKey = this.getProfileSpecificSecretKey(key)
|
||||
let secretValue = await this.originalContext.secrets.get(profileSpecificKey)
|
||||
|
||||
// Migration: If profile-specific secret doesn't exist, check for legacy global secret
|
||||
if (!secretValue) {
|
||||
const legacyValue = await this.originalContext.secrets.get(key)
|
||||
if (legacyValue) {
|
||||
// Migrate legacy secret to profile-specific storage
|
||||
await this.originalContext.secrets.store(profileSpecificKey, legacyValue)
|
||||
// Clean up legacy secret to prevent cross-profile leakage
|
||||
await this.originalContext.secrets.delete(key)
|
||||
secretValue = legacyValue
|
||||
logger.info(`Migrated secret ${key} to profile-specific storage during refresh`)
|
||||
}
|
||||
}
|
||||
|
||||
this.secretCache[key] = secretValue
|
||||
} catch (error) {
|
||||
logger.error(
|
||||
`Error refreshing secret ${key}: ${error instanceof Error ? error.message : String(error)}`,
|
||||
|
|
@ -284,7 +349,11 @@ export class ContextProxy {
|
|||
|
||||
await Promise.all([
|
||||
...GLOBAL_STATE_KEYS.map((key) => this.originalContext.globalState.update(key, undefined)),
|
||||
...SECRET_STATE_KEYS.map((key) => this.originalContext.secrets.delete(key)),
|
||||
...SECRET_STATE_KEYS.map((key) => {
|
||||
// Use profile-specific key for secrets to ensure profile isolation
|
||||
const profileSpecificKey = this.getProfileSpecificSecretKey(key)
|
||||
return this.originalContext.secrets.delete(profileSpecificKey)
|
||||
}),
|
||||
])
|
||||
|
||||
await this.initialize()
|
||||
|
|
|
|||
|
|
@ -15,6 +15,11 @@ vi.mock("vscode", () => ({
|
|||
Production: 2,
|
||||
Test: 3,
|
||||
},
|
||||
env: {
|
||||
machineId: "test-machine-id",
|
||||
appName: "Visual Studio Code",
|
||||
uriScheme: "vscode",
|
||||
},
|
||||
}))
|
||||
|
||||
describe("ContextProxy", () => {
|
||||
|
|
@ -76,10 +81,11 @@ describe("ContextProxy", () => {
|
|||
}
|
||||
})
|
||||
|
||||
it("should initialize secret cache with all secret keys", () => {
|
||||
it("should initialize secret cache with all secret keys using profile-specific keys", () => {
|
||||
expect(mockSecrets.get).toHaveBeenCalledTimes(SECRET_STATE_KEYS.length)
|
||||
for (const key of SECRET_STATE_KEYS) {
|
||||
expect(mockSecrets.get).toHaveBeenCalledWith(key)
|
||||
// Should use profile-specific key format: profileId:originalKey
|
||||
expect(mockSecrets.get).toHaveBeenCalledWith(expect.stringMatching(new RegExp(`^[a-f0-9]{16}:${key}$`)))
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
@ -191,22 +197,22 @@ describe("ContextProxy", () => {
|
|||
})
|
||||
|
||||
describe("storeSecret", () => {
|
||||
it("should store secret directly in original context", async () => {
|
||||
it("should store secret directly in original context with profile-specific key", async () => {
|
||||
await proxy.storeSecret("apiKey", "new-secret")
|
||||
|
||||
// Should have called original context
|
||||
expect(mockSecrets.store).toHaveBeenCalledWith("apiKey", "new-secret")
|
||||
// Should have called original context with profile-specific key
|
||||
expect(mockSecrets.store).toHaveBeenCalledWith(expect.stringMatching(/^[a-f0-9]{16}:apiKey$/), "new-secret")
|
||||
|
||||
// Should have stored the value in cache
|
||||
const storedValue = await proxy.getSecret("apiKey")
|
||||
expect(storedValue).toBe("new-secret")
|
||||
})
|
||||
|
||||
it("should handle undefined value for secret deletion", async () => {
|
||||
it("should handle undefined value for secret deletion with profile-specific key", async () => {
|
||||
await proxy.storeSecret("apiKey", undefined)
|
||||
|
||||
// Should have called delete on original context
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey")
|
||||
// Should have called delete on original context with profile-specific key
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith(expect.stringMatching(/^[a-f0-9]{16}:apiKey$/))
|
||||
|
||||
// Should have stored undefined in cache
|
||||
const storedValue = await proxy.getSecret("apiKey")
|
||||
|
|
@ -391,7 +397,7 @@ describe("ContextProxy", () => {
|
|||
expect(mockGlobalState.update).toHaveBeenCalledTimes(expectedUpdateCalls)
|
||||
})
|
||||
|
||||
it("should delete all secrets", async () => {
|
||||
it("should delete all secrets using profile-specific keys", async () => {
|
||||
// Setup initial secrets
|
||||
await proxy.storeSecret("apiKey", "test-api-key")
|
||||
await proxy.storeSecret("openAiApiKey", "test-openai-key")
|
||||
|
|
@ -399,9 +405,11 @@ describe("ContextProxy", () => {
|
|||
// Reset all state
|
||||
await proxy.resetAllState()
|
||||
|
||||
// Should have called delete for each key
|
||||
// Should have called delete for each key with profile-specific format
|
||||
for (const key of SECRET_STATE_KEYS) {
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith(key)
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith(
|
||||
expect.stringMatching(new RegExp(`^[a-f0-9]{16}:${key}$`)),
|
||||
)
|
||||
}
|
||||
|
||||
// Total calls should equal the number of secret keys
|
||||
|
|
@ -419,4 +427,136 @@ describe("ContextProxy", () => {
|
|||
expect(initializeSpy).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe("profile-aware secret storage", () => {
|
||||
it("should generate consistent profile IDs", () => {
|
||||
// Create multiple instances and verify they generate the same profile ID
|
||||
const proxy1 = new ContextProxy(mockContext)
|
||||
const proxy2 = new ContextProxy(mockContext)
|
||||
|
||||
// Access private method for testing
|
||||
const profileId1 = (proxy1 as any).generateProfileId()
|
||||
const profileId2 = (proxy2 as any).generateProfileId()
|
||||
|
||||
expect(profileId1).toBe(profileId2)
|
||||
expect(profileId1).toMatch(/^[a-f0-9]{16}$/)
|
||||
})
|
||||
|
||||
it("should create profile-specific secret keys", () => {
|
||||
const profileSpecificKey = (proxy as any).getProfileSpecificSecretKey("codeIndexQdrantApiKey")
|
||||
|
||||
expect(profileSpecificKey).toMatch(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/)
|
||||
})
|
||||
|
||||
it("should migrate legacy secrets to profile-specific storage during initialization", async () => {
|
||||
// Setup mock to return legacy secret on first call, undefined on profile-specific call
|
||||
mockSecrets.get
|
||||
.mockImplementationOnce((key: string) => {
|
||||
// Profile-specific key call returns undefined (no existing profile-specific secret)
|
||||
if (key.includes(":")) return Promise.resolve(undefined)
|
||||
// Legacy key call returns the legacy value
|
||||
return Promise.resolve("legacy-qdrant-key")
|
||||
})
|
||||
.mockImplementation((key: string) => {
|
||||
// Subsequent calls for legacy key return the legacy value
|
||||
if (!key.includes(":")) return Promise.resolve("legacy-qdrant-key")
|
||||
return Promise.resolve(undefined)
|
||||
})
|
||||
|
||||
// Create new proxy to trigger initialization
|
||||
const newProxy = new ContextProxy(mockContext)
|
||||
await newProxy.initialize()
|
||||
|
||||
// Should have attempted to get profile-specific key first
|
||||
expect(mockSecrets.get).toHaveBeenCalledWith(expect.stringMatching(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/))
|
||||
|
||||
// Should have attempted to get legacy key when profile-specific wasn't found
|
||||
expect(mockSecrets.get).toHaveBeenCalledWith("codeIndexQdrantApiKey")
|
||||
|
||||
// Should have stored the migrated value with profile-specific key
|
||||
expect(mockSecrets.store).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/),
|
||||
"legacy-qdrant-key",
|
||||
)
|
||||
|
||||
// Should have deleted the legacy key
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith("codeIndexQdrantApiKey")
|
||||
})
|
||||
|
||||
it("should migrate legacy secrets during refreshSecrets", async () => {
|
||||
// Setup mock to simulate legacy secret exists
|
||||
mockSecrets.get.mockImplementation((key: string) => {
|
||||
if (key.includes(":")) return Promise.resolve(undefined) // No profile-specific secret
|
||||
if (key === "codeIndexQdrantApiKey") return Promise.resolve("legacy-refresh-key")
|
||||
return Promise.resolve(undefined)
|
||||
})
|
||||
|
||||
await proxy.refreshSecrets()
|
||||
|
||||
// Should have migrated the legacy secret
|
||||
expect(mockSecrets.store).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/),
|
||||
"legacy-refresh-key",
|
||||
)
|
||||
expect(mockSecrets.delete).toHaveBeenCalledWith("codeIndexQdrantApiKey")
|
||||
})
|
||||
|
||||
it("should not migrate when profile-specific secret already exists", async () => {
|
||||
// Setup mock to return existing profile-specific secret
|
||||
mockSecrets.get.mockImplementation((key: string) => {
|
||||
if (key.includes(":codeIndexQdrantApiKey")) return Promise.resolve("existing-profile-key")
|
||||
return Promise.resolve(undefined)
|
||||
})
|
||||
|
||||
// Create new proxy to trigger initialization
|
||||
const newProxy = new ContextProxy(mockContext)
|
||||
await newProxy.initialize()
|
||||
|
||||
// Should not have attempted migration since profile-specific secret exists
|
||||
expect(mockSecrets.store).not.toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/),
|
||||
expect.any(String),
|
||||
)
|
||||
expect(mockSecrets.delete).not.toHaveBeenCalledWith("codeIndexQdrantApiKey")
|
||||
})
|
||||
|
||||
it("should isolate secrets between different profile environments", () => {
|
||||
// Mock different VSCode environments
|
||||
const wslEnv = {
|
||||
machineId: "test-machine-id",
|
||||
appName: "Visual Studio Code",
|
||||
uriScheme: "vscode-remote",
|
||||
}
|
||||
|
||||
const localEnv = {
|
||||
machineId: "test-machine-id",
|
||||
appName: "Visual Studio Code",
|
||||
uriScheme: "vscode",
|
||||
}
|
||||
|
||||
// Mock vscode.env for different environments
|
||||
vi.mocked(vscode.env).machineId = wslEnv.machineId
|
||||
vi.mocked(vscode.env).appName = wslEnv.appName
|
||||
vi.mocked(vscode.env).uriScheme = wslEnv.uriScheme
|
||||
|
||||
const wslProxy = new ContextProxy(mockContext)
|
||||
const wslProfileId = (wslProxy as any).generateProfileId()
|
||||
|
||||
vi.mocked(vscode.env).uriScheme = localEnv.uriScheme
|
||||
|
||||
const localProxy = new ContextProxy(mockContext)
|
||||
const localProfileId = (localProxy as any).generateProfileId()
|
||||
|
||||
// Profile IDs should be different for different environments
|
||||
expect(wslProfileId).not.toBe(localProfileId)
|
||||
|
||||
// Secret keys should be different
|
||||
const wslSecretKey = (wslProxy as any).getProfileSpecificSecretKey("codeIndexQdrantApiKey")
|
||||
const localSecretKey = (localProxy as any).getProfileSpecificSecretKey("codeIndexQdrantApiKey")
|
||||
|
||||
expect(wslSecretKey).not.toBe(localSecretKey)
|
||||
expect(wslSecretKey).toMatch(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/)
|
||||
expect(localSecretKey).toMatch(/^[a-f0-9]{16}:codeIndexQdrantApiKey$/)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue