From 5e49868f0aed3dacb0bd94361056f315094cb275 Mon Sep 17 00:00:00 2001 From: Oliver Faust <90081405+lidonius1122@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:42:28 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=91=20fix:=20Decode=20Action=20OAuth?= =?UTF-8?q?=20Credentials=20After=20Decryption=20(#14698)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔑 fix: Decode Action OAuth Credentials After Decryption Action client credentials are URL-encoded before encryption, but the OAuth token exchange decrypted them with `decryptV2` alone, sending values like `abc%40domain.com` to the provider and causing 401s for secrets containing `@`, `+`, `=`, `/` or `:`. Decoding falls back to the raw decrypted value so legacy secrets stored before 299cabd6e (which contain an unescaped `%`) still work. Fixes #14636 * 🔑 fix: Share Action Credential Codec Between Read Paths The encode-before-encrypt convention had two readers with different behaviour. `ActionService.decryptSensitiveValue` decoded unconditionally, so a credential stored before encoding was introduced (299cabd6e, March 2025) containing a stray `%` threw `URIError` and failed every read of the action, not just the token exchange. Extract the pair into `packages/api/src/actions/crypto.ts` as the single implementation and consume it from both `ActionService` and the OAuth token exchange, so the two paths cannot drift again. Tests cover the codec against real crypto: reserved characters and literal percent escapes round-trip, pre-encoding values are returned unchanged, and a pre-encoding value with a stray `%` no longer throws. A version marker on newly encrypted values was prototyped and dropped: it changes the stored format for every action credential while fixing nothing, since values written by the encoding convention already decode correctly and pre-encoding values stay unmarked either way. --------- Co-authored-by: Danny Avila --- api/server/services/ActionService.js | 25 +---- packages/api/src/actions/crypto.spec.ts | 59 ++++++++++ packages/api/src/actions/crypto.ts | 29 +++++ packages/api/src/actions/index.ts | 1 + packages/api/src/oauth/tokens.spec.ts | 138 ++++++++++++++++++++++++ packages/api/src/oauth/tokens.ts | 11 +- 6 files changed, 236 insertions(+), 27 deletions(-) create mode 100644 packages/api/src/actions/crypto.spec.ts create mode 100644 packages/api/src/actions/crypto.ts diff --git a/api/server/services/ActionService.js b/api/server/services/ActionService.js index 2c877c515e..85b3d45cdc 100644 --- a/api/server/services/ActionService.js +++ b/api/server/services/ActionService.js @@ -2,7 +2,7 @@ const jwt = require('jsonwebtoken'); const { nanoid } = require('nanoid'); const { GraphEvents, sleep } = require('@librechat/agents'); const { tool } = require('@librechat/agents/langchain/tools'); -const { logger, encryptV2, decryptV2 } = require('@librechat/data-schemas'); +const { logger, decryptV2 } = require('@librechat/data-schemas'); const { sendEvent, logAxiosError, @@ -10,6 +10,8 @@ const { refreshAccessToken, GenerationJobManager, createSSRFSafeAgents, + encryptSensitiveValue, + decryptSensitiveValue, validateActionOAuthMetadata, } = require('@librechat/api'); const { @@ -407,27 +409,6 @@ async function createActionTool({ }; } -/** - * Encrypts a sensitive value. - * @param {string} value - * @returns {Promise} - */ -async function encryptSensitiveValue(value) { - // Encode API key to handle special characters like ":" - const encodedValue = encodeURIComponent(value); - return await encryptV2(encodedValue); -} - -/** - * Decrypts a sensitive value. - * @param {string} value - * @returns {Promise} - */ -async function decryptSensitiveValue(value) { - const decryptedValue = await decryptV2(value); - return decodeURIComponent(decryptedValue); -} - /** * Encrypts sensitive metadata values for an action. * diff --git a/packages/api/src/actions/crypto.spec.ts b/packages/api/src/actions/crypto.spec.ts new file mode 100644 index 0000000000..af1a0693ba --- /dev/null +++ b/packages/api/src/actions/crypto.spec.ts @@ -0,0 +1,59 @@ +process.env.CREDS_KEY = + process.env.CREDS_KEY ?? '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'; + +// Loaded via dynamic import in beforeAll so encryption initializes after +// CREDS_KEY is set above (encryptV2 reads the key at module load). +let encryptSensitiveValue: typeof import('./crypto').encryptSensitiveValue; +let decryptSensitiveValue: typeof import('./crypto').decryptSensitiveValue; +let encryptV2: typeof import('@librechat/data-schemas').encryptV2; + +beforeAll(async () => { + ({ encryptSensitiveValue, decryptSensitiveValue } = await import('./crypto')); + ({ encryptV2 } = await import('@librechat/data-schemas')); +}); + +describe('action credential encryption', () => { + describe('round trip', () => { + it.each([ + ['an email-style client id', 'client@id.com'], + ['reserved characters', 's+cret=/end'], + ['a colon', 'user:pass'], + ['a literal percent sign', '100%secret'], + ['a plus sign that must not become a space', 'a+b'], + ['a literal percent escape', 'secret%2Fvalue'], + ['unicode', 'sécret-π'], + ])('preserves %s', async (_label, value) => { + expect(await decryptSensitiveValue(await encryptSensitiveValue(value))).toBe(value); + }); + }); + + describe('credentials stored before encoding was introduced', () => { + it('returns a pre-encoding value unchanged', async () => { + const stored = await encryptV2('plain-secret'); + + expect(await decryptSensitiveValue(stored)).toBe('plain-secret'); + }); + + /** + * Regression: decoding unconditionally threw `URIError` here, which broke every read of the + * action rather than just mangling the credential. + */ + it('does not throw on a pre-encoding value containing a stray percent sign', async () => { + const stored = await encryptV2('100%secret'); + + await expect(decryptSensitiveValue(stored)).resolves.toBe('100%secret'); + }); + + /** + * Known, pre-existing limitation rather than desired behaviour: stored values carry no marker + * saying whether they were encoded, so a pre-encoding credential that happens to contain a + * valid escape is indistinguishable from an encoded one and is still decoded. Re-saving the + * action rewrites it in the encoded format and settles the ambiguity. + */ + it('still rewrites a pre-encoding value that happens to contain a valid escape', async () => { + const stored = await encryptV2('secret%2Fvalue'); + + expect(await decryptSensitiveValue(stored)).toBe('secret/value'); + }); + }); +}); diff --git a/packages/api/src/actions/crypto.ts b/packages/api/src/actions/crypto.ts new file mode 100644 index 0000000000..85f1121113 --- /dev/null +++ b/packages/api/src/actions/crypto.ts @@ -0,0 +1,29 @@ +import { encryptV2, decryptV2 } from '@librechat/data-schemas'; + +/** + * Encrypts an action credential, encoding it first so that reserved characters (`@`, `+`, `=`, + * `/`, `:`) survive storage. + */ +export async function encryptSensitiveValue(value: string): Promise { + return encryptV2(encodeURIComponent(value)); +} + +/** + * Decrypts an action credential, reversing the encoding applied by {@link encryptSensitiveValue}. + * + * Encoding before encryption was introduced in 299cabd6e (March 2025). Credentials stored earlier + * were encrypted raw and carry no marker saying so, so a legacy value containing a stray `%` would + * make `decodeURIComponent` throw `URIError`; those are returned as decrypted. + * + * Every reader of these fields must use this helper. Decoding in one read path but not another + * sends different credentials to the provider depending on which path ran. + */ +export async function decryptSensitiveValue(encryptedValue: string): Promise { + const decryptedValue = await decryptV2(encryptedValue); + + try { + return decodeURIComponent(decryptedValue); + } catch { + return decryptedValue; + } +} diff --git a/packages/api/src/actions/index.ts b/packages/api/src/actions/index.ts index d0ecba2ce2..1204d803f8 100644 --- a/packages/api/src/actions/index.ts +++ b/packages/api/src/actions/index.ts @@ -1,4 +1,5 @@ export * from './credentials'; +export * from './crypto'; export * from './protection'; export * from './tools'; export * from './update'; diff --git a/packages/api/src/oauth/tokens.spec.ts b/packages/api/src/oauth/tokens.spec.ts index f3e5b4a29e..3d47840e79 100644 --- a/packages/api/src/oauth/tokens.spec.ts +++ b/packages/api/src/oauth/tokens.spec.ts @@ -22,6 +22,18 @@ jest.mock('@librechat/data-schemas', () => ({ if (value === 'encrypted-refresh-token') { return 'refresh-token'; } + if (value === 'encrypted-encoded-client-id') { + return 'client%40id.com'; + } + if (value === 'encrypted-encoded-client-secret') { + return 's%2Bcret%3D%2Fend'; + } + if (value === 'encrypted-legacy-client-id') { + return 'legacy%value'; + } + if (value === 'encrypted-legacy-client-secret') { + return '100%secret'; + } return value; }), })); @@ -215,6 +227,132 @@ describe('action OAuth token exchange validation', () => { expect(accessConfig.httpsAgent).toBe(refreshConfig.httpsAgent); }); + describe('URL-encoded stored credentials', () => { + const encodedFields = { + ...baseFields, + encrypted_oauth_client_id: 'encrypted-encoded-client-id', + encrypted_oauth_client_secret: 'encrypted-encoded-client-secret', + }; + + const legacyFields = { + ...baseFields, + encrypted_oauth_client_id: 'encrypted-legacy-client-id', + encrypted_oauth_client_secret: 'encrypted-legacy-client-secret', + }; + + it('decodes credentials in the authorization-code exchange body', async () => { + await getAccessToken( + { + ...encodedFields, + code: 'authorization-code', + redirect_uri: 'https://chat.example.com/api/actions/action-1/oauth/callback', + token_exchange_method: TokenExchangeMethodEnum.DefaultPost, + }, + createTokenMethods(), + ); + + const params = new URLSearchParams(getAxiosConfig().data as string); + + expect(params.get('client_id')).toBe('client@id.com'); + expect(params.get('client_secret')).toBe('s+cret=/end'); + }); + + it('decodes credentials in the authorization-code Basic auth header', async () => { + await getAccessToken( + { + ...encodedFields, + code: 'authorization-code', + redirect_uri: 'https://chat.example.com/api/actions/action-1/oauth/callback', + token_exchange_method: TokenExchangeMethodEnum.BasicAuthHeader, + }, + createTokenMethods(), + ); + + const headers = getAxiosConfig().headers as Record; + const credentials = Buffer.from( + headers.Authorization.replace('Basic ', ''), + 'base64', + ).toString('utf8'); + + expect(credentials).toBe('client@id.com:s+cret=/end'); + }); + + it('decodes credentials in the refresh-token exchange body', async () => { + await refreshAccessToken( + { + ...encodedFields, + refresh_token: 'refresh-token', + token_exchange_method: TokenExchangeMethodEnum.DefaultPost, + }, + createTokenMethods(), + ); + + const params = new URLSearchParams(getAxiosConfig().data as string); + + expect(params.get('client_id')).toBe('client@id.com'); + expect(params.get('client_secret')).toBe('s+cret=/end'); + }); + + it('decodes credentials in the refresh-token Basic auth header', async () => { + await refreshAccessToken( + { + ...encodedFields, + refresh_token: 'refresh-token', + token_exchange_method: TokenExchangeMethodEnum.BasicAuthHeader, + }, + createTokenMethods(), + ); + + const headers = getAxiosConfig().headers as Record; + const credentials = Buffer.from( + headers.Authorization.replace('Basic ', ''), + 'base64', + ).toString('utf8'); + + expect(credentials).toBe('client@id.com:s+cret=/end'); + }); + + it('passes through legacy unencoded credentials containing a stray percent sign', async () => { + await expect( + getAccessToken( + { + ...legacyFields, + code: 'authorization-code', + redirect_uri: 'https://chat.example.com/api/actions/action-1/oauth/callback', + token_exchange_method: TokenExchangeMethodEnum.DefaultPost, + }, + createTokenMethods(), + ), + ).resolves.toEqual(tokenResponse); + + const params = new URLSearchParams(getAxiosConfig().data as string); + + expect(params.get('client_id')).toBe('legacy%value'); + expect(params.get('client_secret')).toBe('100%secret'); + }); + + it('passes through legacy unencoded credentials when refreshing', async () => { + await expect( + refreshAccessToken( + { + ...legacyFields, + refresh_token: 'refresh-token', + token_exchange_method: TokenExchangeMethodEnum.BasicAuthHeader, + }, + createTokenMethods(), + ), + ).resolves.toEqual(tokenResponse); + + const headers = getAxiosConfig().headers as Record; + const credentials = Buffer.from( + headers.Authorization.replace('Basic ', ''), + 'base64', + ).toString('utf8'); + + expect(credentials).toBe('legacy%value:100%secret'); + }); + }); + it('allows explicitly exempted private token endpoints', async () => { await getAccessToken( { diff --git a/packages/api/src/oauth/tokens.ts b/packages/api/src/oauth/tokens.ts index 02fb80e287..2a44af334d 100644 --- a/packages/api/src/oauth/tokens.ts +++ b/packages/api/src/oauth/tokens.ts @@ -1,10 +1,11 @@ import axios from 'axios'; +import { logger, encryptV2 } from '@librechat/data-schemas'; import { TokenExchangeMethodEnum } from 'librechat-data-provider'; -import { logger, encryptV2, decryptV2 } from '@librechat/data-schemas'; import type { IToken, TokenMethods } from '@librechat/data-schemas'; import type { AxiosError } from 'axios'; import { DEFAULT_OAUTH_TOKEN_TTL_SECONDS, normalizeExpiresIn } from './expiry'; import { validateActionOAuthEndpoint } from './validation'; +import { decryptSensitiveValue } from '~/actions/crypto'; import { createSSRFSafeAgents } from '~/auth'; import { logAxiosError } from '~/utils'; @@ -189,8 +190,8 @@ export async function refreshAccessToken( await validateActionOAuthEndpoint(client_url, 'client_url', allowedAddresses); try { - const oauth_client_id = await decryptV2(encrypted_oauth_client_id); - const oauth_client_secret = await decryptV2(encrypted_oauth_client_secret); + const oauth_client_id = await decryptSensitiveValue(encrypted_oauth_client_id); + const oauth_client_secret = await decryptSensitiveValue(encrypted_oauth_client_secret); const headers: Record = { 'Content-Type': 'application/x-www-form-urlencoded', @@ -294,8 +295,8 @@ export async function getAccessToken( }> { await validateActionOAuthEndpoint(client_url, 'client_url', allowedAddresses); - const oauth_client_id = await decryptV2(encrypted_oauth_client_id); - const oauth_client_secret = await decryptV2(encrypted_oauth_client_secret); + const oauth_client_id = await decryptSensitiveValue(encrypted_oauth_client_id); + const oauth_client_secret = await decryptSensitiveValue(encrypted_oauth_client_secret); const headers: Record = { 'Content-Type': 'application/x-www-form-urlencoded',