From 83e462c0be0c43179cfb17e07b8361b5cbce5909 Mon Sep 17 00:00:00 2001 From: Kenneth Feng Date: Thu, 28 Oct 2021 21:39:43 -0400 Subject: [PATCH] plugins/scaffolder-backend: Fix a bug where only file mode 775 is considered executable. Due to the default umask 002 on Ubuntu, the executable files checked out by Git have file mode 100775 (33277), which by mistake, was assumed to be the only possible file mode produced by Git. This bug was introduced in https://github.com/backstage/backstage/pull/7738. There are other possible file modes, such as 100700, 100755, which should be considered executable. This behavior is consistent with how Git behaves. Git only tracks whether a file is executable. This pull request considers a file to be executable as long as there is an execute bit in owner, group, or everyone. Signed-off-by: Kenneth Feng --- .../builtin/publish/githubPullRequest.test.ts | 80 +++++++++++++++++-- .../builtin/publish/githubPullRequest.ts | 7 +- .../actions/builtin/publish/util.test.ts | 32 +++++++- .../actions/builtin/publish/util.ts | 5 ++ 4 files changed, 114 insertions(+), 10 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts index 27c6f1e59d..0d4deb87e0 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.test.ts @@ -254,7 +254,7 @@ describe('createPublishGithubPullRequestAction', () => { }); }); - describe('with executable file', () => { + describe('with executable file mode 755', () => { let input: GithubPullRequestActionInput; let ctx: ActionContext; @@ -268,9 +268,9 @@ describe('createPublishGithubPullRequestAction', () => { mockFs({ [workspacePath]: { - 'file.txt': mockFs.file({ - content: 'Hello there!', - mode: 33277, // File mode: 100755 + 'hello.sh': mockFs.file({ + content: 'echo Hello there!', + mode: 0o100755, }), }, }); @@ -297,8 +297,76 @@ describe('createPublishGithubPullRequestAction', () => { { commit: 'Create my new app', files: { - 'file.txt': { - content: Buffer.from('Hello there!').toString('base64'), + 'hello.sh': { + content: Buffer.from('echo Hello there!').toString('base64'), + encoding: 'base64', + mode: '100755', + }, + }, + }, + ], + }); + }); + + it('creates outputs for the url', async () => { + await instance.handler(ctx); + + expect(ctx.output).toHaveBeenCalledWith( + 'remoteUrl', + 'https://github.com/myorg/myrepo/pull/123', + ); + }); + afterEach(() => { + mockFs.restore(); + jest.resetAllMocks(); + }); + }); + + describe('with executable file mode 775', () => { + let input: GithubPullRequestActionInput; + let ctx: ActionContext; + + beforeEach(() => { + input = { + repoUrl: 'github.com?owner=myorg&repo=myrepo', + title: 'Create my new app', + branchName: 'new-app', + description: 'This PR is really good', + }; + + mockFs({ + [workspacePath]: { + 'hello.sh': mockFs.file({ + content: 'echo Hello there!', + mode: 0o100775, + }), + }, + }); + + ctx = { + createTemporaryDirectory: jest.fn(), + output: jest.fn(), + logger: getRootLogger(), + logStream: new Writable(), + input, + workspacePath, + }; + }); + it('creates a pull request', async () => { + await instance.handler(ctx); + + expect(fakeClient.createPullRequest).toHaveBeenCalledWith({ + owner: 'myorg', + repo: 'myrepo', + title: 'Create my new app', + head: 'new-app', + body: 'This PR is really good', + changes: [ + { + commit: 'Create my new app', + files: { + 'hello.sh': { + content: Buffer.from('echo Hello there!').toString('base64'), encoding: 'base64', mode: '100755', }, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts index e147f49b23..0cceb47d71 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts @@ -16,7 +16,7 @@ import fs from 'fs-extra'; import path from 'path'; -import { parseRepoUrl } from './util'; +import { parseRepoUrl, isExecutable } from './util'; import { GithubCredentialsProvider, @@ -202,10 +202,11 @@ export const createPublishGithubPullRequestAction = ({ .readFileSync(absPath) .toString('base64'); const fileStat = fs.statSync(absPath); - const isExecutable = fileStat.mode === 33277; // aka. 100755 // See the properties of tree items // in https://docs.github.com/en/rest/reference/git#trees - const githubTreeItemMode = isExecutable ? '100755' : '100644'; + const githubTreeItemMode = isExecutable(fileStat.mode) + ? '100755' + : '100644'; // Always use base64 encoding to avoid doubling a binary file in size // due to interpreting a binary file as utf-8 and sending github // the utf-8 encoded content. diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts index f179289624..cc879d4f55 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts @@ -15,7 +15,7 @@ */ import path from 'path'; -import { getRepoSourceDirectory } from './util'; +import { getRepoSourceDirectory, isExecutable } from './util'; describe('getRepoSourceDirectory', () => { it('should return workspace root if no sub folder is given', () => { @@ -55,3 +55,33 @@ describe('getRepoSourceDirectory', () => { ).toEqual(path.join('/', 'var', 'workspace', 'absolute', 'secret')); }); }); + +describe('isExecutable', () => { + it('should return true for file mode 777', () => { + expect(isExecutable(0o100777)).toBe(true); + }); + it('should return true for file mode 775', () => { + expect(isExecutable(0o100775)).toBe(true); + }); + it('should return true for file mode 755', () => { + expect(isExecutable(0o100755)).toBe(true); + }); + it('should return true for file mode 700', () => { + expect(isExecutable(0o100700)).toBe(true); + }); + it('should return true for file mode 770', () => { + expect(isExecutable(0o100770)).toBe(true); + }); + it('should return true for file mode 670', () => { + expect(isExecutable(0o100670)).toBe(true); + }); + it('should return false for file mode 644', () => { + expect(isExecutable(0o100644)).toBe(false); + }); + it('should return false for file mode 600', () => { + expect(isExecutable(0o100600)).toBe(false); + }); + it('should return false for file mode 640', () => { + expect(isExecutable(0o100640)).toBe(false); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts index 58ba5bfdd1..49156e7d97 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts @@ -101,3 +101,8 @@ export const parseRepoUrl = ( return { host, owner, repo, organization, workspace, project }; }; +export const isExecutable = (fileMode: number) => { + const executeBitMask = 0o000111; + const res = fileMode & executeBitMask; + return res > 0; +};