Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build-tests-mac.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ jobs:
buildForAllPlatformsMacOS:
name: ${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}
runs-on: macos-latest
continue-on-error: true
strategy:
fail-fast: false
matrix:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/orchestrator-async-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ jobs:
# AWS_STACK_NAME: game-ci-github-pipelines
CHECKS_UPDATE: ${{ github.event.inputs.checksObject }}
run: |
git clone -b orchestrator-develop https://github.com/game-ci/unity-builder
git clone -b main https://github.com/game-ci/unity-builder
cd unity-builder
yarn
ls
Expand Down
6 changes: 6 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ inputs:
required: false
default: ''
description: '[Orchestrator] Github private token to pull from github'
gitAuthMode:
required: false
default: 'header'
description:
'[Orchestrator] How git authentication is configured. "header" (default) uses http.extraHeader so the token
never appears in clone URLs or git config. "url" embeds the token in clone URLs (legacy behavior).'
Comment on lines +111 to +113
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Security claim in input description is too strong.

The header mode description says the token never appears in git config, but current implementation sets http.extraHeader via git config --global, which persists a token-derived value in global git config. Please adjust wording to avoid a false security guarantee.

Proposed wording update
-      '[Orchestrator] How git authentication is configured. "header" (default) uses http.extraHeader so the token
-      never appears in clone URLs or git config. "url" embeds the token in clone URLs (legacy behavior).'
+      '[Orchestrator] How git authentication is configured. "header" (default) uses http.extraHeader to avoid
+      embedding tokens in clone URLs. "url" embeds the token in clone URLs (legacy behavior).'
📝 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.

Suggested change
description:
'[Orchestrator] How git authentication is configured. "header" (default) uses http.extraHeader so the token
never appears in clone URLs or git config. "url" embeds the token in clone URLs (legacy behavior).'
description:
'[Orchestrator] How git authentication is configured. "header" (default) uses http.extraHeader to avoid
embedding tokens in clone URLs. "url" embeds the token in clone URLs (legacy behavior).'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@action.yml` around lines 111 - 113, Update the input description string for
the git authentication mode so it no longer asserts that the token "never"
appears in git config; instead state that "header" uses http.extraHeader and
avoids embedding the token in clone URLs but may write header config entries
(e.g., via git config --global) to git configuration; reference the existing
"description" field and the "header" and "url" mode labels from the action.yml
diff when making this wording change.

