Fixed AWS to work when no secrets specified #686
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to AWS CloudFormation template generation and secret management across multiple files. The changes focus on enhancing the way secrets are defined and handled within cloud infrastructure templates, specifically in AWS-related cloud runner components. Key updates include adding a Changes
Sequence DiagramsequenceDiagram
participant CloudRunner as Cloud Runner
participant AWSTmpl as AWS Templates
participant AWSSec as AWS Secrets Manager
CloudRunner->>AWSTmpl: Generate CloudFormation Template
AWSTmpl->>AWSTmpl: Define Secret References
AWSTmpl->>AWSSec: Create Secret Mappings
AWSSec-->>AWSTmpl: Confirm Secret Configuration
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/model/cloud-runner/providers/aws/aws-job-stack.ts (1)
149-149: Remove redundant logging statement.This logging statement is redundant with the one on line 117.
- CloudRunnerLogger.log(`TaskDef Cloud formation: ${taskDefCloudFormation}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/model/cloud-runner/providers/aws/aws-cloud-formation-templates.ts(1 hunks)src/model/cloud-runner/providers/aws/aws-job-stack.ts(3 hunks)src/model/cloud-runner/providers/aws/cloud-formations/task-definition-formation.ts(1 hunks)
🔇 Additional comments (6)
src/model/cloud-runner/providers/aws/aws-cloud-formation-templates.ts (1)
24-27: LGTM! The secret definition template follows AWS CloudFormation best practices.The template correctly structures secrets with Name and ValueFrom properties, following AWS CloudFormation syntax.
src/model/cloud-runner/providers/aws/cloud-formations/task-definition-formation.ts (2)
129-129: LGTM! Correct configuration for EFS mount points.The ReadOnly flag is properly set to false, allowing write access to the EFS volume.
130-130: LGTM! Clear template marker for secrets injection.The comment provides a clear insertion point for dynamic secret injection, maintaining code clarity.
src/model/cloud-runner/providers/aws/aws-job-stack.ts (3)
73-73: LGTM! Clear template marker for secret definition.The comment provides a consistent marker for secret template insertion.
116-118: LGTM! Enhanced logging for task definition template.Added logging to help debug template generation issues.
146-147: LGTM! Enhanced logging for stack creation.Added logging to help debug stack creation issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/model/cloud-runner/options/cloud-runner-options.ts (2)
77-79: LGTM! Consider documenting the purpose.The getter implementation is correct, providing a sensible default value.
Consider adding a JSDoc comment to document the purpose and usage of this configuration option:
+/** + * Gets the repository name for cloud runner operations. + * @returns {string} The configured repo name or defaults to 'game-ci/unity-builder' + */ static get cloudRunnerRepoName(): string { return CloudRunnerOptions.getInput('cloudRunnerRepoName') || `game-ci/unity-builder`; }
147-149: LGTM! Consider validating the input.The getter implementation is correct, providing a sensible default value.
Consider adding input validation to ensure a positive integer value:
static get cloneDepth(): string { - return CloudRunnerOptions.getInput('cloneDepth') || `50`; + const depth = CloudRunnerOptions.getInput('cloneDepth') || `50`; + const parsedDepth = parseInt(depth, 10); + if (isNaN(parsedDepth) || parsedDepth <= 0) { + throw new Error('cloneDepth must be a positive integer'); + } + return depth; }src/model/build-parameters.ts (2)
70-70: LGTM! Consider adding property documentation.The property declarations are correctly typed.
Consider adding JSDoc comments to document these properties:
+/** The maximum depth when cloning repositories */ public cloneDepth!: number; +/** The repository name used for cloud runner operations */ public cloudRunnerRepoName!: string;Also applies to: 78-78
199-201: LGTM! Consider handling parsing errors.The initialization of properties is correct, with proper fallback values.
Consider adding error handling for the number parsing:
- cloneDepth: Number.parseInt(CloudRunnerOptions.cloneDepth), + cloneDepth: (() => { + const parsed = Number.parseInt(CloudRunnerOptions.cloneDepth); + if (isNaN(parsed)) { + throw new Error('Failed to parse cloneDepth as a number'); + } + return parsed; + })(),Also applies to: 225-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
action.yml(1 hunks)src/model/build-parameters.ts(4 hunks)src/model/cloud-runner/options/cloud-runner-folders.ts(1 hunks)src/model/cloud-runner/options/cloud-runner-options.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- action.yml
🔇 Additional comments (1)
src/model/build-parameters.ts (1)
1-267: Verify alignment with PR objectives.The changes in these files appear to be unrelated to the PR's stated objective of fixing AWS task definition code to handle null secrets. Instead, they add configuration options for clone depth and repository naming.
Please clarify if:
- These changes are intentionally included in this PR
- The AWS-related changes are in other files not shown here
- The PR description needs to be updated to reflect these changes
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 137-137: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
| public static get unityBuilderRepoUrl(): string { | ||
| return `https://${CloudRunner.buildParameters.gitPrivateToken}@github.com/game-ci/unity-builder.git`; | ||
| return `https://${CloudRunner.buildParameters.gitPrivateToken}@github.com/${CloudRunner.buildParameters.unityBuilderRepo}.git`; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security: Avoid exposing git tokens in URLs.
The current implementation has several security concerns:
- Git tokens in URLs can be exposed through logs or error messages.
- Using a read-write token for cloning operations violates the principle of least privilege.
- The
unityBuilderRepovalue is not validated, potentially allowing injection attacks.
Consider these improvements:
- Use environment variables or git configuration for authentication:
- return `https://${CloudRunner.buildParameters.gitPrivateToken}@github.com/${CloudRunner.buildParameters.unityBuilderRepo}.git`;
+ const repo = CloudRunner.buildParameters.unityBuilderRepo;
+ if (!repo.match(/^[a-zA-Z0-9-]+\/[a-zA-Z0-9-]+$/)) {
+ throw new Error('Invalid repository name format');
+ }
+ return `https://github.com/${repo}.git`;-
Configure git to use credentials from environment variables:
-
Use a read-only token specifically for cloning operations.
Let's verify if the repository name format is consistently validated across the codebase:
🏁 Script executed:
git config --global credential.helper store
echo "https://github.com:$GITHUB_TOKEN" > ~/.git-credentialsLength of output: 72
🏁 Script executed:
#!/bin/bash
# Search for other instances where repository names are used in URLs
rg -A 5 'github\.com/.*\.git'
# Search for repository name validation
ast-grep --pattern 'if (!$_repo.match(/^[a-zA-Z0-9-]+\/[a-zA-Z0-9-]+$/)) { $$$_ }'Length of output: 67754
Security issues found – Sensitive token exposure and lack of repository name validation
• The unityBuilderRepoUrl getter directly embeds the git private token in the URL, which can lead to undesired exposure through logs or error messages.
• Our search for consistent repository name validation (using regex checks) in the codebase returned no matches. This confirms that the proposed input validation is missing.
• Similar patterns exist in other getters (e.g., targetBuildRepoUrl), indicating that the issue is not isolated to a single occurrence.
Changes
Updated AWS task definition related code so that it would allow creation of the cloud formation stack and not complain if the ContainerDefinition secrets were null
See Discord discussion
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit
New Features
cloneDepthandcloudRunnerRepoNamefor repository cloning operations.Refactor
Secretsproperty from container definitions.imageRollingVersionproperty type from number to string for better data representation.cloneDepthandcloudRunnerRepoNamein CloudRunner options.Chores