-
Notifications
You must be signed in to change notification settings - Fork 11
Feat: Add URL-encoding detection and retry logic for cluster authentication [reviewed] #594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
11990ff
Added functionality for URL decoding of password and suggesting to fi…
682b2de
chore: update l10n bundle after adding URL-encoded password strings
tnaum-ms be40588
feat: implement URL-encoded password handling with user confirmation …
tnaum-ms 0d20987
feat: enhance URL-encoded password handling with user prompts for ret…
tnaum-ms b35727b
feat: add error handling for saving updated credentials and implement…
tnaum-ms File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { type IActionContext } from '@microsoft/vscode-azext-utils'; | ||
| import * as vscode from 'vscode'; | ||
| import { | ||
| showConnectionFailedAndMaybeOfferDecodedRetry, | ||
| tryDecodeUrlEncodedPassword, | ||
| UrlEncodedPasswordTelemetry, | ||
| } from './urlEncodedPassword'; | ||
|
|
||
| function createMockContext(): IActionContext { | ||
| return { | ||
| telemetry: { properties: {}, measurements: {} }, | ||
| errorHandling: { issueProperties: {} }, | ||
| ui: {} as IActionContext['ui'], | ||
| valuesToMask: [], | ||
| } as unknown as IActionContext; | ||
| } | ||
|
|
||
| describe('tryDecodeUrlEncodedPassword', () => { | ||
| it('returns undefined for undefined input', () => { | ||
| expect(tryDecodeUrlEncodedPassword(undefined)).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined for empty string', () => { | ||
| expect(tryDecodeUrlEncodedPassword('')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined when password has no percent-encoded sequences', () => { | ||
| expect(tryDecodeUrlEncodedPassword('plainPassword123')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined when password contains % but not valid encoding', () => { | ||
| // e.g. "100%" — the % is not followed by two hex digits | ||
| expect(tryDecodeUrlEncodedPassword('100%')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined when password contains %XX that is not valid UTF-8', () => { | ||
| // %C3 alone is an incomplete UTF-8 sequence; decodeURIComponent should throw | ||
| expect(tryDecodeUrlEncodedPassword('%C3')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns decoded password for %40 (@)', () => { | ||
| expect(tryDecodeUrlEncodedPassword('p%40ss')).toBe('p@ss'); | ||
| }); | ||
|
|
||
| it('returns decoded password for multiple encoded characters', () => { | ||
| expect(tryDecodeUrlEncodedPassword('p%40ss%21w%23rd')).toBe('p@ss!w#rd'); | ||
| }); | ||
|
|
||
| it('returns decoded password for %20 (space)', () => { | ||
| expect(tryDecodeUrlEncodedPassword('my%20password')).toBe('my password'); | ||
| }); | ||
|
|
||
| it('returns undefined when decoding produces the same string', () => { | ||
| // %30 decodes to "0", so "abc%30" decodes to "abc0" | ||
| // But a password like "%41" decodes to "A", which differs. | ||
| // A password that is already decoded with no encoded chars won't match the pattern. | ||
| // Let's use a case where decoded === original: not possible with valid %XX that differs. | ||
| // Actually if someone has "%25" it decodes to "%", so that's always different. | ||
| // This edge case is hard to trigger with valid encoding, so skip pure equality test. | ||
| }); | ||
|
|
||
| it('handles case-insensitive hex digits', () => { | ||
| expect(tryDecodeUrlEncodedPassword('p%2Fss')).toBe('p/ss'); | ||
| expect(tryDecodeUrlEncodedPassword('p%2fss')).toBe('p/ss'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('showConnectionFailedAndMaybeOfferDecodedRetry', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const showErrorMessage: jest.Mock = vscode.window.showErrorMessage as unknown as jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('shows error dialog without retry button when password is not URL-encoded', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| showErrorMessage.mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: 'plainPassword', | ||
| isNativeAuth: true, | ||
| originalError: new Error('auth failed'), | ||
| context, | ||
| }); | ||
|
|
||
| expect(result.decodedPassword).toBeUndefined(); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Detected]).toBe('false'); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Offered]).toBe('false'); | ||
| // The dialog should have been called with no extra buttons | ||
| expect(showErrorMessage).toHaveBeenCalledWith( | ||
| expect.stringContaining('test-cluster'), | ||
| expect.objectContaining({ modal: true }), | ||
| ); | ||
| }); | ||
|
|
||
| it('shows error dialog without retry button when auth is not native', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| showErrorMessage.mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: 'p%40ss', // URL-encoded but non-native auth | ||
| isNativeAuth: false, | ||
| originalError: new Error('auth failed'), | ||
| context, | ||
| }); | ||
|
|
||
| expect(result.decodedPassword).toBeUndefined(); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Detected]).toBe('false'); | ||
| }); | ||
|
|
||
| it('offers retry button when password is URL-encoded and auth is native', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| showErrorMessage.mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: 'p%40ss', | ||
| isNativeAuth: true, | ||
| originalError: new Error('auth failed'), | ||
| context, | ||
| }); | ||
|
|
||
| expect(result.decodedPassword).toBeUndefined(); // user didn't click retry | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Detected]).toBe('true'); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Offered]).toBe('true'); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Accepted]).toBe('false'); | ||
| // Should have been called with the retry button | ||
| expect(showErrorMessage).toHaveBeenCalledWith( | ||
| expect.stringContaining('test-cluster'), | ||
| expect.objectContaining({ modal: true }), | ||
| expect.stringContaining('Decoded Password'), | ||
| ); | ||
| }); | ||
|
|
||
| it('returns decoded password when user clicks retry button', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| // Simulate user clicking the retry button | ||
| showErrorMessage.mockImplementation((_msg: string, _opts: unknown, ...buttons: string[]) => | ||
| Promise.resolve(buttons[0]), | ||
| ); | ||
|
|
||
| const result = await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: 'p%40ss', | ||
| isNativeAuth: true, | ||
| originalError: new Error('auth failed'), | ||
| context, | ||
| }); | ||
|
|
||
| expect(result.decodedPassword).toBe('p@ss'); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Accepted]).toBe('true'); | ||
| }); | ||
|
|
||
| it('handles non-Error original error', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| showErrorMessage.mockResolvedValueOnce(undefined); | ||
|
|
||
| await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: undefined, | ||
| isNativeAuth: true, | ||
| originalError: 'string error', | ||
| context, | ||
| }); | ||
|
|
||
| // Should not throw; the dialog detail should contain the stringified error | ||
| expect(showErrorMessage).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| expect.objectContaining({ | ||
| detail: expect.stringContaining('string error'), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('shows error dialog without retry button when password is undefined', async () => { | ||
| const context = createMockContext(); | ||
|
|
||
| showErrorMessage.mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await showConnectionFailedAndMaybeOfferDecodedRetry({ | ||
| clusterName: 'test-cluster', | ||
| password: undefined, | ||
| isNativeAuth: true, | ||
| originalError: new Error('auth failed'), | ||
| context, | ||
| }); | ||
|
|
||
| expect(result.decodedPassword).toBeUndefined(); | ||
| expect(context.telemetry.properties[UrlEncodedPasswordTelemetry.Detected]).toBe('false'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { type IActionContext } from '@microsoft/vscode-azext-utils'; | ||
| import * as l10n from '@vscode/l10n'; | ||
| import * as vscode from 'vscode'; | ||
|
|
||
| /** | ||
| * Helpers for detecting and recovering from URL-encoded passwords. | ||
| * | ||
| * Users sometimes paste a password copied from a connection-string URL and | ||
| * forget to URL-decode it. The server then rejects the credentials because | ||
| * the actual password characters differ from the encoded form (e.g. "p%40ss" | ||
| * vs. "p@ss"). Rather than silently retrying — which could trip brute-force | ||
| * lockouts on the server side — we show a one-time prompt and let the user | ||
| * decide whether to retry with the decoded value. | ||
| */ | ||
|
|
||
| /** Matches a `%XX` percent-encoded byte. */ | ||
| const URL_ENCODED_BYTE_PATTERN = /%[0-9A-Fa-f]{2}/; | ||
|
|
||
| /** | ||
| * Returns the decoded password if `password` contains `%XX` sequences and | ||
| * `decodeURIComponent` produces a different, non-empty string; otherwise | ||
| * returns `undefined`. Intended as a hint — callers must never auto-apply | ||
| * the decoded value without user consent. | ||
| */ | ||
| export function tryDecodeUrlEncodedPassword(password: string | undefined): string | undefined { | ||
| if (!password || !URL_ENCODED_BYTE_PATTERN.test(password)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| let decoded: string; | ||
| try { | ||
| decoded = decodeURIComponent(password); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (decoded.length === 0 || decoded === password) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return decoded; | ||
| } | ||
|
|
||
| /** | ||
| * Telemetry property keys reported by {@link showConnectionFailedAndMaybeOfferDecodedRetry}. | ||
| * No password material is ever recorded — only boolean indicators about the flow. | ||
| */ | ||
| export const UrlEncodedPasswordTelemetry = { | ||
| /** `true` when the original password looked URL-encoded and a decoded variant was available. */ | ||
| Detected: 'urlDecodePasswordDetected', | ||
| /** `true` when the user was shown the "Retry with decoded password" button. */ | ||
| Offered: 'urlDecodePasswordOffered', | ||
| /** `true` when the user clicked the retry button. */ | ||
| Accepted: 'urlDecodePasswordAccepted', | ||
| } as const; | ||
|
|
||
| export interface ShowConnectionFailedOptions { | ||
| /** Display name of the cluster shown in the error dialog. */ | ||
| readonly clusterName: string; | ||
| /** The password that was used for the failed connection attempt (may be undefined for non-password auth). */ | ||
| readonly password: string | undefined; | ||
| /** Whether the failed attempt used password-based (native) auth. Retry is only offered for native auth. */ | ||
| readonly isNativeAuth: boolean; | ||
| /** The error returned by the failed connection attempt. */ | ||
| readonly originalError: unknown; | ||
| /** Action context used to record non-sensitive telemetry about the retry flow. */ | ||
| readonly context: IActionContext; | ||
| } | ||
|
|
||
| export interface ShowConnectionFailedResult { | ||
| /** | ||
| * Populated with the decoded password only when the user explicitly chose to retry. | ||
| * Callers must mask this value via `context.valuesToMask` before use. | ||
| */ | ||
| readonly decodedPassword?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Shows the "Failed to connect" modal. If the password looks URL-encoded, adds a | ||
| * one-time "Retry with decoded password" button. Records telemetry about whether | ||
| * the hint was offered and accepted. | ||
| */ | ||
| export async function showConnectionFailedAndMaybeOfferDecodedRetry( | ||
| options: ShowConnectionFailedOptions, | ||
| ): Promise<ShowConnectionFailedResult> { | ||
| const { clusterName, password, isNativeAuth, originalError, context } = options; | ||
|
|
||
| const decodedPassword = isNativeAuth ? tryDecodeUrlEncodedPassword(password) : undefined; | ||
| const canOfferRetry = decodedPassword !== undefined; | ||
|
|
||
| context.telemetry.properties[UrlEncodedPasswordTelemetry.Detected] = canOfferRetry ? 'true' : 'false'; | ||
| context.telemetry.properties[UrlEncodedPasswordTelemetry.Offered] = canOfferRetry ? 'true' : 'false'; | ||
|
|
||
| const errorMessage = originalError instanceof Error ? originalError.message : String(originalError); | ||
|
|
||
| let detail = | ||
| l10n.t('Revisit connection details and try again.') + | ||
| '\n\n' + | ||
| l10n.t('Error: {error}', { error: errorMessage }); | ||
|
|
||
| const retryButton = l10n.t('Try with Decoded Password'); | ||
| const buttons: string[] = []; | ||
|
|
||
| if (canOfferRetry) { | ||
| detail += | ||
| '\n\n' + | ||
| l10n.t( | ||
| '💡 Your password appears to contain URL-encoded characters (e.g. %40 instead of @). This often happens when copying a password from a connection string URL. Would you like to retry with the decoded version?', | ||
| ); | ||
| buttons.push(retryButton); | ||
| } | ||
|
|
||
| const selected = await vscode.window.showErrorMessage( | ||
| l10n.t('Failed to connect to "{cluster}"', { cluster: clusterName }), | ||
| { modal: true, detail }, | ||
| ...buttons, | ||
| ); | ||
|
|
||
| const accepted = canOfferRetry && selected === retryButton; | ||
| context.telemetry.properties[UrlEncodedPasswordTelemetry.Accepted] = accepted ? 'true' : 'false'; | ||
|
|
||
| return { decodedPassword: accepted ? decodedPassword : undefined }; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.