PostgreSQL replacement with SQLite - #435
Conversation
Commit git ignore for SQLite test deployment
Replacement of PostgreSQL as default database used for the MDV platform. Replacing with SQLite to better handle users/credentials. But keeping PostgrSQL as an option when decided by the user to use it. The deployment scripts build the environment according to what the user desire.
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
lferreira86
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds interactive local Docker deployment scripts and docs, per-deployment env/override ignore rules, DB backend handling validated by tests (sqlite vs postgres), and a new CLI to export user–project associations with optional Auth0 login enrichment and CSV output. ChangesLocal deployment documentation and tooling
Database backend flexibility
User–project reporting CLI tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy_gui.sh (1)
149-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
source "$env_file"in the GUI deploy path too.That executes
.env.<deployment>as shell code. It also means saved values containing shell syntax can be mutated or executed on the next run.Suggested fix
- # Source the existing .env file to retain values - source "$env_file" + # Read only the keys this script needs; do not execute the file. + while IFS='=' read -r key value; do + case "$key" in + DB_BACKEND|DB_HOST|DB_SCHEMA|DB_USER|DB_NAME) + printf -v "$key" '%s' "$value" + ;; + esac + done < "$env_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy_gui.sh` around lines 149 - 163, The create_or_validate_env_file function currently uses source "$env_file", which executes .env.<deployment> as shell code; replace this with a safe parser that reads the file contents without executing them and extracts the DB_* keys, then assign PREV_DB_BACKEND, PREV_DB_HOST, PREV_DB_SCHEMA, PREV_DB_USER, and PREV_DB_NAME from the parsed values. Implement a non-executing reader (e.g., read the file line-by-line and parse lines matching ^DB_[A-Z_]+=, stripping quotes and not evaluating expressions) and use those parsed strings instead of sourcing in create_or_validate_env_file so saved values cannot be executed or mutated on load.deploy.sh (1)
194-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't
source.env.<deployment>here.This turns the generated env file into executable shell code. A hand-edited value like
$(...)or backticks will run on the next deploy, and ordinary secrets containing shell metacharacters can be misread as well.Suggested fix
- # Source existing .env file to retain values - [ -f "$env_file" ] && source "$env_file" + # Read only the keys this script needs; do not execute the file. + if [ -f "$env_file" ]; then + while IFS='=' read -r key value; do + case "$key" in + DB_BACKEND|DB_HOST|DB_SCHEMA|DB_USER|DB_NAME) + printf -v "$key" '%s' "$value" + ;; + esac + done < "$env_file" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy.sh` around lines 194 - 207, The script currently unsafely executes the env file via "[ -f "$env_file" ] && source "$env_file""; remove that line and instead safely parse only the required DB_* keys into PREV_DB_BACKEND, PREV_DB_HOST, PREV_DB_SCHEMA, PREV_DB_USER, PREV_DB_NAME by reading env_file as data (e.g., use grep -E '^DB_BACKEND=' env_file | cut -d= -f2- or a while IFS='=' read -r key val; case "$key" in DB_BACKEND|DB_HOST|DB_SCHEMA|DB_USER|DB_NAME) assign sanitized values ;; esac done) so no shell evaluation occurs and only the specific variables populate the PREV_DB_* variables referenced later.
🧹 Nitpick comments (5)
docker-compose.yml (2)
6-7: 💤 Low valuePort mapping differs from documentation default.
The compose file maps host port
5056to container port5055, but the README documentation states the auto-suggested port starts at5055. This could cause confusion for users following the documentation.Consider aligning with documentation by using
5055:5055or updating the README to clarify that5056is the default when usingdocker-compose.ymldirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 6 - 7, The docker-compose ports mapping currently uses "5056:5055", which diverges from the README's documented default of port 5055; update the ports entry in docker-compose.yml to "5055:5055" to align with documentation (or alternatively update the README to explicitly note that docker-compose uses 5056 on the host), ensuring the change is made at the "ports" mapping line so behavior and docs match.
40-46: 💤 Low valueConsider adding a health check for PostgreSQL.
The
mdv_dbservice lacks a health check, which meansdepends_ononly ensures the container starts, not that PostgreSQL is ready to accept connections. While the application handles this viawait_for_database(), adding a health check would improve container orchestration reliability.💡 Suggested health check
mdv_db: image: postgres:16 volumes: - postgres-data:/var/lib/postgresql/data env_file: - .env # Load database credentials from the .env file restart: always + healthcheck: + test: ["CMD-SHELL", "pg_isready -U $${POSTGRES_USER:-postgres}"] + interval: 10s + timeout: 5s + retries: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 40 - 46, Add a Docker healthcheck to the mdv_db service so Compose can detect when Postgres is ready (rather than only started); update the mdv_db service definition to include a healthcheck that runs a Postgres readiness probe (e.g., using pg_isready or psql) with sensible parameters (interval, timeout, retries, and start_period) so dependents wait for a healthy state before starting; keep the existing application-level wait_for_database() but add this healthcheck to improve orchestration reliability.postgres (1)
1-1: 💤 Low valueAmbiguous filename and missing context.
This file named
postgreswith no extension appears to contain a user prompt string for deployment scripts. Consider:
- Renaming to something clearer like
prompts/sqlite-volume-prompt.txtor embedding this directly in the deployment scripts- Adding a trailing newline for POSIX compliance
The current name
postgresis misleading since the content relates to SQLite switching behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postgres` at line 1, The file named "postgres" contains a prompt string "Switching sqlite- Remove old app volume \"cartography_mdv-data\"? (y/n) [n]:" which is misleading and lacks a trailing newline; rename or relocate this file to a clearer path (e.g., prompts/sqlite-volume-prompt.txt) or embed the prompt directly into the deployment script that invokes it (reference the exact prompt string above when searching), and ensure the file ends with a POSIX-compliant trailing newline; also update any references to the old filename to the new location to avoid confusion.python/mdvtools/scripts/users_projects_dataframe.py (2)
95-122: 💤 Low valueAuth0 stats fetched outside app context.
_fetch_auth0_login_stats()accessesapp.config(lines 37-40) but is called on line 122 after thewith app.app_context():block has started the query but before context exit. The function itself doesn't use a context wrapper, which means it relies on the caller's context.Currently this works because the call is inside the
withblock, but the function's implementation accessesapp.configwithout ensuring a context exists. Consider either:
- Documenting this dependency
- Moving the call outside the context block since
app.configdoesn't require an active context- auth0_login_stats = _fetch_auth0_login_stats() if include_auth0_login_stats else {} + # Fetch Auth0 stats outside app context (config access doesn't require context) + auth0_login_stats = _fetch_auth0_login_stats() if include_auth0_login_stats else {} + + with app.app_context(): + # Query database...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mdvtools/scripts/users_projects_dataframe.py` around lines 95 - 122, The call to _fetch_auth0_login_stats relies on app config but the function itself doesn't ensure a Flask app context; either make the function context-safe or call it outside the with app.app_context(): specifically, update _fetch_auth0_login_stats to obtain config via flask.current_app (or accept an app parameter and use with app.app_context() internally) so it no longer depends on the caller's context, or alternatively move the _fetch_auth0_login_stats() invocation out of the with app.app_context(): block; refer to the _fetch_auth0_login_stats symbol and the existing with app.app_context(): usage to locate where to change.
134-135: 💤 Low valuePyright false positive on DataFrame columns parameter.
The static analysis error on
pd.DataFrame(columns=columns)is a known false positive with pandas type stubs. The code is correct at runtime. You can silence this with a type ignore comment if desired:return pd.DataFrame(columns=columns) # type: ignore[arg-type]The same applies to line 164.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mdvtools/scripts/users_projects_dataframe.py` around lines 134 - 135, The Pyright false positive on the pd.DataFrame(columns=columns) call should be silenced by adding a type ignore comment to the return statements; update the two occurrences where you return an empty DataFrame (the branch starting with "if not rows" that calls pd.DataFrame(columns=columns) and the similar return around line 164) to include "# type: ignore[arg-type]" after the pd.DataFrame(...) call so the static checker ignores the known pandas stub mismatch for the columns parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy_gui.sh`:
- Around line 82-103: The DEPLOYMENT_NAME from the Zenity prompts (initial entry
and the rename branch) is not validated for unsafe characters and can inject
path separators or quotes into generated filenames (e.g.,
.env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml) and
DB_SCHEMA; add the same safe-name validation used elsewhere immediately after
the initial DEPLOYMENT_NAME assignment and again after the rename prompt:
validate against a strict pattern (e.g., allow only [A-Za-z0-9._-] and forbid
slashes/quotes), and if the name fails validation show a zenity error and exit
(or reprompt) so only safe filenames are produced; update the logic that handles
DEPLOYMENT_NAME, the rename flow where DEPLOYMENT_NAME is reassigned, and any
subsequent checks that assume a valid name.
- Around line 59-67: The regex in get_existing_deployment_port() uses an
unescaped "->" which breaks the [[ ... =~ ... ]] expression; update the match
pattern to escape the greater-than so the shell parses it correctly, e.g. change
the conditional to: [[ "$ports_line" =~ (0\.0\.0\.0|\[::\]):([0-9]+)-\>5055 ]],
keeping the RHS unquoted so BASH_REMATCH[2] still captures the port; leave the
echo of BASH_REMATCH[2] unchanged.
In `@deploy.bat`:
- Around line 25-53: Validate and sanitize DEPLOYMENT_NAME every time it is set
(both initial prompt and inside the "rename" branch) before interpolating into
commands: add a validation step after the initial set /p DEPLOYMENT_NAME and
again after the rename prompt that rejects or re-prompts on any disallowed
characters (e.g. &|%!"^<>/\\ and whitespace) or enforces an allowlist pattern
(e.g. [A-Za-z0-9_.-]); if invalid, print an error and exit/reprompt. Apply this
same validation before using DEPLOYMENT_NAME in the docker filter loop, building
ENV_FILE, and any powershell/cmd redirections so you never interpolate an
unchecked value; reference the variables DEPLOYMENT_NAME, DEPLOY_ACTION,
HAS_EXISTING_DEPLOYMENT, and ENV_FILE when adding these checks.
- Line 2: The script enables delayed expansion globally with the setlocal
EnableDelayedExpansion directive which will corrupt secrets containing "!" when
reading or parsing values; modify the flow to DisableDelayedExpansion before any
secret reads or file parsing (references: set /p prompts used to capture secrets
and the file parsing logic mentioned around the original file parsing block),
then re-enable only where necessary (wrap only the blocks that require delayed
expansion around setlocal EnableDelayedExpansion) and ensure variables that are
later expanded as !VAR! (noted in the review around the expansions at lines
referenced) are handled with delayed expansion enabled only when those
expansions are safe; in short, switch to setlocal DisableDelayedExpansion before
secret capture/parsing and use setlocal EnableDelayedExpansion only for the
minimal blocks that need it, restoring previous state as needed.
In `@deploy.sh`:
- Around line 130-145: DEPLOYMENT_NAME is reused in filenames and DB schema and
isn't validated after user input or after the rename flow, allowing unsafe
values; add a whitelist validation (e.g., allow only [A-Za-z0-9._-] and forbid
sequences like ".." and characters like "/" and "\" and quotes) applied
immediately after setting DEPLOYMENT_NAME from deployment_name_input and again
after reading new_deployment_name in the rename branch (referencing
DEPLOYMENT_NAME and new_deployment_name), and if the value fails the regex or is
empty, print an error and exit 1 so only safe names are used for
.env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml and
DB_SCHEMA.
- Around line 107-115: The regex in get_existing_deployment_port uses an
unescaped '>' causing a shell parse error; fix it by assigning the pattern to a
variable (e.g., port_pattern) and use that in the [[ ... =~ ... ]] test,
escaping the arrow as \-> (e.g., "(0\.0\.0\.0|\[::\]):([0-9]+)\->5055") so the
shell parses correctly and BASH_REMATCH[2] still contains the port.
In `@python/mdvtools/dbutils/mdv_server_app.py`:
- Around line 223-233: The code builds SQL by interpolating db_schema directly
into text(...) which risks SQL injection; change the CREATE SCHEMA call to
safely quote or validate the identifier before executing: obtain a safe quoted
identifier via schema_engine.dialect.identifier_preparer.quote(db_schema) (or
use sqlalchemy.schema.CreateSchema) and use that quoted value in the text() call
instead of raw f-string interpolation, and/or add a simple validation (e.g.,
allow only [A-Za-z0-9_\\-] or similar) on db_schema before attempting to create
the schema; update the block around create_engine, schema_engine, and
schema_connection to use the quoted identifier (or CreateSchema) when calling
schema_connection.execute(...) so the schema name cannot inject SQL.
In `@python/mdvtools/scripts/users_projects_dataframe.py`:
- Around line 42-55: The variables auth0_domain, client_id, client_secret, and
auth0_db_connection are checked with if not all(...) but Pyright still treats
them as Optional; add explicit runtime type narrowing before use (e.g., assert
statements or explicit casts) to guarantee they are str for GetToken and Auth0
calls: after the existing if-block add assertions like assert auth0_domain is
not None and assert client_id is not None etc., or cast each to str when passing
into GetToken(... ) and Auth0(...), ensuring symbols token_client,
mgmt_api_token, GetToken, and Auth0 receive properly narrowed string types.
---
Outside diff comments:
In `@deploy_gui.sh`:
- Around line 149-163: The create_or_validate_env_file function currently uses
source "$env_file", which executes .env.<deployment> as shell code; replace this
with a safe parser that reads the file contents without executing them and
extracts the DB_* keys, then assign PREV_DB_BACKEND, PREV_DB_HOST,
PREV_DB_SCHEMA, PREV_DB_USER, and PREV_DB_NAME from the parsed values. Implement
a non-executing reader (e.g., read the file line-by-line and parse lines
matching ^DB_[A-Z_]+=, stripping quotes and not evaluating expressions) and use
those parsed strings instead of sourcing in create_or_validate_env_file so saved
values cannot be executed or mutated on load.
In `@deploy.sh`:
- Around line 194-207: The script currently unsafely executes the env file via
"[ -f "$env_file" ] && source "$env_file""; remove that line and instead safely
parse only the required DB_* keys into PREV_DB_BACKEND, PREV_DB_HOST,
PREV_DB_SCHEMA, PREV_DB_USER, PREV_DB_NAME by reading env_file as data (e.g.,
use grep -E '^DB_BACKEND=' env_file | cut -d= -f2- or a while IFS='=' read -r
key val; case "$key" in DB_BACKEND|DB_HOST|DB_SCHEMA|DB_USER|DB_NAME) assign
sanitized values ;; esac done) so no shell evaluation occurs and only the
specific variables populate the PREV_DB_* variables referenced later.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 6-7: The docker-compose ports mapping currently uses "5056:5055",
which diverges from the README's documented default of port 5055; update the
ports entry in docker-compose.yml to "5055:5055" to align with documentation (or
alternatively update the README to explicitly note that docker-compose uses 5056
on the host), ensuring the change is made at the "ports" mapping line so
behavior and docs match.
- Around line 40-46: Add a Docker healthcheck to the mdv_db service so Compose
can detect when Postgres is ready (rather than only started); update the mdv_db
service definition to include a healthcheck that runs a Postgres readiness probe
(e.g., using pg_isready or psql) with sensible parameters (interval, timeout,
retries, and start_period) so dependents wait for a healthy state before
starting; keep the existing application-level wait_for_database() but add this
healthcheck to improve orchestration reliability.
In `@postgres`:
- Line 1: The file named "postgres" contains a prompt string "Switching sqlite-
Remove old app volume \"cartography_mdv-data\"? (y/n) [n]:" which is misleading
and lacks a trailing newline; rename or relocate this file to a clearer path
(e.g., prompts/sqlite-volume-prompt.txt) or embed the prompt directly into the
deployment script that invokes it (reference the exact prompt string above when
searching), and ensure the file ends with a POSIX-compliant trailing newline;
also update any references to the old filename to the new location to avoid
confusion.
In `@python/mdvtools/scripts/users_projects_dataframe.py`:
- Around line 95-122: The call to _fetch_auth0_login_stats relies on app config
but the function itself doesn't ensure a Flask app context; either make the
function context-safe or call it outside the with app.app_context():
specifically, update _fetch_auth0_login_stats to obtain config via
flask.current_app (or accept an app parameter and use with app.app_context()
internally) so it no longer depends on the caller's context, or alternatively
move the _fetch_auth0_login_stats() invocation out of the with
app.app_context(): block; refer to the _fetch_auth0_login_stats symbol and the
existing with app.app_context(): usage to locate where to change.
- Around line 134-135: The Pyright false positive on the
pd.DataFrame(columns=columns) call should be silenced by adding a type ignore
comment to the return statements; update the two occurrences where you return an
empty DataFrame (the branch starting with "if not rows" that calls
pd.DataFrame(columns=columns) and the similar return around line 164) to include
"# type: ignore[arg-type]" after the pd.DataFrame(...) call so the static
checker ignores the known pandas stub mismatch for the columns parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbdbc141-67fe-4a79-aaaf-152cd71dd38e
📒 Files selected for processing (11)
.gitignoreREADME.mddeploy.batdeploy.shdeploy_gui.shdocker-compose.ymldocker-sqlite.override.ymlpostgrespython/mdvtools/dbutils/mdv_server_app.pypython/mdvtools/dbutils/test/unit/test_mdv_server_app.pypython/mdvtools/scripts/users_projects_dataframe.py
| DEPLOYMENT_NAME=$(zenity --entry --title="Deployment Name" --text="Deployment name:" --entry-text="mdv") | ||
| if [ -z "$DEPLOYMENT_NAME" ]; then | ||
| DEPLOYMENT_NAME="mdv" | ||
| fi | ||
|
|
||
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | ||
| DEPLOY_ACTION=$(zenity --list --radiolist \ | ||
| --title="Existing Deployment Found" \ | ||
| --text="Deployment '${DEPLOYMENT_NAME}' already exists. What would you like to do?" \ | ||
| --column="Select" --column="Action" \ | ||
| TRUE "redeploy" FALSE "rename" FALSE "exit") | ||
| if [ -z "$DEPLOY_ACTION" ] || [ "$DEPLOY_ACTION" = "exit" ]; then | ||
| exit 0 | ||
| elif [ "$DEPLOY_ACTION" = "rename" ]; then | ||
| DEPLOYMENT_NAME=$(zenity --entry --title="New Deployment Name" --text="Enter a new deployment name:" --entry-text="${DEPLOYMENT_NAME}_new") | ||
| if [ -z "$DEPLOYMENT_NAME" ]; then | ||
| zenity --error --text="Deployment name cannot be empty." | ||
| exit 1 | ||
| fi | ||
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | ||
| zenity --error --text="Deployment '${DEPLOYMENT_NAME}' already exists. Choose a different name." | ||
| exit 1 |
There was a problem hiding this comment.
Apply the same safe-name validation in the Zenity flow.
DEPLOYMENT_NAME is later reused in .env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml, and DB_SCHEMA. Allowing path separators or quotes here can break file generation and later schema cleanup.
Suggested fix
DEPLOYMENT_NAME=$(zenity --entry --title="Deployment Name" --text="Deployment name:" --entry-text="mdv")
if [ -z "$DEPLOYMENT_NAME" ]; then
DEPLOYMENT_NAME="mdv"
fi
+ if [[ ! "$DEPLOYMENT_NAME" =~ ^[A-Za-z0-9._-]+$ ]]; then
+ zenity --error --text="Deployment name may only contain letters, numbers, dot, underscore, and dash."
+ exit 1
+ fiApply the same validation after the rename prompt too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEPLOYMENT_NAME=$(zenity --entry --title="Deployment Name" --text="Deployment name:" --entry-text="mdv") | |
| if [ -z "$DEPLOYMENT_NAME" ]; then | |
| DEPLOYMENT_NAME="mdv" | |
| fi | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| DEPLOY_ACTION=$(zenity --list --radiolist \ | |
| --title="Existing Deployment Found" \ | |
| --text="Deployment '${DEPLOYMENT_NAME}' already exists. What would you like to do?" \ | |
| --column="Select" --column="Action" \ | |
| TRUE "redeploy" FALSE "rename" FALSE "exit") | |
| if [ -z "$DEPLOY_ACTION" ] || [ "$DEPLOY_ACTION" = "exit" ]; then | |
| exit 0 | |
| elif [ "$DEPLOY_ACTION" = "rename" ]; then | |
| DEPLOYMENT_NAME=$(zenity --entry --title="New Deployment Name" --text="Enter a new deployment name:" --entry-text="${DEPLOYMENT_NAME}_new") | |
| if [ -z "$DEPLOYMENT_NAME" ]; then | |
| zenity --error --text="Deployment name cannot be empty." | |
| exit 1 | |
| fi | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| zenity --error --text="Deployment '${DEPLOYMENT_NAME}' already exists. Choose a different name." | |
| exit 1 | |
| DEPLOYMENT_NAME=$(zenity --entry --title="Deployment Name" --text="Deployment name:" --entry-text="mdv") | |
| if [ -z "$DEPLOYMENT_NAME" ]; then | |
| DEPLOYMENT_NAME="mdv" | |
| fi | |
| if [[ ! "$DEPLOYMENT_NAME" =~ ^[A-Za-z0-9._-]+$ ]]; then | |
| zenity --error --text="Deployment name may only contain letters, numbers, dot, underscore, and dash." | |
| exit 1 | |
| fi | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| DEPLOY_ACTION=$(zenity --list --radiolist \ | |
| --title="Existing Deployment Found" \ | |
| --text="Deployment '${DEPLOYMENT_NAME}' already exists. What would you like to do?" \ | |
| --column="Select" --column="Action" \ | |
| TRUE "redeploy" FALSE "rename" FALSE "exit") | |
| if [ -z "$DEPLOY_ACTION" ] || [ "$DEPLOY_ACTION" = "exit" ]; then | |
| exit 0 | |
| elif [ "$DEPLOY_ACTION" = "rename" ]; then | |
| DEPLOYMENT_NAME=$(zenity --entry --title="New Deployment Name" --text="Enter a new deployment name:" --entry-text="${DEPLOYMENT_NAME}_new") | |
| if [ -z "$DEPLOYMENT_NAME" ]; then | |
| zenity --error --text="Deployment name cannot be empty." | |
| exit 1 | |
| fi | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| zenity --error --text="Deployment '${DEPLOYMENT_NAME}' already exists. Choose a different name." | |
| exit 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy_gui.sh` around lines 82 - 103, The DEPLOYMENT_NAME from the Zenity
prompts (initial entry and the rename branch) is not validated for unsafe
characters and can inject path separators or quotes into generated filenames
(e.g., .env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml)
and DB_SCHEMA; add the same safe-name validation used elsewhere immediately
after the initial DEPLOYMENT_NAME assignment and again after the rename prompt:
validate against a strict pattern (e.g., allow only [A-Za-z0-9._-] and forbid
slashes/quotes), and if the name fails validation show a zenity error and exit
(or reprompt) so only safe filenames are produced; update the logic that handles
DEPLOYMENT_NAME, the rename flow where DEPLOYMENT_NAME is reassigned, and any
subsequent checks that assume a valid name.
| @@ -1,4 +1,5 @@ | |||
| @echo off | |||
| setlocal EnableDelayedExpansion | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the deploy.bat file
find . -name "deploy.bat" -type fRepository: Taylor-CCB-Group/MDV
Length of output: 77
🏁 Script executed:
# Read the deploy.bat file to examine the code structure
cat -n deploy.bat | head -50Repository: Taylor-CCB-Group/MDV
Length of output: 2230
🏁 Script executed:
# Check the specific lines mentioned in the review
cat -n deploy.bat | sed -n '1,10p; 143,155p; 321,340p; 347,387p'Repository: Taylor-CCB-Group/MDV
Length of output: 4536
🌐 Web query:
cmd.exe EnableDelayedExpansion exclamation mark ! character in variable value corruption
💡 Result:
In Windows batch, ! “corruption” is caused by delayed variable expansion: when delayed expansion is enabled, cmd parses exclamation marks as delimiters for !var! expansions, so any literal exclamation marks in data (especially inside FOR / FOR /F loops and parenthesized blocks) get consumed or transformed. What’s happening in your case - Your script enables delayed expansion (SETLOCAL ... ENABLEDELAYEDEXPANSION). - You then store line text containing ! into environment variables and later read it back using delayed expansion mechanics (directly or indirectly). With delayed expansion on, the cmd parser treats parts between !…! as variable references; if there’s a single ! (or there isn’t a matching variable), it effectively disappears/changes when expansions happen. - This is a known pitfall of delayed expansion interacting with FOR /F and variable contents containing !. Fix patterns (pick one) 1) Temporarily disable delayed expansion while capturing/handling the line - The common workaround is to toggle delayed expansion off for the specific block where you expand the FOR/F variable and store/use it, then turn it back on. Example pattern (based on the accepted solution for this exact symptom): setlocal EnableExtensions EnableDelayedExpansion for /f "tokens=*" %%a in (bang1.txt) do ( setlocal DisableDelayedExpansion set "line=%%a" setlocal EnableDelayedExpansion call :doit1 line endlocal endlocal ) 2) Toggle delayed expansion only around the variable assignment/read - Disable delayed expansion before you SET a variable that may contain !, and re-enable it only when you later need !var! behavior. - This is repeatedly recommended: literal ! must be preserved by having delayed expansion disabled during the “set/parse” of that literal. 3) Alternative escaping: escape literal ! when delayed expansion is required - You can escape ! as ^! in the command context where delayed expansion is enabled. - But this can be tricky depending on quoting and which parsing phase happens next; disabling delayed expansion is usually more reliable than manual caret-escaping. Bottom line - Yes: this is expected behavior from delayed expansion. - The reliable solution is to disable delayed expansion while you move/print/store data that can contain literal exclamation marks (or escape those literals as ^! when you must keep delayed expansion on).
Citations:
- 1: https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc978727(v=technet.10)
- 2: https://stackoverflow.com/questions/35562664/the-curious-case-of-the-missing-exclamation-mark-in-cmd-files
- 3: https://ss64.com/nt/delayedexpansion.html
- 4: https://stackoverflow.com/questions/3288552/how-can-i-escape-an-exclamation-mark-in-cmd-scripts
- 5: https://superuser.com/questions/1769368/delayed-expansion-in-a-for-loop-processing-paths-that-contain-exclamation-marks
🌐 Web query:
batch script delayed expansion ! special character handling data loss
💡 Result:
In Windows batch scripts, enabling delayed expansion with setlocal EnableDelayedExpansion causes the exclamation mark ! to become a special delimiter for dynamic variable expansion (e.g., !var!). This leads to data loss when processing strings containing literal ! characters, such as filenames, paths, or text lines. When delayed expansion is enabled: - During variable assignment (e.g., set "var=%%f" in a FOR loop), ! in the source string (like a FOR parameter %%f) is interpreted as the start/end of a variable reference. - Unmatched ! are removed entirely. - Paired !var! are replaced by the value of var (empty if undefined), corrupting the string. This occurs because command lines are parsed in phases: percent expansion first, then delayed expansion after FOR parameters are expanded. FOR variables like %%f expand before delayed expansion, exposing embedded ! to interpretation. Examples of data loss: - Filename "file!name.txt" becomes "filename.txt" when assigned to a variable. - Paths with ! truncate after the first !. Official behavior confirmed in Microsoft docs: delayed expansion uses ! as variable delimiters, enabled via setlocal enabledelayedexpansion. Solutions to prevent data loss: 1. Temporarily disable delayed expansion when assigning variables holding potentially unsafe strings: for %%f in (*) do ( setlocal DisableDelayedExpansion set "safe=%%f" setlocal EnableDelayedExpansion echo(!safe! endlocal ) This preserves ! during assignment, as delayed expansion is off. 2. Use CALL for delayed expansion without enabling the feature globally: call set "result=%%varname%%" 3. Toggle per-line with nested SETLOCAL/ENDLOCAL or FOR tricks to pass values over ENDLOCAL. 4. Avoid delayed expansion in loops processing files: Use pure percent expansion or restructure code. Prioritize disabling delayed expansion during string capture from FOR loops or file reads. Re-enable only for safe variable reads. This is the standard fix in batch scripting communities (SS64, Stack Overflow). No changes in Windows up to 2026; behavior is by design for delayed expansion.
Citations:
- 1: https://stackoverflow.com/questions/36336518/issue-with-special-characters-in-path-exclamation-point-carrot-etc-using
- 2: https://stackoverflow.com/questions/37333620/special-characters-in-batch-file
- 3: https://www.reddit.com/r/Batch/comments/pak81f/how_do_you_handle_file_names_with_when_delayed/
- 4: https://superuser.com/questions/973601/delayedexpansion-variable-replacement-issues
- 5: https://stackoverflow.com/questions/56284870/how-to-not-lose-special-characters-when-processing-a-text-file-in-a-bat
- 6: https://superuser.com/questions/1769368/delayed-expansion-in-a-for-loop-processing-paths-that-contain-exclamation-marks
- 7: https://ss64.com/nt/delayedexpansion.html
- 8: https://stackoverflow.com/questions/35562664/the-curious-case-of-the-missing-exclamation-mark-in-cmd-files
- 9: https://stackoverflow.com/questions/32923139/exclamation-removed-from-cmd-cd-output
Global delayed expansion will corrupt secrets containing ! characters.
In cmd.exe, EnableDelayedExpansion makes ! a special delimiter for variable expansion. When secrets are captured via set /p (lines 321, 328) or file parsing (line 148) and later expanded as !VAR! (lines 358, 370, 375), any literal ! in the value will be removed or misinterpreted, silently corrupting database passwords, Flask secrets, and API keys. This is documented behavior when delayed expansion parses variable contents.
Disable delayed expansion while reading and writing secret-bearing values, then re-enable only for blocks that need it:
setlocal DisableDelayedExpansion
set /p FLASK_SECRET_KEY=Enter FLASK_SECRET_KEY [...]:
setlocal EnableDelayedExpansionAlso applies to: 143-155, 321-340, 347-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy.bat` at line 2, The script enables delayed expansion globally with the
setlocal EnableDelayedExpansion directive which will corrupt secrets containing
"!" when reading or parsing values; modify the flow to DisableDelayedExpansion
before any secret reads or file parsing (references: set /p prompts used to
capture secrets and the file parsing logic mentioned around the original file
parsing block), then re-enable only where necessary (wrap only the blocks that
require delayed expansion around setlocal EnableDelayedExpansion) and ensure
variables that are later expanded as !VAR! (noted in the review around the
expansions at lines referenced) are handled with delayed expansion enabled only
when those expansions are safe; in short, switch to setlocal
DisableDelayedExpansion before secret capture/parsing and use setlocal
EnableDelayedExpansion only for the minimal blocks that need it, restoring
previous state as needed.
| set /p DEPLOYMENT_NAME=Deployment name [mdv]: | ||
| if "%DEPLOYMENT_NAME%"=="" set DEPLOYMENT_NAME=mdv | ||
| set HAS_EXISTING_DEPLOYMENT=0 | ||
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=%DEPLOYMENT_NAME%"') do set HAS_EXISTING_DEPLOYMENT=1 | ||
| if "%HAS_EXISTING_DEPLOYMENT%"=="1" ( | ||
| set /p DEPLOY_ACTION=Deployment "%DEPLOYMENT_NAME%" exists. Action ^(redeploy/rename/exit^) [redeploy]: | ||
| if /I "!DEPLOY_ACTION!"=="" set DEPLOY_ACTION=redeploy | ||
| if /I "!DEPLOY_ACTION!"=="exit" exit /b 0 | ||
| if /I "!DEPLOY_ACTION!"=="rename" ( | ||
| set /p DEPLOYMENT_NAME=New deployment name: | ||
| if "!DEPLOYMENT_NAME!"=="" ( | ||
| echo Error: deployment name cannot be empty. | ||
| exit /b 1 | ||
| ) | ||
| set HAS_EXISTING_DEPLOYMENT=0 | ||
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=!DEPLOYMENT_NAME!"') do set HAS_EXISTING_DEPLOYMENT=1 | ||
| if "!HAS_EXISTING_DEPLOYMENT!"=="1" ( | ||
| echo Error: deployment "!DEPLOYMENT_NAME!" already exists. Please choose a different name. | ||
| exit /b 1 | ||
| ) | ||
| set DEPLOY_MODE=new | ||
| ) else ( | ||
| set DEPLOY_MODE=replace | ||
| ) | ||
| ) else ( | ||
| set DEPLOY_MODE=new | ||
| echo No existing deployment found for "%DEPLOYMENT_NAME%". Using new mode. | ||
| ) | ||
| set ENV_FILE=.env.%DEPLOYMENT_NAME% |
There was a problem hiding this comment.
Validate DEPLOYMENT_NAME before interpolating it into cmd and PowerShell commands.
In this script the deployment name flows into docker, powershell, redirections, filenames, and DB_SCHEMA. Characters like &, |, %, !, <, >, /, and quotes can break the command line or target the wrong file entirely.
Suggested fix
set /p DEPLOYMENT_NAME=Deployment name [mdv]:
if "%DEPLOYMENT_NAME%"=="" set DEPLOYMENT_NAME=mdv
+echo(%DEPLOYMENT_NAME%| findstr /R "^[A-Za-z0-9][A-Za-z0-9._-]*$" >nul || (
+ echo Error: deployment name may only contain letters, numbers, dot, underscore, and dash.
+ exit /b 1
+)Apply the same check after the rename prompt as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set /p DEPLOYMENT_NAME=Deployment name [mdv]: | |
| if "%DEPLOYMENT_NAME%"=="" set DEPLOYMENT_NAME=mdv | |
| set HAS_EXISTING_DEPLOYMENT=0 | |
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=%DEPLOYMENT_NAME%"') do set HAS_EXISTING_DEPLOYMENT=1 | |
| if "%HAS_EXISTING_DEPLOYMENT%"=="1" ( | |
| set /p DEPLOY_ACTION=Deployment "%DEPLOYMENT_NAME%" exists. Action ^(redeploy/rename/exit^) [redeploy]: | |
| if /I "!DEPLOY_ACTION!"=="" set DEPLOY_ACTION=redeploy | |
| if /I "!DEPLOY_ACTION!"=="exit" exit /b 0 | |
| if /I "!DEPLOY_ACTION!"=="rename" ( | |
| set /p DEPLOYMENT_NAME=New deployment name: | |
| if "!DEPLOYMENT_NAME!"=="" ( | |
| echo Error: deployment name cannot be empty. | |
| exit /b 1 | |
| ) | |
| set HAS_EXISTING_DEPLOYMENT=0 | |
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=!DEPLOYMENT_NAME!"') do set HAS_EXISTING_DEPLOYMENT=1 | |
| if "!HAS_EXISTING_DEPLOYMENT!"=="1" ( | |
| echo Error: deployment "!DEPLOYMENT_NAME!" already exists. Please choose a different name. | |
| exit /b 1 | |
| ) | |
| set DEPLOY_MODE=new | |
| ) else ( | |
| set DEPLOY_MODE=replace | |
| ) | |
| ) else ( | |
| set DEPLOY_MODE=new | |
| echo No existing deployment found for "%DEPLOYMENT_NAME%". Using new mode. | |
| ) | |
| set ENV_FILE=.env.%DEPLOYMENT_NAME% | |
| set /p DEPLOYMENT_NAME=Deployment name [mdv]: | |
| if "%DEPLOYMENT_NAME%"=="" set DEPLOYMENT_NAME=mdv | |
| echo(%DEPLOYMENT_NAME%| findstr /R "^[A-Za-z0-9][A-Za-z0-9._-]*$" >nul || ( | |
| echo Error: deployment name may only contain letters, numbers, dot, underscore, and dash. | |
| exit /b 1 | |
| ) | |
| set HAS_EXISTING_DEPLOYMENT=0 | |
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=%DEPLOYMENT_NAME%"') do set HAS_EXISTING_DEPLOYMENT=1 | |
| if "%HAS_EXISTING_DEPLOYMENT%"=="1" ( | |
| set /p DEPLOY_ACTION=Deployment "%DEPLOYMENT_NAME%" exists. Action ^(redeploy/rename/exit^) [redeploy]: | |
| if /I "!DEPLOY_ACTION!"=="" set DEPLOY_ACTION=redeploy | |
| if /I "!DEPLOY_ACTION!"=="exit" exit /b 0 | |
| if /I "!DEPLOY_ACTION!"=="rename" ( | |
| set /p DEPLOYMENT_NAME=New deployment name: | |
| if "!DEPLOYMENT_NAME!"=="" ( | |
| echo Error: deployment name cannot be empty. | |
| exit /b 1 | |
| ) | |
| set HAS_EXISTING_DEPLOYMENT=0 | |
| for /f %%i in ('docker ps -aq --filter "label=com.docker.compose.project=!DEPLOYMENT_NAME!"') do set HAS_EXISTING_DEPLOYMENT=1 | |
| if "!HAS_EXISTING_DEPLOYMENT!"=="1" ( | |
| echo Error: deployment "!DEPLOYMENT_NAME!" already exists. Please choose a different name. | |
| exit /b 1 | |
| ) | |
| set DEPLOY_MODE=new | |
| ) else ( | |
| set DEPLOY_MODE=replace | |
| ) | |
| ) else ( | |
| set DEPLOY_MODE=new | |
| echo No existing deployment found for "%DEPLOYMENT_NAME%". Using new mode. | |
| ) | |
| set ENV_FILE=.env.%DEPLOYMENT_NAME% |
🧰 Tools
🪛 Blinter (1.0.112)
[error] 30-30: Potential command injection vulnerability. Explanation: User input used in commands without validation could allow malicious code execution. Recommendation: Validate and sanitize all user input before using in commands. Context: User input used in command without validation
(SEC001)
[error] 53-53: Unsafe SET command usage. Explanation: SET commands without proper validation or quoting can cause security issues. Recommendation: Always quote SET values and validate input: SET "var=safe value". Context: SET command value should be quoted for safety
(SEC002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy.bat` around lines 25 - 53, Validate and sanitize DEPLOYMENT_NAME every
time it is set (both initial prompt and inside the "rename" branch) before
interpolating into commands: add a validation step after the initial set /p
DEPLOYMENT_NAME and again after the rename prompt that rejects or re-prompts on
any disallowed characters (e.g. &|%!"^<>/\\ and whitespace) or enforces an
allowlist pattern (e.g. [A-Za-z0-9_.-]); if invalid, print an error and
exit/reprompt. Apply this same validation before using DEPLOYMENT_NAME in the
docker filter loop, building ENV_FILE, and any powershell/cmd redirections so
you never interpolate an unchecked value; reference the variables
DEPLOYMENT_NAME, DEPLOY_ACTION, HAS_EXISTING_DEPLOYMENT, and ENV_FILE when
adding these checks.
| read -p "Deployment name [mdv]: " deployment_name_input | ||
| DEPLOYMENT_NAME=${deployment_name_input:-mdv} | ||
|
|
||
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | ||
| read -p "Deployment '${DEPLOYMENT_NAME}' exists. Action (redeploy/rename/exit) [redeploy]: " deploy_action_input | ||
| DEPLOY_ACTION=$(echo "${deploy_action_input:-redeploy}" | tr '[:upper:]' '[:lower:]' | xargs) | ||
| if [[ "$DEPLOY_ACTION" == "exit" ]]; then | ||
| echo "Exiting deployment." | ||
| exit 0 | ||
| elif [[ "$DEPLOY_ACTION" == "rename" ]]; then | ||
| read -p "New deployment name: " new_deployment_name | ||
| if [[ -z "$new_deployment_name" ]]; then | ||
| echo -e "${RED}Error: deployment name cannot be empty.${NC}" | ||
| exit 1 | ||
| fi | ||
| DEPLOYMENT_NAME="$new_deployment_name" |
There was a problem hiding this comment.
Whitelist DEPLOYMENT_NAME before reusing it as a filename and schema.
This value is later written into .env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml, and DB_SCHEMA. Names containing /, .., or " can escape the expected path or break the later schema cleanup SQL.
Suggested fix
read -p "Deployment name [mdv]: " deployment_name_input
DEPLOYMENT_NAME=${deployment_name_input:-mdv}
+ if [[ ! "$DEPLOYMENT_NAME" =~ ^[A-Za-z0-9._-]+$ ]]; then
+ echo -e "${RED}Error: deployment name may only contain letters, numbers, dot, underscore, and dash.${NC}"
+ exit 1
+ fiApply the same validation after the rename prompt as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| read -p "Deployment name [mdv]: " deployment_name_input | |
| DEPLOYMENT_NAME=${deployment_name_input:-mdv} | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| read -p "Deployment '${DEPLOYMENT_NAME}' exists. Action (redeploy/rename/exit) [redeploy]: " deploy_action_input | |
| DEPLOY_ACTION=$(echo "${deploy_action_input:-redeploy}" | tr '[:upper:]' '[:lower:]' | xargs) | |
| if [[ "$DEPLOY_ACTION" == "exit" ]]; then | |
| echo "Exiting deployment." | |
| exit 0 | |
| elif [[ "$DEPLOY_ACTION" == "rename" ]]; then | |
| read -p "New deployment name: " new_deployment_name | |
| if [[ -z "$new_deployment_name" ]]; then | |
| echo -e "${RED}Error: deployment name cannot be empty.${NC}" | |
| exit 1 | |
| fi | |
| DEPLOYMENT_NAME="$new_deployment_name" | |
| read -p "Deployment name [mdv]: " deployment_name_input | |
| DEPLOYMENT_NAME=${deployment_name_input:-mdv} | |
| if [[ ! "$DEPLOYMENT_NAME" =~ ^[A-Za-z0-9._-]+$ ]]; then | |
| echo -e "${RED}Error: deployment name may only contain letters, numbers, dot, underscore, and dash.${NC}" | |
| exit 1 | |
| fi | |
| if docker ps -aq --filter "label=com.docker.compose.project=${DEPLOYMENT_NAME}" | grep -q .; then | |
| read -p "Deployment '${DEPLOYMENT_NAME}' exists. Action (redeploy/rename/exit) [redeploy]: " deploy_action_input | |
| DEPLOY_ACTION=$(echo "${deploy_action_input:-redeploy}" | tr '[:upper:]' '[:lower:]' | xargs) | |
| if [[ "$DEPLOY_ACTION" == "exit" ]]; then | |
| echo "Exiting deployment." | |
| exit 0 | |
| elif [[ "$DEPLOY_ACTION" == "rename" ]]; then | |
| read -p "New deployment name: " new_deployment_name | |
| if [[ -z "$new_deployment_name" ]]; then | |
| echo -e "${RED}Error: deployment name cannot be empty.${NC}" | |
| exit 1 | |
| fi | |
| DEPLOYMENT_NAME="$new_deployment_name" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy.sh` around lines 130 - 145, DEPLOYMENT_NAME is reused in filenames and
DB schema and isn't validated after user input or after the rename flow,
allowing unsafe values; add a whitelist validation (e.g., allow only
[A-Za-z0-9._-] and forbid sequences like ".." and characters like "/" and "\"
and quotes) applied immediately after setting DEPLOYMENT_NAME from
deployment_name_input and again after reading new_deployment_name in the rename
branch (referencing DEPLOYMENT_NAME and new_deployment_name), and if the value
fails the regex or is empty, print an error and exit 1 so only safe names are
used for .env.${DEPLOYMENT_NAME}, .docker-deploy.${DEPLOYMENT_NAME}.override.yml
and DB_SCHEMA.
| if db_schema: | ||
| schema_engine = create_engine(target_uri, isolation_level="AUTOCOMMIT") | ||
| schema_connection = None | ||
| try: | ||
| schema_connection = schema_engine.connect() | ||
| schema_connection.execute(text(f'CREATE SCHEMA IF NOT EXISTS "{db_schema}"')) | ||
| logger.info(f"Ensured database schema exists: {db_schema}") | ||
| finally: | ||
| if schema_connection: | ||
| schema_connection.close() | ||
| schema_engine.dispose() |
There was a problem hiding this comment.
Potential SQL injection risk with schema name.
The schema name is interpolated directly into the SQL statement using an f-string. While DB_SCHEMA typically comes from environment variables controlled by deployment scripts, this pattern could be exploited if the schema name contains malicious SQL.
🛡️ Suggested fix using identifier quoting
try:
schema_connection = schema_engine.connect()
- schema_connection.execute(text(f'CREATE SCHEMA IF NOT EXISTS "{db_schema}"'))
+ # Use quote_ident equivalent - PostgreSQL double-quote escaping
+ safe_schema = db_schema.replace('"', '""')
+ schema_connection.execute(text(f'CREATE SCHEMA IF NOT EXISTS "{safe_schema}"'))
logger.info(f"Ensured database schema exists: {db_schema}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/mdvtools/dbutils/mdv_server_app.py` around lines 223 - 233, The code
builds SQL by interpolating db_schema directly into text(...) which risks SQL
injection; change the CREATE SCHEMA call to safely quote or validate the
identifier before executing: obtain a safe quoted identifier via
schema_engine.dialect.identifier_preparer.quote(db_schema) (or use
sqlalchemy.schema.CreateSchema) and use that quoted value in the text() call
instead of raw f-string interpolation, and/or add a simple validation (e.g.,
allow only [A-Za-z0-9_\\-] or similar) on db_schema before attempting to create
the schema; update the block around create_engine, schema_engine, and
schema_connection to use the quoted identifier (or CreateSchema) when calling
schema_connection.execute(...) so the schema name cannot inject SQL.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/mdvtools/scripts/users_projects_dataframe.py`:
- Around line 42-47: The current validation in users_projects_dataframe.py only
checks types for auth0_domain, client_id, client_secret, and auth0_db_connection
so empty strings slip through; update the check to require non-empty strings
(e.g., isinstance(var, str) and var.strip()) for each of auth0_domain,
client_id, client_secret, and auth0_db_connection to enforce presence while
preserving type narrowing, and raise the existing "missing config" or ValueError
path when any of these are empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47ef7803-7e59-4b52-ab03-f7cdd37fb4d1
📒 Files selected for processing (2)
python/mdvtools/dbutils/test/unit/test_mdv_server_app.pypython/mdvtools/scripts/users_projects_dataframe.py
|
Actionable comments posted: 0 |
|
Hi @lferreira86, I was fixing some small issues raised by the CI and ran into these while trying to run the
local port_pattern='(0\.0\.0\.0|\[::\]|:::):([0-9]+)->5055'
if [[ "$ports_line" =~ $port_pattern ]]
|
@MarShaikh, please check my answers below: |
…e the same except a port number
* - `_fetch_admin_auth_ids()`, resolves the admin role id, pages through its members (with rate-limit retry), returns a set of auth0 `user_ids`, replaces `_fetch_user_roles_with_retry()`. - `_process_single_user()`, `is_admin` = `auth0_id` in `context.admin_auth_ids` - `sync_users_to_db()`, calls `_fetch_admin_auth_ids()` once before the user loop; stores it on the context. - Removed now-dead code, and updated stale docstring * rewrote the two tests that asserted per-user list_roles behavior
…ly the permissions file if given) once the app is healthy, then restart so the in-memory cache reflects the new rows
…o a bare deploy still sets admins up
…aches in-process without a container restart
3 participants
Branch to test replacement of PostgreSQL with SQLite backend database.
Summary by CodeRabbit
deploy.sh,deploy.bat, anddeploy_gui.sh, with selectable DB backend (sqlite/postgres), new vs replace/reuse modes, and automatic host port selectionDB_BACKENDwith improved PostgreSQL readiness and optional schema setup