Skip to content

[Spec Resync] 06-25-2025 #2407

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mongodb-drivers-pr-bot[bot]
Copy link

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot commented Jun 25, 2025

The following specs were changed:

  • bson_binary_vector
  • bson_corpus
  • collection_management
  • connection_logging
  • connection_monitoring
  • connection_string
  • crud
  • discovery_and_monitoring
  • gridfs
  • load_balancer
  • sdam_monitoring
  • server_selection_logging
  • sessions
  • unified-test-format

The following spec syncs encountered errors:

  • transactions-convenient-api
Already on 'master'
284 blocks
Already on 'master'
rm: cannot remove '/data/mci/df6a3325d0f03ada91aad0e3c2299597/src/test/transactions/legacy/errors-client.json': No such file or directory

  • transactions
Already on 'master'
284 blocks
Already on 'master'
rm: cannot remove '/data/mci/df6a3325d0f03ada91aad0e3c2299597/src/test/transactions/legacy/errors-client.json': No such file or directory

pr_body += "The following specs were changed:\n- "
process = subprocess.run(
["git diff --name-only | awk -F'/' '{print $2}' | sort | uniq"],
shell=True,

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.

To resolve this comment:

💡 Follow autofix suggestion

Suggested change
shell=True,
shell=False,
View step-by-step instructions
  1. Change the subprocess.run call to use shell=False (which is the default), and provide the command as a list rather than a string.
  2. Split the long shell command (git diff --name-only | awk -F'/' '{print $2}' | sort | uniq) into individual arguments so you can pass it as a list, or use Python modules like subprocess.PIPE and multiple subprocess calls to replicate the shell pipeline.
  3. For your example, you can achieve similar results in Python by using multiple subprocess runs:
    • Run git diff --name-only
    • Pass the output to awk via subprocess, or process it in Python
    • Sort and deduplicate results in Python
  4. Replace the vulnerable code:
    process = subprocess.run(
        ["git diff --name-only | awk -F'/' '{print $2}' | sort | uniq"],
        shell=True,
        capture_output=True,
        text=True)
    
    With safer code like:
    process1 = subprocess.run(["git", "diff", "--name-only"], capture_output=True, text=True)
    lines = [line.strip().split('/')[1] for line in process1.stdout.strip().splitlines() if '/' in line]
    unique_sorted = sorted(set(lines))
    process_stdout = '\n'.join(unique_sorted) + '\n' if unique_sorted else ''
    pr_body += process_stdout.replace("\n", "\n- ")
    

Alternatively, if you must use shell pipelines, make absolutely sure the arguments are constant and trusted, but avoid shell=True unless absolutely necessary. Using direct argument lists is safer and avoids shell injection risks.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by subprocess-shell-true.

You can view more details about this finding in the Semgrep AppSec Platform.

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.

0 participants