Merge pull request #91 from JessRudder/secure-tmp-files

Uses tmp library to ensure more secure tmp file creation

.licenses/npm/@types/tmp.dep.yml

@@ -0,0 +1,32 @@
+---
+name: "@types/tmp"
+version: 0.2.6
+type: npm
+summary: TypeScript definitions for tmp
+homepage: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/tmp
+license: mit
+licenses:
+- sources: LICENSE
+  text: |2
+        MIT License
+
+        Copyright (c) Microsoft Corporation.
+
+        Permission is hereby granted, free of charge, to any person obtaining a copy
+        of this software and associated documentation files (the "Software"), to deal
+        in the Software without restriction, including without limitation the rights
+        to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+        copies of the Software, and to permit persons to whom the Software is
+        furnished to do so, subject to the following conditions:
+
+        The above copyright notice and this permission notice shall be included in all
+        copies or substantial portions of the Software.
+
+        THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+        IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+        FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+        AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+        LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+        OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+        SOFTWARE
+notices: []

.licenses/npm/tmp.dep.yml

@@ -0,0 +1,32 @@
+---
+name: tmp
+version: 0.2.5
+type: npm
+summary: Temporary file and directory creator
+homepage: http://github.com/raszi/node-tmp
+license: mit
+licenses:
+- sources: LICENSE
+  text: |
+    The MIT License (MIT)
+
+    Copyright (c) 2014 KARASZI István
+
+    Permission is hereby granted, free of charge, to any person obtaining a copy
+    of this software and associated documentation files (the "Software"), to deal
+    in the Software without restriction, including without limitation the rights
+    to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+    copies of the Software, and to permit persons to whom the Software is
+    furnished to do so, subject to the following conditions:
+
+    The above copyright notice and this permission notice shall be included in all
+    copies or substantial portions of the Software.
+
+    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+    AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+    OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+    SOFTWARE.
+notices: []

__tests__/main.test.ts

