From 850f571acbefa86b8244cdb6c77dc2aa2e26011c Mon Sep 17 00:00:00 2001 From: Desmond Howard Date: Thu, 10 Aug 2023 17:07:18 -0400 Subject: [PATCH] Fix C# CodeQL alerts + add JS/TS to CodeQL (#139) ### Motivation and Context - changes the fix for the C# CodeQL alerts to be closer to the recommended solution since the code is still being flagged: https://github.com/microsoft/chat-copilot/security/code-scanning/7 my guess is that the alerts are still firing because the tool sees the input variable (`memoryName`) used on the same line as `LogWarning()` with no explicit call to `.Replace()`: image - updates the CodeQL config to also run on JS/TS files as well, which finds 2 alerts in the tests: https://github.com/dehoward/chat-copilot/security/code-scanning/6 ### Description ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone :smile: --- .github/workflows/codeql-analysis.yml | 61 +++++++++++----------- webapi/Controllers/ChatMemoryController.cs | 34 +++++------- webapp/tests/utils.ts | 4 +- 3 files changed, 45 insertions(+), 54 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index a360ada33..7ebf0c625 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -6,9 +6,9 @@ name: "CodeQL" on: push: - branches: [ "main", "experimental*", "feature*" ] + branches: ["main", "experimental*", "feature*"] schedule: - - cron: '17 11 * * 2' + - cron: "17 11 * * 2" jobs: analyze: @@ -22,45 +22,44 @@ jobs: strategy: fail-fast: false matrix: - language: [ 'csharp' ] + language: ["csharp", "javascript"] # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ] # Use only 'java' to analyze code written in Java, Kotlin or both # Use only 'javascript' to analyze code written in JavaScript, TypeScript or both # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support steps: - - name: Checkout repository - uses: actions/checkout@v3 + - name: Checkout repository + uses: actions/checkout@v3 - # Initializes the CodeQL tools for scanning. - - name: Initialize CodeQL - uses: github/codeql-action/init@v2 - with: - languages: ${{ matrix.language }} - # If you wish to specify custom queries, you can do so here or in a config file. - # By default, queries listed here will override any specified in a config file. - # Prefix the list here with "+" to use these queries and those in the config file. + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v2 + with: + languages: ${{ matrix.language }} + # If you wish to specify custom queries, you can do so here or in a config file. + # By default, queries listed here will override any specified in a config file. + # Prefix the list here with "+" to use these queries and those in the config file. - # Details on CodeQL's query packs refer to : https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs - # queries: security-extended,security-and-quality + # Details on CodeQL's query packs refer to : https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs + # queries: security-extended,security-and-quality + # Autobuild attempts to build any compiled languages (C/C++, C#, Go, or Java). + # If this step fails, then you should remove it and run the build manually (see below) + - name: Autobuild + uses: github/codeql-action/autobuild@v2 - # Autobuild attempts to build any compiled languages (C/C++, C#, Go, or Java). - # If this step fails, then you should remove it and run the build manually (see below) - - name: Autobuild - uses: github/codeql-action/autobuild@v2 + # ℹī¸ Command-line programs to run using the OS shell. + # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun - # ℹī¸ Command-line programs to run using the OS shell. - # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun + # If the Autobuild fails above, remove it and uncomment the following three lines. + # modify them (or add more) to build your code if your project, please refer to the EXAMPLE below for guidance. - # If the Autobuild fails above, remove it and uncomment the following three lines. - # modify them (or add more) to build your code if your project, please refer to the EXAMPLE below for guidance. + # - run: | + # echo "Run, Build Application using script" + # ./location_of_script_within_repo/buildscript.sh - # - run: | - # echo "Run, Build Application using script" - # ./location_of_script_within_repo/buildscript.sh - - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v2 - with: - category: "/language:${{matrix.language}}" + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v2 + with: + category: "/language:${{matrix.language}}" diff --git a/webapi/Controllers/ChatMemoryController.cs b/webapi/Controllers/ChatMemoryController.cs index 19337509a..2fba99335 100644 --- a/webapi/Controllers/ChatMemoryController.cs +++ b/webapi/Controllers/ChatMemoryController.cs @@ -60,18 +60,23 @@ public async Task GetSemanticMemoriesAsync( [FromRoute] string chatId, [FromRoute] string memoryName) { + // Sanitize the log input by removing new line characters. + // https://github.com/microsoft/chat-copilot/security/code-scanning/1 + var sanitizedChatId = chatId.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + var sanitizedMemoryName = memoryName.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + // Make sure the chat session exists. if (!await this._chatSessionRepository.TryFindByIdAsync(chatId, v => _ = v)) { - this._logger.LogWarning("Chat session: {0} does not exist.", this.SanitizeLogInput(chatId)); - return this.BadRequest($"Chat session: {chatId} does not exist."); + this._logger.LogWarning("Chat session: {0} does not exist.", sanitizedChatId); + return this.BadRequest($"Chat session: {sanitizedChatId} does not exist."); } // Make sure the memory name is valid. - if (!this.ValidateMemoryName(memoryName)) + if (!this.ValidateMemoryName(sanitizedMemoryName)) { - this._logger.LogWarning("Memory name: {0} is invalid.", this.SanitizeLogInput(memoryName)); - return this.BadRequest($"Memory name: {memoryName} is invalid."); + this._logger.LogWarning("Memory name: {0} is invalid.", sanitizedMemoryName); + return this.BadRequest($"Memory name: {sanitizedMemoryName} is invalid."); } // Gather the requested semantic memory. @@ -79,7 +84,7 @@ public async Task GetSemanticMemoriesAsync( // Will use a dummy query since we don't care about relevance. An empty string will cause exception. // minRelevanceScore is set to 0.0 to return all memories. List memories = new(); - string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(chatId, memoryName); + string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(sanitizedChatId, sanitizedMemoryName); try { var results = semanticTextMemory.SearchAsync( @@ -95,7 +100,8 @@ public async Task GetSemanticMemoriesAsync( catch (SKException connectorException) { // A store exception might be thrown if the collection does not exist, depending on the memory store connector. - this._logger.LogError(connectorException, "Cannot search collection {0}", this.SanitizeLogInput(memoryCollectionName)); + var sanitizedMemoryCollectionName = memoryCollectionName.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); + this._logger.LogError(connectorException, "Cannot search collection {0}", sanitizedMemoryCollectionName); } return this.Ok(memories); @@ -113,19 +119,5 @@ private bool ValidateMemoryName(string memoryName) return this._promptOptions.MemoryMap.ContainsKey(memoryName); } - /// - /// Sanitizes the log input by removing new line characters. - /// This helps prevent log forgery attacks from malicious text. - /// - /// - /// https://github.com/microsoft/chat-copilot/security/code-scanning/1 - /// - /// The input to sanitize. - /// The sanitized input. - private string SanitizeLogInput(string input) - { - return input.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal); - } - # endregion } diff --git a/webapp/tests/utils.ts b/webapp/tests/utils.ts index 0e584e86e..19bcf928d 100644 --- a/webapp/tests/utils.ts +++ b/webapp/tests/utils.ts @@ -13,7 +13,7 @@ export async function loginHelper(page, useraccount, password) { // Expect the page to contain a "Login" button. await page.getByRole('button').click(); // Clicking the login button should redirect to the login page. - await expect(page).toHaveURL(new RegExp('^' + process.env.REACT_APP_AAD_AUTHORITY)); + await expect(page).toHaveURL(process.env.REACT_APP_AAD_AUTHORITY); // Login with the test user. await page.getByPlaceholder('Email, phone, or Skype').click(); await page.getByPlaceholder('Email, phone, or Skype').fill(useraccount as string); @@ -36,7 +36,7 @@ export async function loginHelperAnotherUser(page, useraccount, password) { // Expect the page to contain a "Login" button. await page.getByRole('button').click(); // Clicking the login button should redirect to the login page. - await expect(page).toHaveURL(new RegExp('^' + process.env.REACT_APP_AAD_AUTHORITY)); + await expect(page).toHaveURL(process.env.REACT_APP_AAD_AUTHORITY); // Login with the another user account. await page.getByRole('button', { name: 'Use another account' }).click(); await page.getByPlaceholder('Email, phone, or Skype').click();