Skip to content

Feat: Add URL-encoding detection and retry logic for cluster authentication #454

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.

Sign up for GitHub

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

Open
vaibh123540 wants to merge 2 commits into
base: rel/release-0.7.1
Choose a base branch
from
+75 −8
Open

Feat: Add URL-encoding detection and retry logic for cluster authentication #454

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3586848
Added functionality for URL decoding of password and suggesting to fi…
Jan 20, 2026
ec6e189
Merge branch 'next' into dev/vaibh123540/password-url-warning
tnaum-ms Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 75 additions & 8 deletions src/tree/connections-view/DocumentDBClusterItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,17 +225,84 @@ export class DocumentDBClusterItem extends ClusterItemBase implements TreeElemen
} catch (error) {
ext.outputChannel.appendLine(l10n.t('Error: {error}', { error: (error as Error).message }));

void vscode.window.showErrorMessage(
// URL-Encoding checks
let decodedPassword: string | undefined;
try {
decodedPassword = password ? decodeURIComponent(password) : undefined;
} catch {
decodedPassword = undefined;
}

const retryButton = l10n.t('Retry with Decoded Password');
const buttons: string[] = [];

let detailMessage: string =
l10n.t('Revisit connection details and try again.') +
'\n\n' +
l10n.t('Error: {error}', { error: (error as Error).message });
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling uses (error as Error).message without verifying the error is actually an Error instance. While this works in most cases, it could fail if the caught error is not an Error object. For consistency with the retry error handling (line 289 which uses retryErr instanceof Error), consider using the same defensive pattern to ensure robust error message extraction.

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern /%[0-9A-Fa-f]{2}/ only detects passwords that contain URL-encoded characters, but it doesn't verify that the entire password is URL-encoded or that the encoding is valid. This could result in false positives for passwords that legitimately contain sequences like "%20" as part of their actual password text (not as encoding). Consider adding additional validation to reduce false positives, or document this limitation in a comment explaining that some legitimate passwords may trigger this detection.

Suggested change
// Heuristic: detect presence of URL-encoded-looking sequences in the password.
// This does not guarantee that the entire password is URL-encoded or that the
// encoding is valid; some legitimate passwords may contain sequences like "%20"
// and will still trigger this hint. We rely on a successful decode and a changed
// value (password !== decodedPassword) before offering the retry.

Copilot uses AI. Check for mistakes.
const shouldOfferRetry =
password && /%[0-9A-Fa-f]{2}/.test(password) && decodedPassword && password !== decodedPassword;

Comment on lines +245 to +246
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic doesn't verify that authMethod is specifically NativeAuth before offering to decode the password. While the condition on line 259 checks that username and password exist (which implicitly suggests native auth), it's possible for other auth methods to have these values set. Consider adding an explicit check authMethod === AuthMethodId.NativeAuth to the shouldOfferRetry condition on line 244 to ensure this feature only applies to native authentication where passwords are actually used.

Suggested change
password && /%[0-9A-Fa-f]{2}/.test(password) && decodedPassword && password !== decodedPassword;
authMethod === AuthMethodId.NativeAuth &&
password &&
/%[0-9A-Fa-f]{2}/.test(password) &&
decodedPassword &&
password !== decodedPassword;

Copilot uses AI. Check for mistakes.
if (shouldOfferRetry) {
detailMessage += '\n\n' + l10n.t('Your password appears to be URL-encoded.');
buttons.push(retryButton);
}

// Offer a one-time retry using the decoded value
const result = await vscode.window.showErrorMessage(
l10n.t('Failed to connect to "{cluster}"', { cluster: this.cluster.name }),
{
modal: true,
detail:
l10n.t('Revisit connection details and try again.') +
'\n\n' +
l10n.t('Error: {error}', { error: (error as Error).message }),
},
{ modal: true, detail: detailMessage },
...buttons,
);
Comment on lines +253 to 257
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'result' shadows the outer variable 'result' from the callWithTelemetryAndErrorHandling callback function scope. This can lead to confusion and potential bugs. Consider renaming this variable to something more specific like 'userResponse' or 'dialogResult' to avoid shadowing.

Copilot uses AI. Check for mistakes.

if (result === retryButton && decodedPassword && username && authMethod) {
context.valuesToMask.push(decodedPassword);

CredentialCache.setAuthCredentials(
this.id,
authMethod,
connectionString.toString(),
{ connectionUser: username, connectionPassword: decodedPassword },
this.cluster.emulatorConfiguration,
connectionCredentials.secrets.entraIdAuthConfig,
);

await ClustersClient.deleteClient(this.id);

try {
clustersClient = await ClustersClient.getClient(this.id);

connectionCredentials.secrets.nativeAuthConfig = {
connectionUser: username,
connectionPassword: decodedPassword,
};
await ConnectionStorageService.save(connectionType, connectionCredentials, true);

ext.outputChannel.appendLine(
l10n.t('Connected to the cluster "{cluster}" using decoded password.', {
cluster: this.cluster.name,
}),
);
return clustersClient;
} catch (retryErr: unknown) {
const retryError = retryErr instanceof Error ? retryErr : new Error(String(retryErr));
ext.outputChannel.appendLine(l10n.t('Retry Error: {error}', { error: retryError.message }));

// Show final error screen without the URL hint
void vscode.window.showErrorMessage(
l10n.t('Failed to connect to "{cluster}"', { cluster: this.cluster.name }),
{
modal: true,
detail:
l10n.t('Revisit connection details and try again.') +
'\n\n' +
l10n.t('Error: {error}', { error: retryError.message }),
},
);
}
}
Comment on lines +228 to +304
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL-encoding detection and retry logic lacks test coverage. This new feature introduces complex conditional logic, user interaction flows, and credential management that should be validated with automated tests. Consider adding tests for: (1) detection of URL-encoded passwords, (2) successful retry with decoded password, (3) failed retry with decoded password, (4) user declining the retry offer, and (5) edge cases like malformed encoding or passwords with legitimate percent sequences.

Copilot uses AI. Check for mistakes.

// If connection fails, remove cached credentials
await ClustersClient.deleteClient(this.id);
CredentialCache.deleteCredentials(this.id);
Expand Down
Loading