Proper platform handle gitcreds #62
Conversation
feat: New parameter for git credential manager on docker host
|
Any thoughts on this? Happy to update as needed. It's working well for us but figured it's be nice for others having this issue. |
Original author: https://github.com/jt-metatheory This was part of #62
Original author: https://github.com/jt-metatheory This was part of #62
|
@coderabbitai review this |
|
@GabLeRoux: I will review the changes. ✅ Actions performedReview triggered.
|
WalkthroughThe changes introduced in this pull request enhance the documentation and functionality of the Unity Orb project. A new "Manual Deploy" section has been added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant README
participant PrepareEnv
participant BuildJob
participant GitCredential
participant DockerContainer
User->>README: Read "Manual Deploy"
README->>User: Instructions for private orb creation
User->>PrepareEnv: Set up environment
PrepareEnv->>GitCredential: Set Git credentials with PAT
GitCredential->>PrepareEnv: Credentials set
PrepareEnv->>BuildJob: Start build job with PAT
BuildJob->>DockerContainer: Run Docker container for Unity
DockerContainer->>BuildJob: Return license
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: 17
🧹 Outside diff range and nitpick comments (10)
src/scripts/set-gitcredential.sh (1)
1-19: Consider these security and operational recommendations
Token Security:
- Consider using CircleCI's built-in contexts or encrypted environment variables instead of directly passing tokens
- Implement token rotation mechanisms
- Add token scope validation to ensure minimal required permissions
CI/CD Integration:
- Add documentation for required CircleCI contexts and environment variables
- Consider implementing a dry-run mode for testing credential setup
- Add cleanup steps to remove credentials after the build
Observability:
- Add structured logging for better debugging
- Consider implementing credential validation checks
- Add metrics for credential setup success/failure
Would you like me to provide example implementations for any of these recommendations?
src/scripts/macos/set-gitcredential.sh (1)
5-9: Enhance token validation and error messagingWhile the basic check for token presence is good, consider:
- Validating the token format (e.g., length, character set)
- Providing more descriptive messages about what actions are being skipped
- Using stderr for diagnostic messages
if [ -z "${GIT_PRIVATE_TOKEN}" ] then - echo "GIT_PRIVATE_TOKEN unset skipping" + echo >&2 "Warning: GIT_PRIVATE_TOKEN is not set. Git credential configuration will be skipped." + exit 0 else - echo "GIT_PRIVATE_TOKEN is set configuring git credentials" + if [[ ! "${GIT_PRIVATE_TOKEN}" =~ ^gh[ps]_[a-zA-Z0-9]{36,251}$ ]]; then + echo >&2 "Error: GIT_PRIVATE_TOKEN appears to be in an invalid format" + exit 1 + fi + echo "Configuring git credentials using provided token..."src/scripts/windows/return-license.sh (3)
6-15: Enhance error messaging for better debugging.While the error handling is well-structured, consider making the error message more specific by including the exit status and potential failure reason.
if [ "$exit_status" -ne 0 ]; then - printf '%s\n' 'The script did not complete successfully.' + printf 'Script failed with exit status %d. This might be due to Unity license return failure or Docker container issues.\n' "$exit_status"
28-30: Potential portability issues with hard-coded Windows paths.The Visual Studio paths are hard-coded which could cause issues if Visual Studio is installed in a different location or if different versions are used.
Consider:
- Making these paths configurable via environment variables
- Adding validation to check if these paths exist before mounting
- Documenting the required Visual Studio components and versions
Line range hint
36-38: Improve Unity command robustness and error handling.The Unity command execution has several potential issues:
- The wildcard in the Unity path could match multiple versions
- The error output handling could be improved
Consider this improved version:
-docker exec "$CONTAINER_NAME" powershell '& "C:\Program Files\Unity\Hub\Editor\*\Editor\Unity.exe" -returnlicense -batchmode -quit -nographics -username $Env:UNITY_USERNAME -password $Env:UNITY_PASSWORD -logfile | Out-Host' +docker exec "$CONTAINER_NAME" powershell ' +$unityPath = Get-ChildItem "C:\Program Files\Unity\Hub\Editor\*\Editor\Unity.exe" | Select-Object -First 1 +if (-not $unityPath) { + Write-Error "Unity executable not found" + exit 1 +} +& $unityPath -returnlicense -batchmode -quit -nographics -username $Env:UNITY_USERNAME -password $Env:UNITY_PASSWORD -logFile unity.log 2>&1 | Tee-Object -FilePath return-license.log +if ($LASTEXITCODE -ne 0) { + Write-Error "Unity license return failed with exit code $LASTEXITCODE" + exit $LASTEXITCODE +}'src/scripts/windows/set-gitcredential.sh (1)
18-30: Enhance error loggingWhile the error handling is good, consider enhancing the logging to include:
- The specific error that occurred
- The step where the failure happened
trap_exit() { local exit_status="$?" if [ "$exit_status" -ne 0 ]; then - printf '%s\n' 'The script did not complete successfully.' + printf 'Error: Script failed with exit code %d\n' "$exit_status" + printf 'Error occurred in function: %s\n' "${FUNCNAME[1]:-main}" printf '%s\n' "Removing the container \"$CONTAINER_NAME\"." docker rm -f "$CONTAINER_NAME" &> /dev/null || true exit "$exit_status" fi }README.md (2)
34-36: Consider enhancing the section introductionWhile the introduction is clear, it would be helpful to briefly explain why someone might need a private orb (e.g., for proprietary configurations or internal team usage).
### Manual Deploy -If you want a private orb for your build env. The following steps allow you to do so. These are adapted from the CircleCI +If you need a private orb for proprietary configurations or internal team usage, the following steps will help you create one. These instructions are adapted from the CircleCI [Manual Orb Authoring Process](https://circleci.com/docs/orb-author-validate-publish/#publish-your-orb)
37-41: Add brief explanations for each commandThe commands are correct, but adding brief explanations would help users understand what each step accomplishes.
-1. `circleci namespace create <name> --org-id <your-organization-id>` -2. `circleci orb create <my-namespace>/<my-orb-name> --private` -3. `circleci orb pack src > unity-orb.yml` -4. `circleci orb publish unity-orb.yml <my-namespace>/<my-orb-name>@dev:first` -5. `circleci orb publish promote <my-namespace>/<my-orb-name>@dev:first patch` +1. Create a namespace for your organization: + `circleci namespace create <name> --org-id <your-organization-id>` + +2. Create a new private orb in your namespace: + `circleci orb create <my-namespace>/<my-orb-name> --private` + +3. Package your orb's source code: + `circleci orb pack src > unity-orb.yml` + +4. Publish your orb to the registry in development: + `circleci orb publish unity-orb.yml <my-namespace>/<my-orb-name>@dev:first` + +5. Promote the development version to production: + `circleci orb publish promote <my-namespace>/<my-orb-name>@dev:first patch`src/commands/prepare-env.yml (1)
27-31: Enhance the parameter description with security guidanceWhile the parameter is correctly typed as
env_var_name, the description should be expanded to include:
- Required GitHub PAT permissions/scopes
- Security best practices
- Instructions for token generation
Apply this diff to improve the description:
description: | - Enter your Github PAT that you want to pass to the credential manager + The environment variable name containing your GitHub Personal Access Token (PAT). + The PAT should have the following permissions: + - read:packages scope for package registry access + - repo scope for private repository access + + Generate a new token at: https://github.com/settings/tokens + + Security Note: Ensure the PAT has minimal required permissions and is stored securely.src/jobs/build.yml (1)
36-40: Enhance the parameter documentation with security best practices.While the parameter is correctly configured, the description could be more detailed to help users set it up securely:
- Specify required GitHub PAT scopes/permissions
- Add link to GitHub's PAT creation documentation
- Mention security implications and token management best practices
git-private-token-var-name: type: env_var_name default: "GIT_PRIVATE_TOKEN" description: | - Enter your Github PAT that you want to pass to the credential manager + GitHub Personal Access Token (PAT) used by the credential manager for Unity Package Manager checkouts. + Required scopes: read:packages, repo (if accessing private repositories) + For token creation instructions, see: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/creating-a-personal-access-token + Security note: Use fine-grained PATs with minimal permissions when possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
README.md(1 hunks)src/commands/prepare-env.yml(2 hunks)src/jobs/build.yml(2 hunks)src/scripts/linux/set-gitcredential.sh(1 hunks)src/scripts/macos/set-gitcredential.sh(1 hunks)src/scripts/set-gitcredential.sh(1 hunks)src/scripts/windows/return-license.sh(1 hunks)src/scripts/windows/set-gitcredential.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.9.0-1)
src/scripts/windows/set-gitcredential.sh
[warning] 6-6: unity_project_full_path appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (6)
src/scripts/set-gitcredential.sh (1)
16-19: 🛠️ Refactor suggestion
Improve platform-specific script handling and add error cases
The platform-specific script execution needs better error handling and validation:
-if [ "$PLATFORM" = "linux" ]; then eval "$SCRIPT_GIT_CREDENTIAL_LINUX";
-elif [ "$PLATFORM" = "macos" ]; then eval "$SCRIPT_GIT_CREDENTIAL_MACOS";
-elif [ "$PLATFORM" = "windows" ]; then eval "$SCRIPT_GIT_CREDENTIAL_WINDOWS";
-fi
+# Validate platform-specific script variables
+declare -A platform_scripts=(
+ ["linux"]="$SCRIPT_GIT_CREDENTIAL_LINUX"
+ ["macos"]="$SCRIPT_GIT_CREDENTIAL_MACOS"
+ ["windows"]="$SCRIPT_GIT_CREDENTIAL_WINDOWS"
+)
+
+if [ -z "${platform_scripts[$PLATFORM]:-}" ]; then
+ echo "Error: Unsupported platform: $PLATFORM"
+ exit 1
+fi
+
+if [ -z "${platform_scripts[$PLATFORM]}" ]; then
+ echo "Error: Git credential script for $PLATFORM is not set"
+ exit 1
+fi
+
+# Execute platform-specific script
+if ! eval "${platform_scripts[$PLATFORM]}"; then
+ echo "Error: Failed to configure Git credentials for $PLATFORM"
+ exit 1
+fiLet's verify the platform-specific scripts:
src/scripts/linux/set-gitcredential.sh (1)
1-4: Review the script execution configuration.
The #!/bin/false shebang prevents direct execution of this script. If this is intentional (e.g., if the script should only be sourced), consider adding a comment explaining this. Also, disabling SC2154 might hide unset variable issues.
✅ Verification successful
The #!/bin/false shebang is correct for this use case
The script is designed to be included and sourced within CircleCI's prepare-env.yml configuration, not executed directly. The #!/bin/false shebang is intentional as it prevents accidental direct execution while allowing the script to be sourced properly through CircleCI's << include() >> directive. The SC2154 shellcheck disable is also appropriate as the environment variables are provided by CircleCI's configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this script is sourced elsewhere
rg -l "source.*set-gitcredential.sh" || rg -l "\. .*set-gitcredential.sh"
Length of output: 74
Script:
#!/bin/bash
# Let's check if there are any references to this script in other files
rg -l "set-gitcredential"
# Also check the script's permissions to understand if it's meant to be executable
ls -l src/scripts/linux/set-gitcredential.sh
# Look for any documentation or comments that might explain the execution model
head -n 20 src/scripts/linux/set-gitcredential.sh
Length of output: 1043
Script:
#!/bin/bash
# Check how this script is used in prepare-env.yml
cat src/commands/prepare-env.yml
Length of output: 3738
src/scripts/windows/return-license.sh (1)
19-23: Verify container existence check logic.
The container existence check is good practice, but the comment suggests this is specifically for failed builds. Consider adding more comprehensive checks.
✅ Verification successful
Container management logic is appropriate for the use case
The container existence check and recovery mechanism is well-implemented and serves its specific purpose. The script:
- Properly checks for existing containers before creating a new one
- Has appropriate error handling via trap_exit
- Contains necessary volume mounts for Visual Studio
- Includes proper cleanup after license return
- Uses environment variables consistently
The comment accurately describes the recovery scenario where the script needs to create a new container to return the Unity license after a failed build. This is a valid approach given the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar recovery mechanisms exist in other scripts
rg -l "docker ps.*filter.*Name=\$CONTAINER_NAME"
Length of output: 49
Script:
#!/bin/bash
# Let's try a broader search for Docker container management patterns
rg -l "docker ps.*filter"
# Also search for similar container name checks
rg -l "docker ps.*Name"
# Search for container recovery or cleanup patterns
rg "docker (run|start|rm).*CONTAINER_NAME" -l
Length of output: 313
Script:
#!/bin/bash
# Let's examine the container management logic in all related scripts
rg -A 5 -B 5 "docker ps.*filter.*Name=\$CONTAINER_NAME" src/scripts/windows/return-license.sh
# Check how other scripts handle container management
rg -A 5 "docker (run|start|rm).*CONTAINER_NAME" src/scripts/windows/test.sh src/scripts/windows/build.sh src/scripts/windows/set-gitcredential.sh
Length of output: 1178
Script:
#!/bin/bash
# Let's see the full content of return-license.sh to understand the complete container management flow
cat src/scripts/windows/return-license.sh
Length of output: 1573
src/scripts/windows/set-gitcredential.sh (2)
11-12: Clarify platform requirements
The script is located in a 'windows' directory but includes OS detection. This might cause confusion about platform compatibility.
Let's check the OS-specific handling:
✅ Verification successful
Let me gather more information about the OS-specific handling in these scripts.
OS detection is correctly used for platform-specific Docker operations
The script's location in the 'windows' directory is not problematic as the OS detection is used appropriately. The script runs in a Linux environment (CircleCI) and performs Docker operations to configure git credentials for a Windows container. The detect-os function is a shared utility that helps ensure proper execution across different CI environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how detect-os is used across the codebase
rg -A 5 "detect-os"
Length of output: 2573
Script:
#!/bin/bash
# Check the content of utils.sh to see the OS detection logic
cat src/scripts/utils.sh
# Check the content of the windows-specific script
cat src/scripts/windows/set-gitcredential.sh
Length of output: 2685
9-9: Review the security implications of using eval
Using eval for importing scripts can be dangerous if the content of SCRIPT_UTILS is not properly sanitized or comes from an untrusted source.
Let's verify the source and content of SCRIPT_UTILS:
✅ Verification successful
Using eval is safe in this context
The use of eval "$SCRIPT_UTILS" is safe because:
SCRIPT_UTILSis populated by CircleCI's<< include(scripts/utils.sh) >>directive, which inlines the script content at build time- The utils.sh file contains only the
detect-osfunction used for platform detection - The script is part of a controlled CI/CD environment where the content is version controlled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how SCRIPT_UTILS is defined
rg -l "SCRIPT_UTILS="
# Check the content of utils.sh
fd -t f "utils.sh"
Length of output: 61
Script:
#!/bin/bash
# Check how SCRIPT_UTILS is defined in the codebase
rg "SCRIPT_UTILS=" -A 2
# Check the content of utils.sh
cat src/scripts/utils.sh
Length of output: 637
Script:
#!/bin/bash
# Let's check where and how this script is sourced
rg -B 2 "set-gitcredential.sh"
# Check the directory structure and related files
fd -t f -p "src/scripts/(windows|linux|macos)/*" --exec ls -l
Length of output: 2050
Script:
#!/bin/bash
# Let's check the content of prepare-env.yml to understand how these scripts are included
cat src/commands/prepare-env.yml
# Also check the main set-gitcredential.sh that's being included
fd -t f "set-gitcredential.sh" --exclude "*(windows|linux|macos)*" --exec cat {}
Length of output: 8401
README.md (1)
33-41: Documentation changes look good!
The new section provides clear instructions for private orb creation, which aligns well with the PR's objective of improving Git credentials handling. The steps are accurate and follow CircleCI's best practices.
| #!/usr/bin/env bash | ||
| # shellcheck disable=SC2034 | ||
|
|
||
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" | ||
| readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH" | ||
|
|
||
| # Import "utils.sh". | ||
| eval "$SCRIPT_UTILS" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and strict mode for improved robustness
The script setup needs additional safeguards to prevent potential issues:
#!/usr/bin/env bash
-# shellcheck disable=SC2034
+# shellcheck disable=SC2034 # Justification: Variables are used in evaluated scripts
+
+set -euo pipefail # Exit on error, undefined vars, and pipe failures
+
+if [ -z "${CIRCLE_WORKING_DIRECTORY:-}" ] || [ -z "${PARAM_PROJECT_PATH:-}" ]; then
+ echo "Error: Required environment variables CIRCLE_WORKING_DIRECTORY and PARAM_PROJECT_PATH must be set"
+ exit 1
+fi
readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}"
readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH"
# Import "utils.sh".
+if ! eval "$SCRIPT_UTILS"; then
+ echo "Error: Failed to import utils.sh"
+ exit 1
+fi
-eval "$SCRIPT_UTILS"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/usr/bin/env bash | |
| # shellcheck disable=SC2034 | |
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" | |
| readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH" | |
| # Import "utils.sh". | |
| eval "$SCRIPT_UTILS" | |
| #!/usr/bin/env bash | |
| # shellcheck disable=SC2034 # Justification: Variables are used in evaluated scripts | |
| set -euo pipefail # Exit on error, undefined vars, and pipe failures | |
| if [ -z "${CIRCLE_WORKING_DIRECTORY:-}" ] || [ -z "${PARAM_PROJECT_PATH:-}" ]; then | |
| echo "Error: Required environment variables CIRCLE_WORKING_DIRECTORY and PARAM_PROJECT_PATH must be set" | |
| exit 1 | |
| fi | |
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" | |
| readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH" | |
| # Import "utils.sh". | |
| if ! eval "$SCRIPT_UTILS"; then | |
| echo "Error: Failed to import utils.sh" | |
| exit 1 | |
| fi |
| # Detect host OS. | ||
| detect-os | ||
|
|
||
| # Expand environment name variable parameters. | ||
| readonly git_private_token="${!PARAM_GIT_PRIVATE_TOKEN}" |
There was a problem hiding this comment.
Enhance security for Git token handling
The current token handling implementation needs additional security measures:
# Detect host OS.
-detect-os
+if ! detect-os; then
+ echo "Error: Failed to detect operating system"
+ exit 1
+fi
# Expand environment name variable parameters.
+if [ -z "${PARAM_GIT_PRIVATE_TOKEN:-}" ]; then
+ echo "Error: PARAM_GIT_PRIVATE_TOKEN environment variable name must be set"
+ exit 1
+fi
+
readonly git_private_token="${!PARAM_GIT_PRIVATE_TOKEN}"
+
+if [ -z "$git_private_token" ]; then
+ echo "Error: Git private token cannot be empty"
+ exit 1
+fi
+
+# Mask token in debug output
+if [ "${BASH_VERBOSE:-}" = "true" ]; then
+ export PS4='+ $(date "+%Y-%m-%d %H:%M:%S") ${BASH_SOURCE##*/}:${LINENO}: '
+ set -x
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Detect host OS. | |
| detect-os | |
| # Expand environment name variable parameters. | |
| readonly git_private_token="${!PARAM_GIT_PRIVATE_TOKEN}" | |
| # Detect host OS. | |
| if ! detect-os; then | |
| echo "Error: Failed to detect operating system" | |
| exit 1 | |
| fi | |
| # Expand environment name variable parameters. | |
| if [ -z "${PARAM_GIT_PRIVATE_TOKEN:-}" ]; then | |
| echo "Error: PARAM_GIT_PRIVATE_TOKEN environment variable name must be set" | |
| exit 1 | |
| fi | |
| readonly git_private_token="${!PARAM_GIT_PRIVATE_TOKEN}" | |
| if [ -z "$git_private_token" ]; then | |
| echo "Error: Git private token cannot be empty" | |
| exit 1 | |
| fi | |
| # Mask token in debug output | |
| if [ "${BASH_VERBOSE:-}" = "true" ]; then | |
| export PS4='+ $(date "+%Y-%m-%d %H:%M:%S") ${BASH_SOURCE##*/}:${LINENO}: ' | |
| set -x | |
| fi |
| git config --global credential.helper store | ||
| git config --global --replace-all url."https://token:[email protected]/".insteadOf ssh://[email protected]/ | ||
| git config --global --add url."https://token:[email protected]/".insteadOf [email protected] | ||
| git config --global --add url."https://token:[email protected]/".insteadOf "https://github.com/" | ||
|
|
||
| git config --global url."https://ssh:[email protected]/".insteadOf "ssh://[email protected]/" | ||
| git config --global url."https://git:[email protected]/".insteadOf "[email protected]:" | ||
|
|
There was a problem hiding this comment.
Limit the scope of Git configurations and enhance security.
Several security and scope concerns:
- Using
--globalaffects all repositories credential.helper storesaves credentials in plaintext- No cleanup of sensitive configurations
Consider these improvements:
-git config --global credential.helper store
+# Use git credential cache instead of store
+git config credential.helper 'cache --timeout=3600'
-git config --global --replace-all url."https://token:[email protected]/".insteadOf ssh://[email protected]/
+# Scope configurations to the repository instead of global
+git config --replace-all url."https://token:[email protected]/".insteadOf ssh://[email protected]/Also, add a cleanup function:
cleanup_git_config() {
git config --unset-all url."https://token:$GIT_PRIVATE_TOKEN@github.com/".insteadOf
git config --unset credential.helper
}
trap cleanup_git_config EXIT| #!/bin/false | ||
| # shellcheck shell=bash | ||
| # shellcheck disable=SC2154 | ||
|
|
||
| if [ -z "${GIT_PRIVATE_TOKEN}" ] | ||
| then | ||
| echo "GIT_PRIVATE_TOKEN unset skipping" | ||
| else | ||
| echo "GIT_PRIVATE_TOKEN is set configuring git credentials" | ||
|
|
||
| git config --global credential.helper store | ||
| git config --global --replace-all url."https://token:[email protected]/".insteadOf ssh://[email protected]/ | ||
| git config --global --add url."https://token:[email protected]/".insteadOf [email protected] | ||
| git config --global --add url."https://token:[email protected]/".insteadOf "https://github.com/" | ||
|
|
||
| git config --global url."https://ssh:[email protected]/".insteadOf "ssh://[email protected]/" | ||
| git config --global url."https://git:[email protected]/".insteadOf "[email protected]:" | ||
|
|
||
| fi | ||
|
|
||
| echo "---------- git config --list -------------" | ||
| git --no-pager config --list | ||
|
|
||
| echo "---------- git config --list --show-origin -------------" | ||
| git --no-pager config --list --show-origin |
There was a problem hiding this comment.
💡 Codebase verification
Cross-platform scripts exist but macOS version is empty
The verification shows that:
- All platforms (Linux, Windows, macOS) have corresponding scripts
- Linux and Windows versions have consistent git credential configurations
- The macOS version (
src/scripts/macos/set-gitcredential.sh) exists but appears to be empty - There's also a root-level script with identical content to the Linux version
🔗 Analysis chain
Verify cross-platform consistency.
Ensure similar scripts exist for other platforms (Windows/macOS) with consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar scripts in other platform directories
echo "Checking for platform-specific scripts..."
fd -t f "set-gitcredential" src/scripts
# Compare script contents for consistency
echo -e "\nComparing script contents..."
for script in $(fd -t f "set-gitcredential" src/scripts); do
echo "=== $script ==="
cat "$script" | grep -E "credential.helper|url\."
done
Length of output: 3324
| echo "---------- git config --list -------------" | ||
| git --no-pager config --list | ||
|
|
||
| echo "---------- git config --list --show-origin -------------" | ||
| git --no-pager config --list --show-origin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secure sensitive information in logs.
The current configuration display might leak sensitive tokens in logs. Consider filtering sensitive information before display.
Add filtering:
echo "---------- git config --list -------------"
-git --no-pager config --list
+git --no-pager config --list | sed -e 's/\(url.*token:\)[^@]*\(@.*\)/\1[FILTERED]\2/'
echo "---------- git config --list --show-origin -------------"
-git --no-pager config --list --show-origin
+git --no-pager config --list --show-origin | sed -e 's/\(url.*token:\)[^@]*\(@.*\)/\1[FILTERED]\2/'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "---------- git config --list -------------" | |
| git --no-pager config --list | |
| echo "---------- git config --list --show-origin -------------" | |
| git --no-pager config --list --show-origin | |
| echo "---------- git config --list -------------" | |
| git --no-pager config --list | sed -e 's/\(url.*token:\)[^@]*\(@.*\)/\1[FILTERED]\2/' | |
| echo "---------- git config --list --show-origin -------------" | |
| git --no-pager config --list --show-origin | sed -e 's/\(url.*token:\)[^@]*\(@.*\)/\1[FILTERED]\2/' |
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" | ||
| readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable declaration
The unity_project_full_path variable is declared but never used in the script.
readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}"
-readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" | |
| readonly unity_project_full_path="$base_dir/$PARAM_PROJECT_PATH" | |
| readonly base_dir="${CIRCLE_WORKING_DIRECTORY/\~/$HOME}" |
🧰 Tools
🪛 Shellcheck (0.9.0-1)
[warning] 6-6: unity_project_full_path appears unused. Verify use (or export if used externally).
(SC2034)
| echo "---------- git config --list -------------" | ||
| docker exec "$CONTAINER_NAME" "git --no-pager config --list" | ||
|
|
||
| echo "---------- git config --list --show-origin -------------" | ||
| docker exec "$CONTAINER_NAME" "git --no-pager config --list --show-origin" |
There was a problem hiding this comment.
Secure debug output
The current debug output might expose sensitive information including the Git token.
-echo "---------- git config --list -------------"
-docker exec "$CONTAINER_NAME" "git --no-pager config --list"
-
-echo "---------- git config --list --show-origin -------------"
-docker exec "$CONTAINER_NAME" "git --no-pager config --list --show-origin"
+echo "---------- git config (sanitized) -------------"
+docker exec "$CONTAINER_NAME" "git --no-pager config --list | sed -E 's/(oauth|token):[^@]+@/\1:***@/g'"
+
+echo "---------- git config origins -------------"
+docker exec "$CONTAINER_NAME" "git --no-pager config --list --show-origin | grep -v 'url\.'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "---------- git config --list -------------" | |
| docker exec "$CONTAINER_NAME" "git --no-pager config --list" | |
| echo "---------- git config --list --show-origin -------------" | |
| docker exec "$CONTAINER_NAME" "git --no-pager config --list --show-origin" | |
| echo "---------- git config (sanitized) -------------" | |
| docker exec "$CONTAINER_NAME" "git --no-pager config --list | sed -E 's/(oauth|token):[^@]+@/\1:***@/g'" | |
| echo "---------- git config origins -------------" | |
| docker exec "$CONTAINER_NAME" "git --no-pager config --list --show-origin | grep -v 'url\.'" |
| docker exec "$CONTAINER_NAME" "git config --global credential.helper store" | ||
| docker exec "$CONTAINER_NAME" "git config --global --replace-all url.\"https://token:[email protected]/\".insteadOf ssh://[email protected]/" | ||
| docker exec "$CONTAINER_NAME" "git config --global --add url.\"https://token:[email protected]/\".insteadOf [email protected]" | ||
| docker exec "$CONTAINER_NAME" "git config --global --add url.\"https://token:[email protected]/\".insteadOf \"https://github.com/\"" | ||
|
|
||
| docker exec "$CONTAINER_NAME" "git config --global url.\"https://ssh:[email protected]/\".insteadOf \"ssh://[email protected]/\"" | ||
| docker exec "$CONTAINER_NAME" "git config --global url.\"https://git:[email protected]/\".insteadOf \"[email protected]:\"" | ||
|
|
There was a problem hiding this comment.
Security and performance improvements needed
Several concerns with the current implementation:
- Security: The token is stored in plain text in Git config and might be visible in logs
- Performance: Multiple similar Docker exec calls could be combined
- Validation: The token format is not validated before use
Consider these improvements:
- docker exec "$CONTAINER_NAME" "git config --global credential.helper store"
- docker exec "$CONTAINER_NAME" "git config --global --replace-all url.\"https://token:[email protected]/\".insteadOf ssh://[email protected]/"
- docker exec "$CONTAINER_NAME" "git config --global --add url.\"https://token:[email protected]/\".insteadOf [email protected]"
- docker exec "$CONTAINER_NAME" "git config --global --add url.\"https://token:[email protected]/\".insteadOf \"https://github.com/\""
-
- docker exec "$CONTAINER_NAME" "git config --global url.\"https://ssh:[email protected]/\".insteadOf \"ssh://[email protected]/\""
- docker exec "$CONTAINER_NAME" "git config --global url.\"https://git:[email protected]/\".insteadOf \"[email protected]:\""
+ # Validate token format
+ if [[ ! $git_private_token =~ ^gh[ps]_[a-zA-Z0-9]{36}$ ]]; then
+ echo "Error: Invalid GitHub token format"
+ exit 1
+ fi
+
+ # Combine all git configs into a single command
+ docker exec "$CONTAINER_NAME" /bin/bash -c '
+ git config --global credential.helper store
+ git config --global credential.helper "store --file=/dev/null" # Don't persist credentials
+ for protocol in "ssh://[email protected]/" "[email protected]" "https://github.com/"; do
+ git config --global --add url."https://oauth:[email protected]/".insteadOf "$protocol"
+ done'Committable suggestion skipped: line range outside the PR's diff.
| - run: | ||
| name: Set Git Credentials | ||
| no_output_timeout: << parameters.no_output_timeout >> | ||
| environment: | ||
| PARAM_GIT_PRIVATE_TOKEN: << parameters.git-private-token-var-name >> | ||
| PARAM_PROJECT_PATH: << parameters.project-path >> | ||
| SCRIPT_UTILS: << include(scripts/utils.sh) >> | ||
| SCRIPT_GIT_CREDENTIAL_LINUX: << include(scripts/linux/set-gitcredential.sh) >> | ||
| SCRIPT_GIT_CREDENTIAL_WINDOWS: << include(scripts/windows/set-gitcredential.sh) >> | ||
| SCRIPT_GIT_CREDENTIAL_MACOS: << include(scripts/macos/set-gitcredential.sh) >> | ||
| command: << include(scripts/set-gitcredential.sh) >> |
There was a problem hiding this comment.
💡 Codebase verification
Security concerns identified in Git credential handling
Several security issues need to be addressed in the credential handling scripts:
- The scripts expose credentials by printing the full git config (including tokens) to logs via
git config --list - Using
--globalfor git config affects the entire user environment instead of being scoped to the repository - No cleanup of credentials after the build process
- Token is exposed in URL rewrites which may appear in logs
Key locations requiring changes:
src/scripts/linux/set-gitcredential.sh: Removegit config --listcommands, use--localinstead of--globalsrc/scripts/windows/set-gitcredential.sh: Remove config listing, use local scopesrc/scripts/macos/set-gitcredential.sh: Same issues as Linux script
Recommended improvements:
- Replace
--globalwith--localin all git config commands - Remove the
git config --listdebug commands - Add cleanup steps to remove credentials after use
- Consider using git credential store with input redirection instead of embedding tokens in URLs
🔗 Analysis chain
Verify secure credential handling in external scripts
The step correctly handles different operating systems, but we should verify the security of credential handling in the referenced scripts.
Let's check the referenced scripts for secure practices:
Additionally, consider these security improvements:
- Add a cleanup step to remove credentials after the build
- Use
--localinstead of--globalfor git config to avoid affecting other repositories
Let me gather more information about the scripts since the previous attempt didn't show their contents.
Let's examine the content of these scripts to understand the complete credential handling flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify credential handling in the external scripts
# Check for potential security issues in credential handling
# Check if scripts exist and examine their content
for script in "scripts/linux/set-gitcredential.sh" "scripts/windows/set-gitcredential.sh" "scripts/macos/set-gitcredential.sh"; do
echo "Checking $script:"
if [ -f "$script" ]; then
# Look for potential security issues
rg -i "echo.*token" "$script" || true
rg "git.*config.*--global" "$script" || true
rg "credential.helper" "$script" || true
fi
done
Length of output: 833
Script:
#!/bin/bash
# Let's first find the actual location of these scripts
fd -t f "set-gitcredential.sh"
# Then let's examine the main script that orchestrates the credential setup
fd -t f "scripts/set-gitcredential.sh" || echo "Main script not found"
# Let's also check for any git credential related patterns in the entire codebase
rg -i "git.*credential" --type sh
Length of output: 1176
Script:
#!/bin/bash
# Check the content of all git credential related scripts
echo "=== Main orchestrator script ==="
cat src/scripts/set-gitcredential.sh
echo -e "\n=== Linux script ==="
cat src/scripts/linux/set-gitcredential.sh
echo -e "\n=== Windows script ==="
cat src/scripts/windows/set-gitcredential.sh
echo -e "\n=== MacOS script ==="
cat src/scripts/macos/set-gitcredential.sh
Length of output: 5005
| @@ -113,6 +118,7 @@ steps: | |||
| unity-password-var-name: << parameters.unity-password-var-name >> | |||
| unity-serial-var-name: << parameters.unity-serial-var-name >> | |||
| unity-license-var-name: << parameters.unity-license-var-name >> | |||
| git-private-token-var-name: << parameters.git-private-token-var-name >> | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential security concerns in git credential handling
The implementation reveals several security considerations:
- The token is exposed in git config list output, which could leak sensitive information in CI logs
- Multiple git config entries are created with the token in plaintext
- The script shows git config --list output which includes credentials
Key locations requiring attention:
src/scripts/linux/set-gitcredential.sh: Remove or mask git config --list commands- Similar changes needed in Windows and macOS credential scripts
🔗 Analysis chain
Verify secure token handling in prepare-env scripts.
The parameter is correctly passed to prepare-env, but we should verify the secure handling of the token in the implementation.
Let me check the implementation details of these files to verify secure token handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check prepare-env implementation for secure token handling
# Check prepare-env command implementation
echo "Checking prepare-env command implementation:"
fd --type f "prepare-env.yml" src/commands
# Check related scripts for secure practices
echo -e "\nChecking credential scripts for secure practices:"
rg -l "GIT_PRIVATE_TOKEN" src/scripts
# Look for potential token exposure in logs
echo -e "\nChecking for potential token logging:"
rg --ignore-case "echo.*token|print.*token" src/scripts
Length of output: 1178
Script:
#!/bin/bash
# Check prepare-env command implementation
echo "=== Prepare-env command implementation ==="
cat src/commands/prepare-env.yml
echo -e "\n=== Linux git credential script ==="
cat src/scripts/linux/set-gitcredential.sh
echo -e "\n=== Check for direct token exposure in git commands ==="
rg "git.*\$GIT_PRIVATE_TOKEN" src/scripts/
Length of output: 5125
Changes
GIT_PRIVATE_TOKENthat mirrors the scripts in the dist folder for the main game.ci code to allow people to enter a git personal access token for Unity Package Manger Checkouts.Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
prepare-env.ymlandbuild.ymlconfigurations to support GitHub Personal Access Token management.Bug Fixes