From a22ed6b8093b5d855f03affdb1eae6d1c3cfb9bd Mon Sep 17 00:00:00 2001 From: Jarek Krochmalski Date: Sun, 6 Sep 2026 17:03:47 +0530 Subject: [PATCH] resolve secret references pasted with surrounding quotes --- src/lib/server/secretproviders/azure-kv.ts | 4 +- src/lib/server/secretproviders/connect.ts | 4 +- src/lib/server/secretproviders/keepass.ts | 3 +- src/lib/server/secretproviders/proton.ts | 3 +- .../server/secretproviders/service-account.ts | 3 +- src/lib/server/secretproviders/shared.ts | 14 +++ tests/provider-reference-quotes.test.ts | 105 ++++++++++++++++++ 7 files changed, 129 insertions(+), 7 deletions(-) create mode 100644 tests/provider-reference-quotes.test.ts diff --git a/src/lib/server/secretproviders/azure-kv.ts b/src/lib/server/secretproviders/azure-kv.ts index 01d40e5..03895a8 100644 --- a/src/lib/server/secretproviders/azure-kv.ts +++ b/src/lib/server/secretproviders/azure-kv.ts @@ -13,7 +13,7 @@ import { request } from 'undici'; import type { AzureKvConfig, SecretProvider, TestConnectionResult } from './shared'; -import { assertSafeProviderHost, isJsonResponse } from './shared'; +import { assertSafeProviderHost, isJsonResponse, stripSurroundingQuotes } from './shared'; const API_VERSION = '7.4'; const AZURE_REF_PREFIX = 'azurekv://'; @@ -140,7 +140,7 @@ export const azureKvProvider: SecretProvider = { supportsBulk: true, isReference(value: unknown): value is string { - return typeof value === 'string' && AZURE_REF_RE.test(value.trim()); + return typeof value === 'string' && AZURE_REF_RE.test(stripSurroundingQuotes(value)); }, async testConnection(config: AzureKvConfig): Promise { diff --git a/src/lib/server/secretproviders/connect.ts b/src/lib/server/secretproviders/connect.ts index 902c152..5bffd8d 100644 --- a/src/lib/server/secretproviders/connect.ts +++ b/src/lib/server/secretproviders/connect.ts @@ -15,7 +15,7 @@ import { request } from 'undici'; import type { ConnectConfig, SecretProvider, TestConnectionResult } from './shared'; -import { assertSafeProviderHost, parseProviderError, isJsonResponse } from './shared'; +import { assertSafeProviderHost, parseProviderError, isJsonResponse, stripSurroundingQuotes } from './shared'; import { UnsupportedOperationError } from './shared'; /** A vault as returned by GET /v1/vaults. */ @@ -228,7 +228,7 @@ export const connectProvider: SecretProvider = { supportsBulk: false, isReference(value: unknown): value is string { - return typeof value === 'string' && value.trim().startsWith('op://'); + return typeof value === 'string' && stripSurroundingQuotes(value).startsWith('op://'); }, async testConnection(config: ConnectConfig): Promise { diff --git a/src/lib/server/secretproviders/keepass.ts b/src/lib/server/secretproviders/keepass.ts index ee26d4f..43026ac 100644 --- a/src/lib/server/secretproviders/keepass.ts +++ b/src/lib/server/secretproviders/keepass.ts @@ -20,6 +20,7 @@ import { spawn } from 'node:child_process'; import { isAbsolute } from 'node:path'; import type { KeePassConfig, SecretProvider, TestConnectionResult } from './shared'; +import { stripSurroundingQuotes } from './shared'; const DEFAULT_CLI_PATH = '/usr/bin/keepassxc-cli'; const COMMAND_TIMEOUT_MS = 30_000; @@ -260,7 +261,7 @@ export const keepassProvider: SecretProvider = { supportsBulk: true, isReference(value: unknown): value is string { - return typeof value === 'string' && value.trim().startsWith(KEEPASS_PREFIX); + return typeof value === 'string' && stripSurroundingQuotes(value).startsWith(KEEPASS_PREFIX); }, async testConnection(config: KeePassConfig): Promise { diff --git a/src/lib/server/secretproviders/proton.ts b/src/lib/server/secretproviders/proton.ts index d894448..07b32db 100644 --- a/src/lib/server/secretproviders/proton.ts +++ b/src/lib/server/secretproviders/proton.ts @@ -28,6 +28,7 @@ import { chmod, mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { isAbsolute, join } from 'node:path'; import type { ProtonConfig, SecretProvider, TestConnectionResult } from './shared'; +import { stripSurroundingQuotes } from './shared'; const DEFAULT_PASS_CLI_PATH = '/usr/local/bin/pass-cli'; const LOGIN_TIMEOUT_MS = 30_000; @@ -368,7 +369,7 @@ export const protonProvider: SecretProvider = { supportsBulk: true, isReference(value: unknown): value is string { - return typeof value === 'string' && value.trim().startsWith('pass://'); + return typeof value === 'string' && stripSurroundingQuotes(value).startsWith('pass://'); }, async testConnection(config: ProtonConfig): Promise { diff --git a/src/lib/server/secretproviders/service-account.ts b/src/lib/server/secretproviders/service-account.ts index 09e2c7e..1584362 100644 --- a/src/lib/server/secretproviders/service-account.ts +++ b/src/lib/server/secretproviders/service-account.ts @@ -15,6 +15,7 @@ import { createClient } from '@1password/sdk'; import type { SecretProvider, ServiceAccountConfig, TestConnectionResult } from './shared'; +import { stripSurroundingQuotes } from './shared'; const INTEGRATION_NAME = 'Dockhand'; const INTEGRATION_VERSION = '1.0.0'; @@ -49,7 +50,7 @@ export const serviceAccountProvider: SecretProvider = { supportsBulk: true, isReference(value: unknown): value is string { - return typeof value === 'string' && value.trim().startsWith('op://'); + return typeof value === 'string' && stripSurroundingQuotes(value).startsWith('op://'); }, async testConnection({ token }: ServiceAccountConfig): Promise { diff --git a/src/lib/server/secretproviders/shared.ts b/src/lib/server/secretproviders/shared.ts index bcd78f1..2d91e34 100644 --- a/src/lib/server/secretproviders/shared.ts +++ b/src/lib/server/secretproviders/shared.ts @@ -48,6 +48,20 @@ export type SecretProviderType = * self-hosted Vault/Infisical/Connect on the local network still works. Throws on * an unsafe host; call it before the first request in every REST provider. */ +/** + * Strip ONE layer of matching surrounding quotes for reference DETECTION only. + * 1Password's "Copy Secret Reference" puts `"op://Vault/Item/field"` on the clipboard + * (quotes included), so a pasted value fails a bare `startsWith('op://')` test and the + * reference is silently skipped (#1521). Providers normalize with this before the prefix + * test, and the resolver uses it to key the reference - the STORED value is left untouched + * (so #1086's "stop stripping quotes on save" still holds). Only strips when both ends are + * the same quote char; leaves a value with mismatched/one-sided quotes as-is. + */ +export function stripSurroundingQuotes(value: string): string { + // \1 backreference: strip only when the SAME quote char wraps both ends. + return value.trim().replace(/^(["'])(.*)\1$/s, '$2'); +} + export function assertSafeProviderHost(rawUrl: string, label: string): void { const safe = isSafeNotificationUrl(rawUrl); if (!safe.ok) { diff --git a/tests/provider-reference-quotes.test.ts b/tests/provider-reference-quotes.test.ts new file mode 100644 index 0000000..88e8035 --- /dev/null +++ b/tests/provider-reference-quotes.test.ts @@ -0,0 +1,105 @@ +/** + * #1521: op:// / pass:// references pasted with surrounding quotes (1Password's + * "Copy Secret Reference" clipboard format) must still be detected as references. + * stripSurroundingQuotes normalizes for the DETECTION/lookup test only - the stored + * value is left untouched (keeps #1086's "don't strip quotes on save"). + */ + +import { describe, test, expect } from 'bun:test'; +import { stripSurroundingQuotes } from '../src/lib/server/secretproviders/shared'; +// NOTE: do NOT import service-account.ts here - it loads @1password/sdk (a native +// module) at import time, which the unit-test env cannot resolve. Its isReference is +// the identical `stripSurroundingQuotes(value).startsWith('op://')` as connectProvider, +// which IS tested below; the pure helper carries the real quote logic. +import { connectProvider } from '../src/lib/server/secretproviders/connect'; +import { protonProvider } from '../src/lib/server/secretproviders/proton'; +import { keepassProvider } from '../src/lib/server/secretproviders/keepass'; +import { azureKvProvider } from '../src/lib/server/secretproviders/azure-kv'; + +describe('stripSurroundingQuotes', () => { + test('strips matching double quotes', () => { + expect(stripSurroundingQuotes('"op://Vault/Item/field"')).toBe('op://Vault/Item/field'); + }); + test('strips matching single quotes', () => { + expect(stripSurroundingQuotes("'op://Vault/Item/field'")).toBe('op://Vault/Item/field'); + }); + test('trims whitespace before stripping', () => { + expect(stripSurroundingQuotes(' "op://v/i/f" ')).toBe('op://v/i/f'); + }); + test('leaves an unquoted value unchanged (just trimmed)', () => { + expect(stripSurroundingQuotes('op://v/i/f')).toBe('op://v/i/f'); + }); + test('leaves mismatched / one-sided quotes unchanged', () => { + expect(stripSurroundingQuotes(`"op://v/i/f'`)).toBe(`"op://v/i/f'`); + expect(stripSurroundingQuotes('"op://v/i/f')).toBe('"op://v/i/f'); + }); + test('does not strip quotes that are not surrounding', () => { + expect(stripSurroundingQuotes('no"quote"inside')).toBe('no"quote"inside'); + }); + test('empty quoted string collapses to empty', () => { + expect(stripSurroundingQuotes('""')).toBe(''); + }); + test('a lone quote char is not a pair', () => { + expect(stripSurroundingQuotes('"')).toBe('"'); + }); + test('strips quotes around a MULTILINE value (pins the /s dotall flag)', () => { + // Without the `s` flag, `.` would not match the newline and the quotes would + // survive - a multi-line secret (e.g. a PEM) pasted with quotes must still strip. + expect(stripSurroundingQuotes('"line1\nline2"')).toBe('line1\nline2'); + }); +}); + +describe('op:// isReference tolerates surrounding quotes (#1521)', () => { + for (const [name, p] of [ + ['connect', connectProvider], + ] as const) { + test(`${name}: detects a bare op:// reference`, () => { + expect(p.isReference('op://Vault/Item/field')).toBe(true); + }); + test(`${name}: detects a double-quoted op:// reference`, () => { + expect(p.isReference('"op://Vault/Item/field"')).toBe(true); + }); + test(`${name}: detects a single-quoted op:// reference`, () => { + expect(p.isReference("'op://Vault/Item/field'")).toBe(true); + }); + test(`${name}: a plain value is not a reference`, () => { + expect(p.isReference('just-a-token')).toBe(false); + expect(p.isReference('"just-a-token"')).toBe(false); + }); + } +}); + +describe('proton pass:// isReference tolerates surrounding quotes (#1521)', () => { + test('detects bare + quoted pass:// references', () => { + expect(protonProvider.isReference('pass://Vault/Item/field')).toBe(true); + expect(protonProvider.isReference('"pass://Vault/Item/field"')).toBe(true); + expect(protonProvider.isReference("'pass://Vault/Item/field'")).toBe(true); + }); + test('a plain value is not a reference', () => { + expect(protonProvider.isReference('"nope"')).toBe(false); + }); +}); + +describe('keepass:// isReference tolerates surrounding quotes (#1521)', () => { + test('detects bare + quoted keepass:// references', () => { + expect(keepassProvider.isReference('keepass://Database/Entry/Password')).toBe(true); + expect(keepassProvider.isReference('"keepass://Database/Entry/Password"')).toBe(true); + expect(keepassProvider.isReference("'keepass://Database/Entry/Password'")).toBe(true); + }); + test('a plain value is not a reference', () => { + expect(keepassProvider.isReference('"nope"')).toBe(false); + }); +}); + +describe('azurekv:// isReference tolerates surrounding quotes (#1521)', () => { + // azure-kv refs are a single segment: azurekv://. The anchored regex broke + // on the leading quote before the fix. + test('detects bare + quoted azurekv:// references', () => { + expect(azureKvProvider.isReference('azurekv://db-password')).toBe(true); + expect(azureKvProvider.isReference('"azurekv://db-password"')).toBe(true); + expect(azureKvProvider.isReference("'azurekv://db-password'")).toBe(true); + }); + test('a plain value is not a reference', () => { + expect(azureKvProvider.isReference('"nope"')).toBe(false); + }); +});