@@ -66,7 +66,7 @@ function mockInputs(inputs: Record<string, string> = {}): void {
  */
 function verifyStandardResponse(): void {
   expect(core.setOutput).toHaveBeenNthCalledWith(1, 'response', 'Hello, user!')
-  expect(core.setOutput).toHaveBeenNthCalledWith(2, 'response-file', expect.stringContaining('modelResponse.txt'))
+  expect(core.setOutput).toHaveBeenNthCalledWith(2, 'response-file', expect.stringContaining('modelResponse-'))
 }
 
 vi.mock('fs', () => ({
@@ -75,6 +75,19 @@ vi.mock('fs', () => ({
   writeFileSync: mockWriteFileSync,
 }))
 
+// Mocks for tmp module to control temporary file creation and cleanup
+const mockRemoveCallback = vi.fn()
+const mockFileSync = vi.fn().mockReturnValue({
+  name: '/secure/temp/dir/modelResponse-abc123.txt',
+  removeCallback: mockRemoveCallback,
+})
+const mockSetGracefulCleanup = vi.fn()
+
+vi.mock('tmp', () => ({
+  fileSync: mockFileSync,
+  setGracefulCleanup: mockSetGracefulCleanup,
+}))
+
 // Mock MCP and inference modules
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const mockConnectToGitHubMCP = vi.fn() as MockedFunction<any>
@@ -269,4 +282,43 @@ describe('main.ts', () => {
     expect(core.setFailed).toHaveBeenCalledWith(`File for prompt-file was not found: ${promptFile}`)
     expect(mockProcessExit).toHaveBeenCalledWith(1)
   })
+
+  it('creates secure temporary files with proper cleanup', async () => {
+    mockInputs({
+      prompt: 'Test prompt',
+      'system-prompt': 'You are a test assistant.',
+    })
+
+    await run()
+
+    expect(mockSetGracefulCleanup).toHaveBeenCalledOnce()
+
+    expect(mockFileSync).toHaveBeenCalledWith({
+      prefix: 'modelResponse-',
+      postfix: '.txt',
+    })
+
+    expect(core.setOutput).toHaveBeenNthCalledWith(2, 'response-file', '/secure/temp/dir/modelResponse-abc123.txt')
+    expect(mockWriteFileSync).toHaveBeenCalledWith('/secure/temp/dir/modelResponse-abc123.txt', 'Hello, user!', 'utf-8')
+    expect(mockRemoveCallback).toHaveBeenCalledOnce()
+
+    expect(mockProcessExit).toHaveBeenCalledWith(0)
+  })
+
+  it('handles cleanup errors gracefully', async () => {
+    mockRemoveCallback.mockImplementationOnce(() => {
+      throw new Error('Cleanup failed')
+    })
+
+    mockInputs({
+      prompt: 'Test prompt',
+      'system-prompt': 'You are a test assistant.',
+    })
+
+    await run()
+
+    expect(mockRemoveCallback).toHaveBeenCalledOnce()
+    expect(core.warning).toHaveBeenCalledWith('Failed to cleanup temporary file: Error: Cleanup failed')
+    expect(mockProcessExit).toHaveBeenCalledWith(0)
+  })
 })

package-lock.json

@@ -11,9 +11,11 @@
       "dependencies": {
         "@actions/core": "^1.11.1",
         "@modelcontextprotocol/sdk": "^1.15.1",
+        "@types/tmp": "^0.2.6",
         "js-yaml": "^4.1.0",
         "openai": "^5.11.0",
-        "pkce-challenge": "^5.0.0"
+        "pkce-challenge": "^5.0.0",
+        "tmp": "^0.2.4"
       },
       "devDependencies": {
         "@eslint/compat": "^1.3.0",
@@ -2492,6 +2494,12 @@
       "dev": true,
       "license": "MIT"
     },
+    "node_modules/@types/tmp": {
+      "version": "0.2.6",
+      "resolved": "https://registry.npmjs.org/@types/tmp/-/tmp-0.2.6.tgz",
+      "integrity": "sha512-chhaNf2oKHlRkDGt+tiKE2Z5aJ6qalm7Z9rlLdBwmOiAAf09YQvvoLXjWK4HWPF1xU/fqvMgfNfpVoBscA/tKA==",
+      "license": "MIT"
+    },
     "node_modules/@typescript-eslint/eslint-plugin": {
       "version": "8.34.0",
       "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.34.0.tgz",
@@ -8943,6 +8951,15 @@
         "node": ">=14.0.0"
       }
     },
+    "node_modules/tmp": {
+      "version": "0.2.5",
+      "resolved": "https://registry.npmjs.org/tmp/-/tmp-0.2.5.tgz",
+      "integrity": "sha512-voyz6MApa1rQGUxT3E+BK7/ROe8itEx7vD8/HEvt4xwXucvQ5G5oeEiHkmHZJuBO21RpOf+YYm9MOivj709jow==",
+      "license": "MIT",
+      "engines": {
+        "node": ">=14.14"
+      }
+    },
     "node_modules/to-regex-range": {
       "version": "5.0.1",
       "resolved": "https://registry.npmjs.org/to-regex-range/-/to-regex-range-5.0.1.tgz",

package.json

@@ -25,9 +25,11 @@
   "dependencies": {
     "@actions/core": "^1.11.1",
     "@modelcontextprotocol/sdk": "^1.15.1",
+    "@types/tmp": "^0.2.6",
     "js-yaml": "^4.1.0",
     "openai": "^5.11.0",
-    "pkce-challenge": "^5.0.0"
+    "pkce-challenge": "^5.0.0",
+    "tmp": "^0.2.4"
   },
   "devDependencies": {
     "@eslint/compat": "^1.3.0",

src/main.ts

@@ -1,7 +1,6 @@
 import * as core from '@actions/core'
 import * as fs from 'fs'
-import * as os from 'os'
-import * as path from 'path'
+import * as tmp from 'tmp'
 import {connectToGitHubMCP} from './mcp.js'
 import {simpleInference, mcpInference} from './inference.js'
 import {loadContentFromFileOrInput, buildInferenceRequest} from './helpers.js'
@@ -13,14 +12,17 @@ import {
   parseFileTemplateVariables,
 } from './prompt.js'
 
-const RESPONSE_FILE = 'modelResponse.txt'
-
 /**
  * The main function for the action.
  *
  * @returns Resolves when the action is complete.
  */
 export async function run(): Promise<void> {
+  let responseFile: tmp.FileResult | null = null
+
+  // Set up graceful cleanup for temporary files on process exit
+  tmp.setGracefulCleanup()
+
   try {
     const promptFilePath = core.getInput('prompt-file')
     const inputVariables = core.getInput('input')
@@ -93,11 +95,16 @@ export async function run(): Promise<void> {
 
     core.setOutput('response', modelResponse || '')
 
-    const responseFilePath = path.join(tempDir(), RESPONSE_FILE)
-    core.setOutput('response-file', responseFilePath)
+    // Create a secure temporary file instead of using the temp directory directly
+    responseFile = tmp.fileSync({
+      prefix: 'modelResponse-',
+      postfix: '.txt',
+    })
+
+    core.setOutput('response-file', responseFile.name)
 
     if (modelResponse && modelResponse !== '') {
-      fs.writeFileSync(responseFilePath, modelResponse, 'utf-8')
+      fs.writeFileSync(responseFile.name, modelResponse, 'utf-8')
     }
   } catch (error) {
     if (error instanceof Error) {
@@ -107,13 +114,18 @@ export async function run(): Promise<void> {
     }
     // Force exit to prevent hanging on open connections
     process.exit(1)
+  } finally {
+    // Explicit cleanup of temporary file if it was created
+    if (responseFile) {
+      try {
+        responseFile.removeCallback()
+      } catch (cleanupError) {
+        // Log cleanup errors but don't fail the action
+        core.warning(`Failed to cleanup temporary file: ${cleanupError}`)
+      }
+    }
   }
 
   // Force exit to prevent hanging on open connections
   process.exit(0)
 }
-
-function tempDir(): string {
-  const tempDirectory = process.env['RUNNER_TEMP'] || os.tmpdir()
-  return tempDirectory
-}