Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@Josbleuet
Copy link
Contributor

The Dynamic Auth Provider was using the full authorization server URL (e.g., 'https://example.com/') as the logger ID, which caused 'Illegal characters in path' errors on Windows when mkdirp tried to create the log file directory.

This change sanitizes the provider ID before using it as a logger filename by replacing characters illegal in file paths (:, /, ?, *, etc.) with underscores.

Changes:

  • Added sanitizeForFilename() private method to DynamicAuthProvider
  • Updated logger creation to use sanitized ID
  • Added unit tests for filename sanitization

Example transformation:
Before: 'https://example.com/' After: 'https___example.com_'

Fixes dynamic auth provider initialization on Windows with MCP servers.

@vs-code-engineering
Copy link

vs-code-engineering bot commented Dec 1, 2025

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@TylerLeonhardt

Matched files:

  • src/vs/workbench/api/common/extHostAuthentication.ts

src/vs/workbench/api/common/extHostAuthentication.ts Outdated Show resolved Hide resolved
@alexr00 alexr00 assigned TylerLeonhardt and unassigned alexr00 Dec 1, 2025
@TylerLeonhardt
Copy link
Member

Actually, why are we fixing this here instead of lower down in the stack. Wouldn't it make sense that we do this replacement generically for all logger names?

This fixes this instance, but if someone does this again, they won't know unless they're in this discussion.

Cc @sandy081

@Josbleuet Josbleuet force-pushed the fix/mcp-auth-illegal-chars branch from b3ce986 to e526f09 Compare December 1, 2025 23:54
Copy link
Contributor Author

@Josbleuet Josbleuet left a comment

Choose a reason for hiding this comment

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

Good catch! You're absolutely right that this should be handled at the logger service level. I've refactored the fix to add the sanitization directly in AbstractLoggerService.toResource() instead of in the auth provider.

Now any code that passes a string with invalid filename characters as a logger ID will be automatically protected. Much better solution than fixing it in one place.

Updated in the latest commit: e526f09

@Josbleuet
Copy link
Contributor Author

Josbleuet commented Dec 2, 2025 via email

@sandy081
Copy link
Member

sandy081 commented Dec 2, 2025

That's right and it also fixes - #269810

@sandy081 sandy081 added this to the November 2025 milestone Dec 2, 2025
@sandy081 sandy081 assigned sandy081 and unassigned TylerLeonhardt Dec 2, 2025
@sandy081 sandy081 self-requested a review December 2, 2025 14:26
Comment on lines 652 to 668
if (isString(idOrResource)) {
const sanitizedId = this.sanitizeForFilename(idOrResource);
return joinPath(this.logsHome, `${sanitizedId}.log`);
}
return idOrResource;
}

/**
* Sanitizes a string to be safe for use as a filename.
* Replaces characters that are illegal in Windows filenames with underscores.
* @param str The string to sanitize
* @returns A filesystem-safe version of the string
*/
private sanitizeForFilename(str: string): string {
// Replace characters that are illegal in Windows filenames: < > : " / \ | ? *
// Also replace whitespace for consistency
return str.replace(/[<>:"/\\|?*\s]/g, '_');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (isString(idOrResource)) {
const sanitizedId = this.sanitizeForFilename(idOrResource);
return joinPath(this.logsHome, `${sanitizedId}.log`);
}
return idOrResource;
}
/**
* Sanitizes a string to be safe for use as a filename.
* Replaces characters that are illegal in Windows filenames with underscores.
* @param str The string to sanitize
* @returns A filesystem-safe version of the string
*/
private sanitizeForFilename(str: string): string {
// Replace characters that are illegal in Windows filenames: < > : " / \ | ? *
// Also replace whitespace for consistency
return str.replace(/[<>:"/\\|?*\s]/g, '_');
return isString(idOrResource) ? joinPath(this.logsHome, `${idOrResource.replace(/[\\/:\*\?"<>\|]/g, '')}.log`) : idOrResource;

@Josbleuet Josbleuet force-pushed the fix/mcp-auth-illegal-chars branch from 3883ce5 to 1cec7c9 Compare December 3, 2025 00:17
@Josbleuet Josbleuet requested a review from sandy081 December 3, 2025 00:21
@Josbleuet
Copy link
Contributor Author

I've updated the PR with your suggested approach - using an inline replace instead of a separate sanitization function.

@sandy081 sandy081 enabled auto-merge (squash) December 3, 2025 08:30
@sandy081 sandy081 merged commit 2d96f38 into microsoft:main Dec 3, 2025
18 checks passed
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.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.