Removed Dependency on Secrets and Streamlined Audio Playback#733
Removed Dependency on Secrets and Streamlined Audio Playback#733johniwasz merged 8 commits intoversion-18from
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…734) * Updated AWS service endpoints list * Corrected markdown formatting issues in CHANGELOG * Storing aws secret securely * Storing AWS Secret Access Key securely. * Added directions about storing secrets. * Removed use of secrets and streamlined audio playback * Updated change log for 18 * Potential fix for code scanning alert no. 2: Useless conditional --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the SpeechMarkdown VS Code extension by removing the dependency on manual AWS credential entry and streamlining audio playback. The changes shift from storing AWS Access Key ID and Secret Access Key in extension settings to using AWS named profiles, and replace external media player invocation with an integrated WebView-based audio player.
Changes:
- AWS authentication migrated from manual key entry to AWS profile-based authentication (more secure, follows AWS best practices)
- Audio playback moved from external applications (QuickTime, Windows Media Player) to an integrated VS Code WebView panel
- Added new configuration settings for AWS profile selection and automatic audio file cleanup
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ssmlAudioPlayer.ts | Core implementation changes for AWS profile authentication and WebView-based audio playback |
| src/smdOutputProvider.ts | Refactored SSML generation with new GetSSMLDirect method and improved parameter handling |
| src/test/ssmlAudioPlayer.test.ts | Test file added (but contains references to deprecated authentication approach) |
| src/test/index.ts | Test runner implementation for Mocha integration |
| src/extension.ts | Changed activate function to async (minor compatibility update) |
| package.json | Updated configuration schema, removed deprecated settings, added new settings, updated voices and regions |
| package-lock.json | Dependency lock file updates (includes mocha and testing dependencies) |
| README.md | Updated documentation for AWS profile authentication and voice lists |
| CHANGELOG.md | Documented changes for version 0.0.18 |
| .vscode/launch.json | Fixed launch configuration version to comply with VS Code standards |
| images/config01.png | New screenshot showing updated configuration UI |
| images/awsconfig.png | New screenshot showing AWS configuration options |
Comments suppressed due to low confidence (1)
package.json:26
- To support AWS profile-based authentication, the package dependencies must include
@aws-sdk/credential-providers. This package is required to use thefromInicredential provider that enables loading AWS credentials from profiles. Without this dependency, the profile-based authentication will not work correctly.
"galleryBanner": {
"color": "#000000",
"theme": "dark"
},
"license": "MIT",
"version": "0.0.18",
"preview": false,
"author": {
"name": "johniwasz"
},
"engines": {
"vscode": "^1.109.0"
},
"categories": [
"Snippets",
"Formatters"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case 'speechmarkdown.aws.accessKeyId': | ||
| return 'test-access-key'; |
There was a problem hiding this comment.
The test file references the deprecated speechmarkdown.aws.accessKeyId setting on line 16, which conflicts with the PR's goal of removing dependency on access keys. This test mock should be updated to reflect the new AWS profile-based authentication approach.
| case 'speechmarkdown.aws.accessKeyId': | |
| return 'test-access-key'; | |
| case 'speechmarkdown.aws.profile': | |
| return 'default'; |
| test('should use environment variables when configuration is missing', async () => { | ||
| // Set environment variables | ||
| process.env.AWS_DEFAULT_REGION = 'us-west-2'; | ||
| process.env.AWS_ACCESS_KEY_ID = 'test-env-key'; | ||
| process.env.AWS_SECRET_ACCESS_KEY = 'test-env-secret'; | ||
|
|
||
| // Override config to return undefined except for required polly voice | ||
| const originalGetConfiguration = vscode.workspace.getConfiguration; | ||
| (vscode.workspace as any).getConfiguration = () => ({ | ||
| get: (section: string) => { | ||
| if (section === 'speechmarkdown.aws.pollyDefaultVoice') { | ||
| return 'Joanna'; | ||
| } | ||
| return undefined; // Force use of environment variables | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| await SSMLAudioPlayer.getSSMLSpeechAsync('Hello world', Engine.STANDARD); | ||
| assert.ok(true, 'Should use environment variables when config is missing'); | ||
| } catch (error) { | ||
| // Expected to fail in test environment, but should attempt to use env vars | ||
| assert.ok(true, 'Environment variable fallback works'); | ||
| } finally { | ||
| // Clean up environment variables | ||
| delete process.env.AWS_DEFAULT_REGION; | ||
| delete process.env.AWS_ACCESS_KEY_ID; | ||
| delete process.env.AWS_SECRET_ACCESS_KEY; | ||
| // Restore original | ||
| (vscode.workspace as any).getConfiguration = originalGetConfiguration; | ||
| } |
There was a problem hiding this comment.
The test references the deprecated AWS access key and secret key environment variables (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY) which are inconsistent with the PR's migration to AWS profiles. These tests should be updated to test the profile-based authentication approach using AWS_PROFILE environment variable instead.
| test('should handle AWS secret storage', async () => { | ||
| // Simulate stored secret | ||
| await mockSecretStorage.store('speechmarkdown.aws.secretAccessKey', 'stored-secret-key'); | ||
|
|
||
| try { | ||
| await SSMLAudioPlayer.getSSMLSpeechAsync('Hello world', Engine.STANDARD); | ||
| assert.ok(true, 'Should access secret storage'); | ||
| } catch (error) { | ||
| // Expected to fail in test environment, but should attempt to access secrets | ||
| assert.ok(true, 'Secret storage access works'); |
There was a problem hiding this comment.
The test references the deprecated secret storage mechanism for AWS credentials (speechmarkdown.aws.secretAccessKey) which has been removed in this PR. This test should be removed or updated to test the new profile-based authentication approach.
| test('should handle AWS secret storage', async () => { | |
| // Simulate stored secret | |
| await mockSecretStorage.store('speechmarkdown.aws.secretAccessKey', 'stored-secret-key'); | |
| try { | |
| await SSMLAudioPlayer.getSSMLSpeechAsync('Hello world', Engine.STANDARD); | |
| assert.ok(true, 'Should access secret storage'); | |
| } catch (error) { | |
| // Expected to fail in test environment, but should attempt to access secrets | |
| assert.ok(true, 'Secret storage access works'); | |
| test('should use AWS profile from configuration', async () => { | |
| const originalGetConfiguration = vscode.workspace.getConfiguration; | |
| (vscode.workspace as any).getConfiguration = () => ({ | |
| get: (section: string) => { | |
| if (section === 'speechmarkdown.aws.profile') { | |
| return 'default'; | |
| } | |
| if (section === 'speechmarkdown.aws.pollyDefaultVoice') { | |
| return 'Joanna'; | |
| } | |
| return undefined; | |
| } | |
| }); | |
| try { | |
| await SSMLAudioPlayer.getSSMLSpeechAsync('Hello world', Engine.STANDARD); | |
| assert.ok(true, 'Should use AWS profile from configuration'); | |
| } catch (error) { | |
| // Expected to fail in test environment, but should attempt to use the configured profile | |
| assert.ok(true, 'Profile-based configuration path is exercised'); | |
| } finally { | |
| (vscode.workspace as any).getConfiguration = originalGetConfiguration; |
| outChannel.appendLine(`Falling back to system default player...`); | ||
|
|
||
| // Fallback to system default player | ||
| const cmd = process.platform === 'win32' ? `start "${outfile}"` : |
There was a problem hiding this comment.
The fallback command for Windows platform is using start with quotes around the file path, which may cause issues with paths containing spaces. The correct format for Windows should be start "" "${outfile}" where the first empty quotes are for the window title and the second quotes are for the file path.
| const cmd = process.platform === 'win32' ? `start "${outfile}"` : | |
| const cmd = process.platform === 'win32' ? `start "" "${outfile}"` : |
| audio.addEventListener('canplay', () => status.textContent = 'Ready to play'); | ||
| audio.addEventListener('play', () => status.textContent = 'Playing...'); | ||
| audio.addEventListener('pause', () => status.textContent = 'Paused'); | ||
| audio.addEventListener('ended', () => status.textContent = 'Playback completed'); | ||
| audio.addEventListener('error', (e) => status.textContent = 'Error: ' + e.message); | ||
|
|
||
| // Auto-play when loaded (only if user hasn't manually stopped) | ||
| audio.addEventListener('canplay', () => { | ||
| if (!userStopped) { | ||
| setTimeout(() => { | ||
| audio.play().catch(e => { | ||
| status.textContent = 'Auto-play blocked. Click Play button.'; | ||
| }); | ||
| }, 100); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The 'canplay' event listener is registered twice (lines 329 and 336), which will cause both handlers to execute. The second handler (lines 336-344) attempts to auto-play the audio, but this conflicts with the first handler (line 329) which only updates the status text. This could lead to unexpected behavior. Consider removing the duplicate event listener registration or consolidating the logic into a single handler.
| </head> | ||
| <body> | ||
| <div class="audio-container"> | ||
| <div class="file-info">🎵 ${fileName}</div> |
There was a problem hiding this comment.
The fileName parameter is directly inserted into the HTML template without any escaping or sanitization. If the temporary file name contains special HTML characters (e.g., <, >, &, "), this could lead to HTML injection. Although the fileName comes from path.basename(outfile) where outfile is generated by tmp.tmpNameSync(), it's still a best practice to sanitize user-facing output. Consider HTML-escaping the fileName before inserting it into the template.
| let clientConfig: any = { region: awsRegion }; | ||
| clientConfig.profile = awsProfile; | ||
|
|
||
| outChannel.appendLine('\nInitializing AWS Polly client...'); | ||
|
|
||
|
|
||
|
|
||
| let client : PollyClient = new PollyClient(clientConfig); |
There was a problem hiding this comment.
The AWS SDK PollyClient configuration is using the profile property directly, but this is not a valid configuration option for the AWS SDK v3. To use AWS profiles with the AWS SDK v3, you need to import and use the fromIni credential provider from @aws-sdk/credential-providers. The current implementation will likely fail to authenticate using profiles. The correct approach is to use credentials: fromIni({ profile: awsProfile }) in the client configuration.
| "speechmarkdown.deleteAudioAfterPlayback": { | ||
| "type": "boolean", | ||
| "order": 4, | ||
| "default": true, | ||
| "description": "Automatically delete temporary audio files when the audio player is closed." |
There was a problem hiding this comment.
The default value for the deleteAudioAfterPlayback setting should align with the behavior users expect. While the description mentions "When enabled (default: on)", the actual default is set to true which is correct. However, for temporary files that are generated automatically, keeping them by default could lead to disk space accumulation over time. The current default of true (delete after playback) is appropriate and follows good practices for temporary file management.
| private static playAudio(commandOutput : SynthesizeSpeechCommandOutput, outChannel: vscode.OutputChannel) { | ||
|
|
||
|
|
||
| if (commandOutput.AudioStream instanceof Readable) { | ||
|
|
||
| let outfile : string = tmp.tmpNameSync() + ".mp3"; | ||
|
|
||
| const writableStream = fs.createWriteStream(outfile); | ||
| const writableStream = fs.createWriteStream(outfile); | ||
|
|
||
| commandOutput.AudioStream.on('data', chunk => { | ||
| writableStream.write(chunk); | ||
| }); | ||
|
|
||
| // First, we need to wait for the command output to end | ||
| // before playing the audio. | ||
| commandOutput.AudioStream.on('end', () => | ||
| { | ||
|
|
||
| writableStream.close((err?: NodeJS.ErrnoException | null) => { | ||
|
|
||
| var cmd: string; | ||
|
|
||
| switch (process.platform) { | ||
| case 'darwin': { | ||
| cmd = `osascript -e 'tell application "QuickTime Player"' -e 'set theMovie to open POSIX file "${outfile}"' -e 'tell theMovie to play' -e 'end tell'`; | ||
| break; | ||
| } | ||
| case 'win32': { | ||
| cmd = `start ${outfile}`; | ||
| break; | ||
| } | ||
| default: { | ||
| cmd = `xdg-open ${outfile}`; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| outChannel.appendLine(`Open command: ${cmd}`); | ||
|
|
||
| child.exec(cmd, {}, (err: Error | null, stdout: string, stderr: string) => { | ||
| // Wait for the stream to end before opening in VS Code | ||
| commandOutput.AudioStream.on('end', () => { | ||
| writableStream.close(async (err?: NodeJS.ErrnoException | null) => { | ||
| if (err) { | ||
| //vscode.window.showErrorMessage(`Launch error: ${err}`); | ||
| outChannel.appendLine(`Launch stdout: ${stdout}`); | ||
| console.error(err, err.stack); | ||
| outChannel.appendLine(`Error writing audio file: ${err.message}`); | ||
| return; | ||
| } | ||
|
|
||
| fs.unlink(outfile, (err) => { | ||
| if (err) throw err; | ||
| outChannel.appendLine(`${outfile} was deleted\n`); | ||
| try { | ||
| // Create a WebView to display the audio player | ||
| const panel = vscode.window.createWebviewPanel( | ||
| 'audioPlayer', | ||
| 'Speech Markdown Audio Player', | ||
| vscode.ViewColumn.Beside, | ||
| { | ||
| enableScripts: true, | ||
| localResourceRoots: [vscode.Uri.file(path.dirname(outfile))] | ||
| } | ||
| ); | ||
|
|
||
| // Convert file path to webview URI | ||
| const audioUri = panel.webview.asWebviewUri(vscode.Uri.file(outfile)); | ||
|
|
||
| // Generate HTML content with audio player | ||
| const htmlContent = this.getAudioPlayerHTML(audioUri.toString(), path.basename(outfile)); | ||
| panel.webview.html = htmlContent; | ||
|
|
||
| // Clean up file when webview is closed | ||
| panel.onDidDispose(() => { | ||
| const deleteAfterPlayback = <boolean>vscode.workspace.getConfiguration().get('speechmarkdown.deleteAudioAfterPlayback'); | ||
|
|
||
| if (deleteAfterPlayback) { | ||
| fs.unlink(outfile, (err) => { | ||
| if (err) { | ||
| outChannel.appendLine(`Error deleting audio file: ${err.message}`); | ||
| } else { | ||
| outChannel.appendLine(`Audio file deleted: ${outfile}`); | ||
| } | ||
| }); | ||
| } else { | ||
| outChannel.appendLine(`Audio file preserved: ${outfile}`); | ||
| } | ||
| }); | ||
|
|
||
| outChannel.appendLine(`\nOpened audio in WebView player: ${outfile}`); | ||
|
|
||
| } catch (openErr) { | ||
| outChannel.appendLine(`Error opening audio in WebView: ${openErr}`); | ||
| outChannel.appendLine(`Attempting VS Code native audio player...`); | ||
|
|
||
| try { | ||
| await vscode.commands.executeCommand('vscode.open', vscode.Uri.file(outfile), { | ||
| viewColumn: vscode.ViewColumn.Beside, | ||
| preview: false, | ||
| preserveFocus: false | ||
| }); | ||
| outChannel.appendLine(`Opened audio in VS Code native audio player: ${outfile}`); | ||
| } catch (nativeErr) { | ||
| outChannel.appendLine(`Error opening native player: ${nativeErr}`); | ||
| outChannel.appendLine(`Falling back to system default player...`); | ||
|
|
||
| // Fallback to system default player | ||
| const cmd = process.platform === 'win32' ? `start "${outfile}"` : | ||
| process.platform === 'darwin' ? `open "${outfile}"` : | ||
| `xdg-open "${outfile}"`; | ||
|
|
||
| child.exec(cmd, {}, (execErr: Error | null) => { | ||
| if (execErr) { | ||
| outChannel.appendLine(`Fallback launch error: ${execErr.message}`); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new WebView-based audio playback functionality introduced in this PR lacks test coverage. There are no tests for the WebView panel creation, HTML generation (getAudioPlayerHTML), or the audio file cleanup behavior (deleteAudioAfterPlayback setting). Consider adding tests to verify the WebView is created correctly, the HTML template is generated properly, and the file deletion logic works as expected based on the configuration setting.
| let awsProfile = <string>vscode.workspace.getConfiguration().get('speechmarkdown.aws.profile'); | ||
|
|
||
|
|
||
| if(!awsProfile && process.env.AWS_PROFILE) | ||
| { | ||
| awsAccessKeyId = process.env.AWS_ACCESS_KEY_ID; | ||
| awsProfile = process.env.AWS_PROFILE; | ||
|
|
||
| } | ||
|
|
||
| let awsSecretKey = <string>vscode.workspace.getConfiguration().get('speechmarkdown.aws.secretAccessKey'); | ||
| if(!awsSecretKey && process.env.AWS_SECRET_ACCESS_KEY) | ||
|
|
||
| if (!awsProfile) | ||
| { | ||
| awsSecretKey = process.env.AWS_SECRET_ACCESS_KEY; | ||
| awsProfile = 'default'; | ||
|
|
||
| } | ||
|
|
||
|
|
||
| let pollyVoice = <VoiceId>vscode.workspace.getConfiguration().get('speechmarkdown.aws.pollyDefaultVoice'); | ||
|
|
||
| if (!awsAccessKeyId || !awsSecretKey || !awsRegion || !pollyVoice) | ||
| // if (!awsAccessKeyId || !awsSecretKey || !awsRegion || !pollyVoice) | ||
| if (!awsRegion || !pollyVoice) | ||
| { | ||
|
|
||
| output += '\nAWS Configuration Incomplete'; | ||
|
|
||
| if(!awsRegion) | ||
| { | ||
| output += '\n speechmarkdown configuration setting AWS Region not specified'; | ||
| } | ||
|
|
||
| if(!awsAccessKeyId) | ||
| { | ||
| output += '\n speechmarkdown configuration setting AWS Access Key ID not specified'; | ||
| } | ||
|
|
||
| if(!awsSecretKey) | ||
| { | ||
| output += '\n speechmarkdown configuration setting AWS Secret Key not specified'; | ||
| output += '\n speechmarkdown configuration setting AWS Region not specified. Configure in extension settings or set AWS_DEFAULT_REGION environment variable.'; | ||
| } | ||
|
|
||
| if(!pollyVoice) | ||
| { | ||
| output += '\n speechmarkdown configuration setting AWS Polly Voice not specified'; | ||
| output += '\n speechmarkdown configuration setting AWS Polly Voice not specified. Configure in extension settings.'; | ||
| } | ||
| } | ||
| else | ||
| { | ||
|
|
||
| output += `\nAWS Region: ${awsRegion}`; | ||
| output += `\nAWS Profile: ${awsProfile}`; | ||
| output += `\nAWS Polly Voice: ${pollyVoice}`; | ||
|
|
||
| try { | ||
| let client : PollyClient = new PollyClient({ region: awsRegion, | ||
| credentials: { | ||
| accessKeyId: awsAccessKeyId, | ||
| secretAccessKey: awsSecretKey | ||
| } | ||
| }); | ||
| let clientConfig: any = { region: awsRegion }; | ||
| clientConfig.profile = awsProfile; | ||
|
|
||
| outChannel.appendLine('\nInitializing AWS Polly client...'); | ||
|
|
||
|
|
||
|
|
||
| let client : PollyClient = new PollyClient(clientConfig); |
There was a problem hiding this comment.
The new AWS profile-based authentication functionality lacks test coverage. Tests should be added to verify that the profile setting is correctly read from configuration, falls back to AWS_PROFILE environment variable when not set, and defaults to 'default' when neither is configured. Additionally, there should be tests to ensure the AWS SDK client is properly configured with the profile credentials.
Changed
~/.aws/credentialsor~/.aws/config). This is more secure and follows AWS best practicesAdded
speechmarkdown.aws.profile— Specifies which AWS named profile to use for authentication. If not set, falls back to theAWS_PROFILEenvironment variable, then thedefaultprofilespeechmarkdown.deleteAudioAfterPlayback— When enabled (default: on), the temporary audio file is automatically deleted when the audio player panel is closedRemoved
speechmarkdown.aws.accessKeyIdsetting — Replaced by AWS profile-based authenticationMigration
If you previously configured
speechmarkdown.aws.accessKeyIdand the AWS Secret Access Key, switch to using an AWS profile:~/.aws/credentialsor viaaws configurespeechmarkdown.aws.profilein VS Code settings to your profile name (or set it todefaultto use the default profile)