Conversation
Replace shell=True with argument list in subprocess.run() to prevent potential command injection via database parameters. Use psql's -f flag for input and Python file handling for output redirection.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors test execution and command invocation: Python now runs Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/runner.py (1)
193-213:⚠️ Potential issue | 🔴 CriticalGuard the SQL error path against
UnboundLocalError.If
build_psql_command(),open(),subprocess.run(),filecmp.cmp(), orgenerate_diff()fails before line 17 completes,resultis never assigned. The except block at lines 33–34 will crash trying to accessresult.stderrandresult.returncode, and line 36 outside the try block will also fail when callinglogTestOutput(testName, result). This converts a single test error into a runner failure.Proposed fix
def runTest(testName): extension = testName[testName.rfind('.')::] # Extract the file extension from the test name if extension == '.sql': # Execute psql command and Compare expected/actual output files, handle any errors gracefully through try/except block + psql_command = [] try: # Build the psql command with appropriate switches/inputs/outputs, psql_command, actual_output_file, expected_output_file = build_psql_command(testName) # Execute the psql command, redirecting output to file via Python (not shell) with open(actual_output_file, 'w') as outf: result = subprocess.run(psql_command, stdout=outf, stderr=subprocess.STDOUT, text=True) # Compare actual and expected output files, setting shallow=False compares content and not just timestamps/size if filecmp.cmp(actual_output_file, expected_output_file, shallow=False): result_status = "pass" result.stdout = "Expected and Actual output files match" # Set success message result.returncode = 0 # Explicitly set the return code for success else: result_status = "fail" # Identify the diff between actual output and expected output diff_output = generate_diff(actual_output_file, expected_output_file) # Set stderr to include the diff output result.stderr = "Expected and Actual output files do not match. Diff as follows:\n" + diff_output result.returncode = 1 # Explicitly set the return code for failure except Exception as e: # Capture the error message result_status = "error" - result.stderr = f"An unexpected error occurred: {str(e)}" - result.returncode = 1 # Set return code to 1 for error + result = subprocess.CompletedProcess( + args=psql_command, + returncode=1, + stdout="", + stderr=f"An unexpected error occurred: {str(e)}", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.py` around lines 193 - 213, The try block can leave `result` unassigned if `build_psql_command`, `open`, `subprocess.run`, `filecmp.cmp`, or `generate_diff` raise, causing the except block and subsequent `logTestOutput(testName, result)` to crash; to fix, initialize a safe default `result` before the try (e.g., a CompletedProcess-like object or plain dict with stdout/stderr/returncode) so it always exists, then inside the except populate `result.stderr` and `result.returncode` with the error message and non-zero code, and ensure `logTestOutput(testName, result)` uses that guaranteed `result`; reference the symbols build_psql_command, generate_diff, result, and logTestOutput when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/runner.py`:
- Around line 193-213: The try block can leave `result` unassigned if
`build_psql_command`, `open`, `subprocess.run`, `filecmp.cmp`, or
`generate_diff` raise, causing the except block and subsequent
`logTestOutput(testName, result)` to crash; to fix, initialize a safe default
`result` before the try (e.g., a CompletedProcess-like object or plain dict with
stdout/stderr/returncode) so it always exists, then inside the except populate
`result.stderr` and `result.returncode` with the error message and non-zero
code, and ensure `logTestOutput(testName, result)` uses that guaranteed
`result`; reference the symbols build_psql_command, generate_diff, result, and
logTestOutput when making the change.
Add default value to PGVER ARG and use ${PGVER} brace syntax in FROM.
Recent GitHub Actions runner updates ship a newer BuildKit version that
performs static validation of ARG references in FROM instructions before
build arguments are applied. Without a default value, BuildKit rejects
the Dockerfile with InvalidDefaultArgInFrom even though --build-arg
PGVER is passed at build time. The default value (17) satisfies the
static check and is overridden by the matrix build-arg in CI.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
1-23: Static analysis: Container runs as root user.Trivy flags that no
USERdirective specifies a non-root user (DS-0002). While the officialpostgresbase image handles user switching viadocker-entrypoint.sh(switching to thepostgresuser before starting the database), explicitly setting the user can improve security posture.This is a pre-existing concern not introduced by this PR, so it can be addressed separately if desired.
💡 Optional: Add explicit USER directive
ENTRYPOINT ["docker-entrypoint.sh"] CMD ["postgres"] + +USER postgresNote: Adding
USER postgresmay require adjusting file permissions or moving it before certain commands. The entrypoint script already handles the user switch, so this is optional hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 1 - 23, The Dockerfile currently never sets a non-root USER which Trivy flags; add an explicit USER postgres (or another non-root user) after files that need root-only permissions (e.g., after RUN USE_PGXS=1 make install and after COPY ./ to /home/postgres/snowflake/) and before EXPOSE/ENTRYPOINT to harden the container; if permission issues occur, adjust ownership of the copied files and build artifacts with chown (e.g., change ownership of /home/postgres/snowflake to postgres) using existing RUN steps so ENTRYPOINT ["docker-entrypoint.sh"] can run as the non-root postgres user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 1-23: The Dockerfile currently never sets a non-root USER which
Trivy flags; add an explicit USER postgres (or another non-root user) after
files that need root-only permissions (e.g., after RUN USE_PGXS=1 make install
and after COPY ./ to /home/postgres/snowflake/) and before EXPOSE/ENTRYPOINT to
harden the container; if permission issues occur, adjust ownership of the copied
files and build artifacts with chown (e.g., change ownership of
/home/postgres/snowflake to postgres) using existing RUN steps so ENTRYPOINT
["docker-entrypoint.sh"] can run as the non-root postgres user.
PG18 Alpine image is built with --with-llvm using clang-19, but the Alpine clang package provides an unversioned clang binary. This causes PGXS to fail when trying to invoke clang-19 for bitcode generation. Since snowflake does not benefit from JIT, disable LLVM bitcode compilation with with_llvm=no and remove the now-unnecessary clang and llvm packages from the image.
Add 18config.env for the PG18 matrix entry in the CI workflow. Without this file, runner.py exits with an error when invoked with -c test/t/lib/18config.env.
Same fix as runner.py: replace shell=True with argument lists in run_cmd() and run_nc_cmd() to prevent potential command injection via the cmd parameter.
Pin all third-party actions in both workflow files to immutable commit SHAs instead of mutable tags, preventing potential supply chain attacks via tag reassignment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/t/lib/18config.env (1)
27-27: Consider using$(whoami)instead of backticks.Modern shell best practice prefers
$()over backticks for command substitution as it's more readable and nestable.Optional style improvement
-export EDGE_REPUSER=`whoami` +export EDGE_REPUSER=$(whoami)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/t/lib/18config.env` at line 27, Replace the backtick-style command substitution for setting EDGE_REPUSER with the modern $() form: locate the export statement that assigns EDGE_REPUSER (export EDGE_REPUSER=`whoami`) and change it to use $(whoami) so the variable assignment uses the recommended command-substitution syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/t/util_test.py`:
- Around line 86-89: The run_cmd helper uses naive cmd.split() which mis-parses
quoted or space-containing arguments; import shlex and replace cmd.split() with
shlex.split(cmd) in run_cmd (function name run_cmd) so arguments are tokenized
correctly (you can also use the list-unpacking style
[f"{node_path}/pgedge/pgedge", *shlex.split(cmd)] to satisfy the RUF005 hint).
- Around line 98-101: The helper run_nc_cmd currently uses cmd.split() which
mis-parses complex shell arguments; update the subprocess.run invocation inside
run_nc_cmd to use shlex.split(cmd) instead of cmd.split(), and add an import for
shlex at the module top if missing so arguments are parsed safely when calling
subprocess.run([f"{node_path}/pgedge"] + shlex.split(cmd), ...).
---
Nitpick comments:
In `@test/t/lib/18config.env`:
- Line 27: Replace the backtick-style command substitution for setting
EDGE_REPUSER with the modern $() form: locate the export statement that assigns
EDGE_REPUSER (export EDGE_REPUSER=`whoami`) and change it to use $(whoami) so
the variable assignment uses the recommended command-substitution syntax.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0f113a6-c17c-4ab1-b4e5-260d6cf05986
📒 Files selected for processing (2)
test/t/lib/18config.envtest/t/util_test.py
Add missing query parameter to function signature and remove redundant f-string wrapper that the static analyzer flagged as potential SQL injection.
danolivo
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
test/runner.pyandtest/t/util_test.py: replace allshell=Truesubprocess calls with argument lists, use Python file I/O instead of shell redirectionARG PGVERfor newer BuildKit compatibility, skip LLVM bitcode compilation (with_llvm=no) to avoid missingclang-19on Alpine, remove unusedclang/llvmpackagestest/t/lib/18config.envfor the test matrixexecute_sqlite_query: add missingqueryparameter and remove redundant f-string that masked the bugChecklist
18config.envadded for PG18 matrixNotes for Reviewers
with_llvm=noin the Dockerfile means extensions won't produce LLVM bitcode. This is intentional — Alpine postgres images don't consistently provide the matching clang version (e.g. PG18 expectsclang-19but Alpine only has an unversionedclang).runner.py,build_psql_command()now returns a list and uses-fflag instead of shell<redirection. Output capture uses Pythonopen()instead of shell>.execute_sqlite_query()previously had noqueryparameter — it referenced an undefined variable. The f-stringf"{query}"also masked this by deferring the NameError to call time.