githubOwner:
required: false
default: ''
Expand Down
108 changes: 96 additions & 12 deletions dist/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions src/model/build-parameters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class BuildParameters {
public sshAgent!: string;
public sshPublicKeysDirectoryPath!: string;
public providerStrategy!: string;
public gitAuthMode!: string;
public gitPrivateToken!: string;
public awsStackName!: string;
public awsEndpoint?: string;
Expand Down Expand Up @@ -194,6 +195,7 @@ class BuildParameters {
containerRegistryRepository: Input.containerRegistryRepository,
containerRegistryImageVersion: Input.containerRegistryImageVersion,
providerStrategy: OrchestratorOptions.providerStrategy,
gitAuthMode: OrchestratorOptions.gitAuthMode,
buildPlatform: OrchestratorOptions.buildPlatform,
kubeConfig: OrchestratorOptions.kubeConfig,
containerMemory: OrchestratorOptions.containerMemory,
Expand Down
140 changes: 140 additions & 0 deletions src/model/orchestrator/options/orchestrator-folders-auth.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import { OrchestratorFolders } from './orchestrator-folders';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and view its content
cat -n src/model/orchestrator/options/orchestrator-folders-auth.test.ts | head -50

Repository: game-ci/unity-builder

Length of output: 1739


🏁 Script executed:

# Search for require statements in the file
rg -n "require\s*\(" src/model/orchestrator/options/orchestrator-folders-auth.test.ts

Repository: game-ci/unity-builder

Length of output: 398


Replace CommonJS require with ES import to satisfy import/no-commonjs lint.

Line 33 uses a CommonJS require() call that violates the lint rule. Since the module is already mocked via jest.mock() at the top of the file, convert to an ES import statement.

Note: Lines 108, 120, and 132 also contain CommonJS require() calls for OrchestratorSystem that should be similarly updated.

Suggested fix for line 33
+import Orchestrator from '../orchestrator';
 import { OrchestratorFolders } from './orchestrator-folders';
@@
-const mockOrchestrator = require('../orchestrator').default;
+const mockOrchestrator = Orchestrator;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/model/orchestrator/options/orchestrator-folders-auth.test.ts` at line 1,
Replace the CommonJS require() calls for the OrchestratorSystem module with a
single ES import and remove the inline require usages: add an ES import
statement (e.g., import { OrchestratorSystem } from './orchestrator-system';) at
the top alongside the existing jest.mock(), then delete the require(...)
occurrences referenced in the test file (the require in the code that supplies
OrchestratorSystem at the places corresponding to lines 33, 108, 120, and 132).
Ensure tests still reference OrchestratorSystem by name (not via
module.exports), and update any default vs named import form to match how
OrchestratorSystem is exported so the mocked module is used correctly.


jest.mock('../orchestrator', () => ({
__esModule: true,
default: {
buildParameters: {
orchestratorRepoName: 'game-ci/unity-builder',
githubRepo: 'myorg/myrepo',
gitPrivateToken: 'ghp_test123',
gitAuthMode: 'header',
buildGuid: 'test-guid',
projectPath: '',
buildPath: 'Builds',
cacheKey: 'test-cache',
},
lockedWorkspace: '',
},
}));

jest.mock('./orchestrator-options', () => ({
__esModule: true,
default: {
useSharedBuilder: false,
},
}));

jest.mock('../services/core/orchestrator-system', () => ({
OrchestratorSystem: {
Run: jest.fn().mockResolvedValue(''),
},
}));

const mockOrchestrator = require('../orchestrator').default;

describe('OrchestratorFolders git auth', () => {
beforeEach(() => {
jest.clearAllMocks();
});

describe('useHeaderAuth', () => {
it('should return true when gitAuthMode is header', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
expect(OrchestratorFolders.useHeaderAuth).toBe(true);
});

it('should return true when gitAuthMode is undefined (default)', () => {
mockOrchestrator.buildParameters.gitAuthMode = undefined;
expect(OrchestratorFolders.useHeaderAuth).toBe(true);
});

it('should return false when gitAuthMode is url', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'url';
expect(OrchestratorFolders.useHeaderAuth).toBe(false);
});
});

describe('unityBuilderRepoUrl', () => {
it('should not include token in URL when using header auth', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
const url = OrchestratorFolders.unityBuilderRepoUrl;
expect(url).toBe('https://github.com/game-ci/unity-builder.git');
expect(url).not.toContain('ghp_test123');
});

it('should include token in URL when using url auth (legacy)', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'url';
const url = OrchestratorFolders.unityBuilderRepoUrl;
expect(url).toBe('https://ghp_test123@github.com/game-ci/unity-builder.git');
});
});

describe('targetBuildRepoUrl', () => {
it('should not include token in URL when using header auth', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
const url = OrchestratorFolders.targetBuildRepoUrl;
expect(url).toBe('https://github.com/myorg/myrepo.git');
expect(url).not.toContain('ghp_test123');
});

it('should include token in URL when using url auth (legacy)', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'url';
const url = OrchestratorFolders.targetBuildRepoUrl;
expect(url).toBe('https://ghp_test123@github.com/myorg/myrepo.git');
});
});

describe('gitAuthConfigScript', () => {
it('should emit http.extraHeader commands in header mode', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
const script = OrchestratorFolders.gitAuthConfigScript;
expect(script).toContain('http.extraHeader');
expect(script).toContain('GIT_PRIVATE_TOKEN');
expect(script).toContain('Authorization: Basic');
});

it('should emit no-op comment in url mode', () => {
mockOrchestrator.buildParameters.gitAuthMode = 'url';
const script = OrchestratorFolders.gitAuthConfigScript;
expect(script).toContain('legacy');
expect(script).not.toContain('http.extraHeader');
});
});

describe('configureGitAuth', () => {
it('should run git config with http.extraHeader in header mode', async () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
mockOrchestrator.buildParameters.gitPrivateToken = 'ghp_test123';
const { OrchestratorSystem } = require('../services/core/orchestrator-system');

await OrchestratorFolders.configureGitAuth();

// Verify the base64 encoding and extraHeader config are correct
const expectedEncoded = Buffer.from('x-access-token:ghp_test123').toString('base64');
expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining(expectedEncoded));
expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('.extraHeader'));
});

it('should not run git config in url mode', async () => {
mockOrchestrator.buildParameters.gitAuthMode = 'url';
const { OrchestratorSystem } = require('../services/core/orchestrator-system');

await OrchestratorFolders.configureGitAuth();

expect(OrchestratorSystem.Run).not.toHaveBeenCalled();
});

it('should not run git config when no token is available', async () => {
mockOrchestrator.buildParameters.gitAuthMode = 'header';
mockOrchestrator.buildParameters.gitPrivateToken = '';
const originalEnv = process.env.GIT_PRIVATE_TOKEN;
delete process.env.GIT_PRIVATE_TOKEN;
const { OrchestratorSystem } = require('../services/core/orchestrator-system');

await OrchestratorFolders.configureGitAuth();

expect(OrchestratorSystem.Run).not.toHaveBeenCalled();
if (originalEnv !== undefined) process.env.GIT_PRIVATE_TOKEN = originalEnv;
});
Comment on lines +127 to +138
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Harden env cleanup with try/finally and fix naming lint error.

Line 130 (originalEnv) violates unicorn/prevent-abbreviations, and env restoration should be in finally to avoid test pollution if the assertion path throws.

Suggested fix
-      const originalEnv = process.env.GIT_PRIVATE_TOKEN;
+      const originalEnvironment = process.env.GIT_PRIVATE_TOKEN;
       delete process.env.GIT_PRIVATE_TOKEN;
       const { OrchestratorSystem } = require('../services/core/orchestrator-system');

-      await OrchestratorFolders.configureGitAuth();
-
-      expect(OrchestratorSystem.Run).not.toHaveBeenCalled();
-      if (originalEnv !== undefined) process.env.GIT_PRIVATE_TOKEN = originalEnv;
+      try {
+        await OrchestratorFolders.configureGitAuth();
+        expect(OrchestratorSystem.Run).not.toHaveBeenCalled();
+      } finally {
+        if (originalEnvironment === undefined) {
+          delete process.env.GIT_PRIVATE_TOKEN;
+        } else {
+          process.env.GIT_PRIVATE_TOKEN = originalEnvironment;
+        }
+      }
🧰 Tools
🪛 ESLint

[error] 130-130: The variable originalEnv should be named originalEnvironment. A more descriptive name will do too.

(unicorn/prevent-abbreviations)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/model/orchestrator/options/orchestrator-folders-auth.test.ts` around
lines 127 - 138, Rename the short variable originalEnv to a non-abbreviated name
(e.g., originalEnvironment) to satisfy unicorn/prevent-abbreviations and wrap
the environment mutation and the assertion in a try/finally so GIT_PRIVATE_TOKEN
is always restored; specifically, in the test that calls
OrchestratorFolders.configureGitAuth() and asserts OrchestratorSystem.Run not to
have been called, store process.env.GIT_PRIVATE_TOKEN into originalEnvironment,
delete it, run the call and expect, and then restore the env inside finally to
avoid test pollution.

});
});
Loading
Loading