Skip to content

Commit

Permalink
Fix C# CodeQL alerts + add JS/TS to CodeQL (microsoft#139)
Browse files Browse the repository at this point in the history
### Motivation and Context

<!-- Thank you for your contribution to the copilot-chat repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->


- 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()`:
<img width="613" alt="image"
src="https://github.com/microsoft/chat-copilot/assets/52973358/b9966d06-5516-40c1-83d0-531073ef7fb3">
- 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

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [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 😄
  • Loading branch information
dehoward authored Aug 10, 2023
1 parent 2632855 commit 850f571
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 54 deletions.
61 changes: 30 additions & 31 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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}}"
34 changes: 13 additions & 21 deletions webapi/Controllers/ChatMemoryController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,26 +60,31 @@ public async Task<IActionResult> 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.
// ISemanticTextMemory doesn't support retrieving all memories.
// 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<string> memories = new();
string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(chatId, memoryName);
string memoryCollectionName = SemanticChatMemoryExtractor.MemoryCollectionName(sanitizedChatId, sanitizedMemoryName);
try
{
var results = semanticTextMemory.SearchAsync(
Expand All @@ -95,7 +100,8 @@ public async Task<IActionResult> 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);
Expand All @@ -113,19 +119,5 @@ private bool ValidateMemoryName(string memoryName)
return this._promptOptions.MemoryMap.ContainsKey(memoryName);
}

/// <summary>
/// Sanitizes the log input by removing new line characters.
/// This helps prevent log forgery attacks from malicious text.
/// </summary>
/// <remarks>
/// https://github.com/microsoft/chat-copilot/security/code-scanning/1
/// </remarks>
/// <param name="input">The input to sanitize.</param>
/// <returns>The sanitized input.</returns>
private string SanitizeLogInput(string input)
{
return input.Replace(Environment.NewLine, string.Empty, StringComparison.Ordinal);
}

# endregion
}
4 changes: 2 additions & 2 deletions webapp/tests/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand Down

0 comments on commit 850f571

Please sign in to comment.