-
-
Notifications
You must be signed in to change notification settings - Fork 308
fix(orchestrator): secure git authentication via http.extraHeader #786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8a41533
b3bd405
b232700
f80e4f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| import { OrchestratorFolders } from './orchestrator-folders'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 -50Repository: 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.tsRepository: game-ci/unity-builder Length of output: 398 Replace CommonJS Line 33 uses a CommonJS Note: Lines 108, 120, and 132 also contain CommonJS 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 |
||
|
|
||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Harden env cleanup with Line 130 ( 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 (unicorn/prevent-abbreviations) 🤖 Prompt for AI Agents |
||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security claim in input description is too strong.
The
headermode description says the token never appears in git config, but current implementation setshttp.extraHeaderviagit config --global, which persists a token-derived value in global git config. Please adjust wording to avoid a false security guarantee.Proposed wording update
📝 Committable suggestion
🤖 Prompt for AI Agents