Skip to content
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

Code Scanning security alert fixes #129

Open
wants to merge 78 commits into
base: main
Choose a base branch
from

Conversation

arpitjain099
Copy link

Purpose

These alerts were identified and fixed using CodeQL extended query suite. Th

Does this introduce a breaking change?

[ ] Yes
[X] No

Pull Request Type

What kind of change does this Pull Request introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[X] Other... Please describe: Patch security based coding issues in the codebase.

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

arpitjain099 and others added 11 commits October 20, 2024 13:16
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 2: DOM text reinterpreted as HTML
Fix code scanning alert no. 9: Reflected server-side cross-site scripting
Fix code scanning alert no. 49: Information exposure through an exception
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@arpitjain099
Copy link
Author

Please review @kristapratico @luisquintanilla @colombod

arpitjain099 and others added 18 commits October 20, 2024 13:40
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 51: Information exposure through an exception
Fix code scanning alert no. 63: Information exposure through an exception
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
arpitjain099 and others added 25 commits October 20, 2024 14:11
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 66: Information exposure through an exception
Fix code scanning alert no. 45: Information exposure through an exception
Fix code scanning alert no. 39: Information exposure through an exception
Fix code scanning alert no. 38: Information exposure through an exception
Fix code scanning alert no. 42: Information exposure through an exception
Fix code scanning alert no. 41: Information exposure through an exception
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 24: Information exposure through an exception
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…tion

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 67: Information exposure through an exception
Fix code scanning alert no. 65: Information exposure through an exception
Fix code scanning alert no. 38: Information exposure through an exception
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 22: Full server-side request forgery
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fix code scanning alert no. 18: Full server-side request forgery
Fix code scanning alert no. 22: Full server-side request forgery
@arpitjain099
Copy link
Author

Hi @kristapratico @luisquintanilla @colombod
Please review this PR when you can. Thank you!!

@arpitjain099
Copy link
Author

Thank you for approving the PR @colombod
Can you please also merge it if possible.

@arpitjain099
Copy link
Author

Hi @kristapratico @luisquintanilla @colombod
This PR has been approved, can you please merge when possible. Thank you!!

Comment on lines +15 to +30
from urllib.parse import urlparse
from urllib.parse import urlparse, urljoin

def _validate_base_uri(self, base_uri: str):
parsed_uri = urlparse(base_uri)
if parsed_uri.scheme not in ["http", "https"]:
raise ValueError("Invalid URI scheme")
if not parsed_uri.netloc:
raise ValueError("Invalid URI netloc")

def _validate_base_uri(self, base_uri: str):
parsed_uri = urlparse(base_uri)
if parsed_uri.scheme not in ["http", "https"]:
raise ValueError("Invalid URI scheme")
if not parsed_uri.netloc:
raise ValueError("Invalid URI netloc")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have duplicate code here

Comment on lines +49 to +59
def _validate_base_uri(self, base_uri: str) -> str:
# Ensure the base_uri is a trusted URL
if not base_uri.startswith("https://trusted-domain.com"):
raise ValueError("Invalid base URI")
return base_uri

def _sanitize_path(self, path: str) -> str:
# Sanitize the path to prevent malicious input
if ".." in path or path.startswith("/"):
raise ValueError("Invalid path")
return path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these? Can't we use the functions we defined above?

@@ -273,11 +286,14 @@ def update_user_group(group_id: str):
user_group = entities_manager.add_users_to_user_group(group_id, new_users)
return Response(response=json.dumps(user_group.to_item()), status=200)
except (TypeError, NullValueError, MissingPropertyError, ValueError) as e:
return Response(response=str(e), status=400)
logging.error("An error occurred while updating user group: %s", e, exc_info=True)
return Response(response="An internal error has occurred.", status=400)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should say something about "bad request" for a 400

Comment on lines +14 to +17
allowed_urls = ["https://api.openai.com/v1/embeddings", "https://another-trusted-url.com"]
url = os.getenv("AOAI_ENDPOINT")
if not is_valid_url(url) or url not in allowed_urls:
raise ValueError("The provided URL is not allowed.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change will work. A user needs to be able to pass their specific endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants