Skip to content

Feat: Add URL-encoding detection and retry logic for cluster authentication [reviewed]#594

Merged
tnaum-ms merged 5 commits intonextfrom
reviews/url-encoded-password-original-pr-454
Apr 21, 2026
Merged

Feat: Add URL-encoding detection and retry logic for cluster authentication [reviewed]#594
tnaum-ms merged 5 commits intonextfrom
reviews/url-encoded-password-original-pr-454

Conversation

@tnaum-ms
Copy link
Copy Markdown
Collaborator

This PR finalizes the review of the contribution originally submitted by @vaibh123540 in #454.

Original PR: #454

Summary

Improves UX when users provide URL-encoded passwords in connection strings. When a connection attempt fails and the password contains URL-encoded characters (%XX sequences), the user is now offered a "Retry with Decoded Password" option in the error dialog. If the retry succeeds, the decoded password is saved to persistent storage so the error doesn't recur in future sessions.

Changes from original PR

  • Resolved conflict with next branch (the showErrorMessage call was updated since the original PR was submitted)
  • Fixed (error as Error).messageerror instanceof Error ? error.message : String(error) for type-safe error handling (per project TypeScript guidelines)
  • Fixed this.idthis.cluster.clusterId in the retry logic for CredentialCache and ClustersClient operations (per the dual-ID architecture — clusterId is the stable cache key)
  • Updated l10n/bundle.l10n.json with new localization strings

Logic

  1. After a connection failure, check if the password contains URL-encoded sequences using /%[0-9A-Fa-f]{2}/
  2. If detected, append a hint to the error message and surface a "Retry with Decoded Password" button
  3. On retry success: save the decoded password to persistent storage
  4. On retry failure: show the standard error without the URL hint

@tnaum-ms tnaum-ms marked this pull request as ready for review April 21, 2026 13:08
@tnaum-ms tnaum-ms requested a review from a team as a code owner April 21, 2026 13:08
Copilot AI review requested due to automatic review settings April 21, 2026 13:08
@tnaum-ms
Copy link
Copy Markdown
Collaborator Author

@vaibh123540 We're merging your contribution today. Thank you very much for taking the time and submitting the original PR. This feature will be shipping with the 0.7.4 release.

@tnaum-ms
Copy link
Copy Markdown
Collaborator Author

@vaibh123540 Congratulations on your first contribution!

first-contribution

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a user-confirmed recovery flow when native-auth connections fail due to URL-encoded passwords, improving connection UX and reducing repeated failures across sessions.

Changes:

  • Introduces URL-encoded password detection + “Try with Decoded Password” retry prompt and telemetry.
  • Updates connection retry logic to re-auth with decoded password and optionally persist updated credentials.
  • Adds new localized strings for the retry prompt/buttons and related messages.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/tree/connections-view/DocumentDBClusterItem.ts Uses the new helper to offer a decoded-password retry after connection failure, and optionally saves corrected credentials.
src/documentdb/auth/urlEncodedPassword.ts New helper module for detecting URL-encoded passwords, prompting retry, and recording non-sensitive telemetry flags.
l10n/bundle.l10n.json Adds localization entries for the new retry flow UI text.
Comments suppressed due to low confidence (1)

src/tree/connections-view/DocumentDBClusterItem.ts:330

  • When a decoded-password retry is attempted, ClustersClient.deleteClient(this.cluster.clusterId) is called before the retry (line 276) and then again unconditionally in the outer failure path (line 328). This double-delete is likely harmless but unnecessary work; consider tracking whether a delete already occurred (or restructuring) to avoid the duplicate call in the retry path.
                    await ClustersClient.deleteClient(this.cluster.clusterId);

                    try {
                        clustersClient = await this.getClientWithProgress(this.cluster.clusterId);

                        ext.outputChannel.appendLine(
                            l10n.t('Connected to the cluster "{cluster}" using decoded password.', {
                                cluster: this.cluster.name,
                            }),
                        );
                        context.telemetry.properties.urlDecodePasswordResult = 'succeeded';

                        // Offer to persist the corrected password so the user does not have to retry next time.
                        const updateButton = l10n.t('Update Saved Password');
                        const saveChoice = await vscode.window.showInformationMessage(
                            l10n.t(
                                'Connected to "{cluster}" using the decoded password. Would you like to update your saved credentials?',
                                { cluster: this.cluster.name },
                            ),
                            { modal: false },
                            updateButton,
                        );

                        if (saveChoice === updateButton) {
                            connectionCredentials.secrets.nativeAuthConfig = {
                                connectionUser: username ?? '',
                                connectionPassword: decodedPassword,
                            };
                            await ConnectionStorageService.save(connectionType, connectionCredentials, true);
                            context.telemetry.properties.urlDecodePasswordSaved = 'true';
                        }

                        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 }));
                        context.telemetry.properties.urlDecodePasswordResult = 'failed';

                        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 }),
                            },
                        );
                    }
                }

                // If connection fails, remove cached credentials
                await ClustersClient.deleteClient(this.cluster.clusterId);
                CredentialCache.deleteCredentials(this.cluster.clusterId);

Comment thread src/documentdb/auth/urlEncodedPassword.ts
Comment thread src/tree/connections-view/DocumentDBClusterItem.ts
@tnaum-ms tnaum-ms merged commit e358bd9 into next Apr 21, 2026
8 checks passed
@tnaum-ms tnaum-ms deleted the reviews/url-encoded-password-original-pr-454 branch April 21, 2026 14:12
@tnaum-ms tnaum-ms mentioned this pull request Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve: Warn users about an URL Encoded password

3 participants