feat: add comprehensive permission management system #50
Conversation
Implements a robust permission management system that ensures database owners have complete access to all extension schemas, custom types, sequences, and functions. Key changes: - Add /scripts/lib/permissions.sh with comprehensive grant functions covering all 46 extensions (AGE, PostGIS, pgvector, pg_cron, etc.) - Add /postgres/initdb/03-permission-healthcheck.sh for startup validation and auto-repair of missing permissions - Update grant_db_owner_privileges() to use the new permissions library - Add CLI commands: permissions-validate, permissions-repair, permissions-report for manual permission management - Add CORE_DATA_PERMISSION_HEALTHCHECK and CORE_DATA_PERMISSION_REPAIR environment variables for controlling startup behavior The system addresses permission gaps including: - Extension schemas: ag_catalog, cron, squeeze, partman, topology, tiger - Custom types: agtype, graphid, geometry, geography, vector - Sequences and functions in all extension schemas - Default privileges for future objects - Role-specific search_path configuration All operations are idempotent (GRANT is no-op if privilege exists) and additive-only (no REVOKE operations) for safe repeated execution.
Summary of ChangesHello @paudley, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the PostgreSQL permission management system by introducing a robust, comprehensive framework. It ensures that database owners consistently have the necessary privileges across all extension schemas, custom types, sequences, and functions. The changes include a new permission library, an automated startup health check with auto-repair capabilities, and dedicated CLI tools for manual validation, repair, and reporting, providing greater control and reliability over database permissions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Add 5 new tests for the permission management system: - test_permissions_validate: Validates CLI command runs successfully - test_permissions_repair: Tests permission repair functionality - test_permissions_report: Tests CSV report generation - test_permissions_age_operations: Tests AGE graph operations as non-superuser - test_permissions_vector_operations: Tests pgvector operations as non-superuser Tests use @pytest.mark.permissions marker for selective execution.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive permission management system, which is a great addition for ensuring database owners have the necessary access. The implementation is well-structured, with a dedicated library for permissions, a startup health check, and CLI commands for manual management. However, I've identified several critical SQL injection vulnerabilities in the new shell scripts that need to be addressed. These vulnerabilities arise from improper handling of user-provided input and database-retrieved values in SQL queries. I've also found some minor issues with logging messages. My review provides specific suggestions to fix these security flaws and improve the overall robustness of the new system.
| _permissions_log() { | ||
| echo "[core_data:permissions] $*" >&2 | ||
| } |
There was a problem hiding this comment.
To prevent SQL injection vulnerabilities in this script and others that source it, it's crucial to properly escape variables interpolated into SQL queries. I recommend adding a helper function for this purpose at the beginning of the script. This function will be used to fix several vulnerabilities in this file and in manage.sh.
| _permissions_log() { | |
| echo "[core_data:permissions] $*" >&2 | |
| } | |
| # _psql_quote_literal escapes a string for use as a literal in a PostgreSQL query. | |
| _psql_quote_literal() { | |
| printf "'%s'" "${1//\'/\'\'}" | |
| } | |
| # _permissions_log outputs a timestamped log message to stderr. | |
| _permissions_log() { | |
| echo "[core_data:permissions] $*" >&2 | |
| } |
| has_usage=$(_run_psql_query "${db}" " | ||
| SELECT CASE WHEN EXISTS ( | ||
| SELECT 1 FROM pg_namespace n | ||
| WHERE n.nspname = '${schema}' | ||
| AND has_schema_privilege('${role}', n.oid, 'USAGE') | ||
| ) THEN 'yes' ELSE 'no' END; | ||
| ") |
There was a problem hiding this comment.
This query is vulnerable to SQL injection. The ${role} variable, which can originate from user input, is directly interpolated into the SQL string. This could allow an attacker to execute arbitrary SQL. Please use the _psql_quote_literal helper function I suggested to properly escape this variable.
| has_usage=$(_run_psql_query "${db}" " | |
| SELECT CASE WHEN EXISTS ( | |
| SELECT 1 FROM pg_namespace n | |
| WHERE n.nspname = '${schema}' | |
| AND has_schema_privilege('${role}', n.oid, 'USAGE') | |
| ) THEN 'yes' ELSE 'no' END; | |
| ") | |
| has_usage=$(_run_psql_query "${db}" " | |
| SELECT CASE WHEN EXISTS ( | |
| SELECT 1 FROM pg_namespace n | |
| WHERE n.nspname = $(_psql_quote_literal "${schema}") | |
| AND has_schema_privilege($(_psql_quote_literal "${role}"), n.oid, 'USAGE') | |
| ) THEN 'yes' ELSE 'no' END; | |
| ") |
| missing_count=$(_run_psql_query "${db}" " | ||
| SELECT COUNT(*) FROM pg_proc p | ||
| JOIN pg_namespace n ON p.pronamespace = n.oid | ||
| WHERE n.nspname = '${schema}' | ||
| AND NOT has_function_privilege('${role}', p.oid, 'EXECUTE'); | ||
| ") |
There was a problem hiding this comment.
This query is vulnerable to SQL injection. The ${role} variable is directly interpolated into the SQL string. Since this can come from user input, it creates a security risk. Please use the _psql_quote_literal helper function to escape it.
| missing_count=$(_run_psql_query "${db}" " | |
| SELECT COUNT(*) FROM pg_proc p | |
| JOIN pg_namespace n ON p.pronamespace = n.oid | |
| WHERE n.nspname = '${schema}' | |
| AND NOT has_function_privilege('${role}', p.oid, 'EXECUTE'); | |
| ") | |
| missing_count=$(_run_psql_query "${db}" " | |
| SELECT COUNT(*) FROM pg_proc p | |
| JOIN pg_namespace n ON p.pronamespace = n.oid | |
| WHERE n.nspname = $(_psql_quote_literal "${schema}") | |
| AND NOT has_function_privilege($(_psql_quote_literal "${role}"), p.oid, 'EXECUTE'); | |
| ") |
| missing_count=$(_run_psql_query "${db}" " | ||
| SELECT COUNT(*) FROM pg_class c | ||
| JOIN pg_namespace n ON c.relnamespace = n.oid | ||
| WHERE n.nspname = '${schema}' AND c.relkind = 'S' | ||
| AND NOT has_sequence_privilege('${role}', c.oid, 'USAGE'); | ||
| ") |
There was a problem hiding this comment.
This query is vulnerable to SQL injection. The ${role} variable is directly interpolated into the SQL string, which is unsafe. Please use the _psql_quote_literal helper function to properly escape the variable.
| missing_count=$(_run_psql_query "${db}" " | |
| SELECT COUNT(*) FROM pg_class c | |
| JOIN pg_namespace n ON c.relnamespace = n.oid | |
| WHERE n.nspname = '${schema}' AND c.relkind = 'S' | |
| AND NOT has_sequence_privilege('${role}', c.oid, 'USAGE'); | |
| ") | |
| missing_count=$(_run_psql_query "${db}" " | |
| SELECT COUNT(*) FROM pg_class c | |
| JOIN pg_namespace n ON c.relnamespace = n.oid | |
| WHERE n.nspname = $(_psql_quote_literal "${schema}") AND c.relkind = 'S' | |
| AND NOT has_sequence_privilege($(_psql_quote_literal "${role}"), c.oid, 'USAGE'); | |
| ") |
| if [[ -n "${POSTGRES_EXEC_MODE:-}" && "${POSTGRES_EXEC_MODE}" == "container" ]]; then | ||
| owner=$(psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | ||
| else | ||
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | ||
| fi |
There was a problem hiding this comment.
This block is vulnerable to SQL injection. The ${db} variable, which contains a database name, is interpolated directly into the SQL query. Database names can contain special characters (like a single quote) that can break the query or be used for injection. Please use the _psql_quote_literal helper function to safely quote the database name.
| if [[ -n "${POSTGRES_EXEC_MODE:-}" && "${POSTGRES_EXEC_MODE}" == "container" ]]; then | |
| owner=$(psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | |
| else | |
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | |
| fi | |
| if [[ -n "${POSTGRES_EXEC_MODE:-}" && "${POSTGRES_EXEC_MODE}" == "container" ]]; then | |
| owner=$(psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = $(_psql_quote_literal "${db}");") | |
| else | |
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = $(_psql_quote_literal "${db}");") | |
| fi |
scripts/manage.sh
Outdated
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") |
There was a problem hiding this comment.
This is a critical SQL injection vulnerability. The ${db} variable comes from a command-line argument and is interpolated directly into the SQL query. An attacker could provide a malicious database name to execute arbitrary SQL. Please use the _psql_quote_literal helper function (defined in permissions.sh) to properly escape the variable.
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | |
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = $(_psql_quote_literal "${db}");") |
scripts/manage.sh
Outdated
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") |
There was a problem hiding this comment.
This is a critical SQL injection vulnerability, identical to the one in the permissions-validate command. The user-provided ${db} variable is used in a raw SQL query. This must be escaped to prevent arbitrary SQL execution. Please use the _psql_quote_literal helper function.
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | |
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | |
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = $(_psql_quote_literal "${db}");") |
| set_role_search_path() { | ||
| local db=$1 | ||
| local role=$2 | ||
| local search_path=${3:-${DEFAULT_SEARCH_PATH}} | ||
|
|
||
| _run_psql "${db}" <<SQL | ||
| ALTER ROLE "${role}" IN DATABASE "${db}" SET search_path TO ${search_path}; | ||
| SQL | ||
| _permissions_log "Set search_path for '${role}' in '${db}' to: ${search_path}" | ||
| } |
There was a problem hiding this comment.
This function is vulnerable to SQL injection through the search_path parameter. While it's currently only called with a safe default value, the function signature is unsafe as it allows passing an arbitrary string that gets interpolated into the ALTER ROLE command. To harden it, I recommend removing the parameter and always using the hardcoded DEFAULT_SEARCH_PATH. This makes the function's behavior explicit and secure.
| set_role_search_path() { | |
| local db=$1 | |
| local role=$2 | |
| local search_path=${3:-${DEFAULT_SEARCH_PATH}} | |
| _run_psql "${db}" <<SQL | |
| ALTER ROLE "${role}" IN DATABASE "${db}" SET search_path TO ${search_path}; | |
| SQL | |
| _permissions_log "Set search_path for '${role}' in '${db}' to: ${search_path}" | |
| } | |
| set_role_search_path() { | |
| local db=$1 | |
| local role=$2 | |
| local search_path=${DEFAULT_SEARCH_PATH} | |
| _run_psql "${db}" <<SQL | |
| ALTER ROLE "${role}" IN DATABASE "${db}" SET search_path TO ${search_path}; | |
| SQL | |
| _permissions_log "Set search_path for '${role}' in '${db}' to: ${search_path}" | |
| } |
scripts/lib/permissions.sh
Outdated
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT EXECUTE ON FUNCTIONS TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| RAISE NOTICE 'Granted permissions on schema % to %', '${schema}', '${role}'; |
There was a problem hiding this comment.
The RAISE NOTICE statement is using placeholders incorrectly. The % character is the placeholder, and variables should be passed as subsequent arguments. Using quote_ident is also best practice to correctly format identifiers in the notice.
| RAISE NOTICE 'Granted permissions on schema % to %', '${schema}', '${role}'; | |
| RAISE NOTICE 'Granted permissions on schema % to %', quote_ident('${schema}'), quote_ident('${role}'); |
scripts/lib/permissions.sh
Outdated
| WHERE n.nspname = '${schema}' AND t.typname = '${type_name}' | ||
| ) THEN | ||
| EXECUTE format('GRANT USAGE ON TYPE %I.%I TO %I', '${schema}', '${type_name}', '${role}'); | ||
| RAISE NOTICE 'Granted USAGE on type %.% to %', '${schema}', '${type_name}', '${role}'; |
There was a problem hiding this comment.
The RAISE NOTICE statement is using placeholders incorrectly. The %.% will be printed literally. To substitute variables, use % as a placeholder and pass the variables as arguments. Using quote_ident is also best practice for identifiers.
| RAISE NOTICE 'Granted USAGE on type %.% to %', '${schema}', '${type_name}', '${role}'; | |
| RAISE NOTICE 'Granted USAGE on type %.% to %', quote_ident('${schema}'), quote_ident('${type_name}'), quote_ident('${role}'); |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive permission management system for PostgreSQL database owners, ensuring they have complete access to all extension schemas, custom types, sequences, and functions across 46 PostgreSQL extensions. The system includes validation, auto-repair capabilities, and CLI management tools.
- Adds a new permissions library (
scripts/lib/permissions.sh) with functions for granting and validating permissions across schemas, tables, sequences, functions, and custom types - Integrates startup health check (
postgres/initdb/03-permission-healthcheck.sh) with configurable auto-repair - Adds three new CLI commands (
permissions-validate,permissions-repair,permissions-report) for manual permission management
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/lib/permissions.sh |
New 442-line library implementing comprehensive permission management functions including grants, validation, repair, and reporting capabilities |
scripts/manage.sh |
Adds three new CLI commands with argument parsing for permission validation, repair, and reporting operations |
postgres/initdb/03-permission-healthcheck.sh |
New startup script that validates and optionally auto-repairs permissions on container initialization |
scripts/lib/db.sh |
Updates grant_db_owner_privileges() to use the new comprehensive permission system instead of limited inline SQL |
.env.example |
Adds two environment variables to control permission health check behavior (CORE_DATA_PERMISSION_HEALTHCHECK and CORE_DATA_PERMISSION_REPAIR) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER ON TABLES TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| -- Sequences | ||
| EXECUTE format('GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA %I TO %I', '${schema}', '${role}'); | ||
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT ALL PRIVILEGES ON SEQUENCES TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| -- Functions | ||
| EXECUTE format('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '${schema}', '${role}'); | ||
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT EXECUTE ON FUNCTIONS TO %I', | ||
| '${grantor}', '${schema}', '${role}'); |
There was a problem hiding this comment.
Security/Maintainability Issue: The grantor variable is interpolated directly into format strings within DO blocks. While this is currently set to POSTGRES_SUPERUSER, if this value comes from environment variables or user input, it could be exploited. Consider validating this value or using a safer approach for setting the grantor role.
scripts/manage.sh
Outdated
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") |
There was a problem hiding this comment.
SQL Injection Risk: The db variable from user input (via --db flag) is directly interpolated into the SQL query without validation. A malicious user could inject SQL through the database name parameter. Consider validating the database name against allowed characters or using psql's -c parameter escaping more carefully.
scripts/lib/permissions.sh
Outdated
| } | ||
|
|
||
| # validate_schema_permissions checks if a role has USAGE on a schema. | ||
| # Returns 0 if permissions are correct, 1 if missing. |
There was a problem hiding this comment.
[nitpick] Documentation Issue: The function comment says "Returns 0 if permissions are correct, 1 if missing" but should clarify that this is the shell exit code (not an output value) for consistency with bash conventions.
| # Returns 0 if permissions are correct, 1 if missing. | |
| # Sets shell exit code to 0 if permissions are correct, 1 if missing. |
| _run_psql "${db}" <<SQL | ||
| DO \$perm\$ | ||
| BEGIN | ||
| IF EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = '${schema}') THEN | ||
| -- Schema access | ||
| EXECUTE format('GRANT USAGE ON SCHEMA %I TO %I', '${schema}', '${role}'); | ||
|
|
||
| -- Tables (including TRUNCATE for bulk operations) | ||
| EXECUTE format('GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER ON ALL TABLES IN SCHEMA %I TO %I', '${schema}', '${role}'); | ||
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER ON TABLES TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| -- Sequences | ||
| EXECUTE format('GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA %I TO %I', '${schema}', '${role}'); | ||
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT ALL PRIVILEGES ON SEQUENCES TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| -- Functions | ||
| EXECUTE format('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '${schema}', '${role}'); | ||
| EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT EXECUTE ON FUNCTIONS TO %I', | ||
| '${grantor}', '${schema}', '${role}'); | ||
|
|
||
| RAISE NOTICE 'Granted permissions on schema % to %', '${schema}', '${role}'; | ||
| END IF; | ||
| END; | ||
| \$perm\$; | ||
| SQL |
There was a problem hiding this comment.
SQL Injection Risk: The schema, role, and db variables are directly interpolated into SQL strings without proper sanitization or validation. While the schema names come from a hardcoded array, the role and db parameters come from user input or database queries. An attacker could potentially inject malicious SQL through database names or role names.
Consider using parameterized queries or at minimum, validate that inputs contain only allowed characters (e.g., alphanumeric, underscore) before string interpolation. PostgreSQL's identifier quoting alone (via format's %I) is not sufficient when the values are first interpolated into the DO block's string literals.
| has_usage=$(_run_psql_query "${db}" " | ||
| SELECT CASE WHEN EXISTS ( | ||
| SELECT 1 FROM pg_namespace n | ||
| WHERE n.nspname = '${schema}' | ||
| AND has_schema_privilege('${role}', n.oid, 'USAGE') | ||
| ) THEN 'yes' ELSE 'no' END; | ||
| ") |
There was a problem hiding this comment.
SQL Injection Risk: The schema and role variables are directly interpolated into WHERE clauses without validation. While these queries use has_schema_privilege, the unsafe string interpolation could still be exploited if malicious values are passed.
| _run_psql "${db}" <<SQL | ||
| DO \$type_perm\$ | ||
| BEGIN | ||
| IF EXISTS ( | ||
| SELECT 1 FROM pg_type t | ||
| JOIN pg_namespace n ON t.typnamespace = n.oid | ||
| WHERE n.nspname = '${schema}' AND t.typname = '${type_name}' | ||
| ) THEN | ||
| EXECUTE format('GRANT USAGE ON TYPE %I.%I TO %I', '${schema}', '${type_name}', '${role}'); | ||
| RAISE NOTICE 'Granted USAGE on type %.% to %', '${schema}', '${type_name}', '${role}'; | ||
| END IF; | ||
| END; | ||
| \$type_perm\$; | ||
| SQL |
There was a problem hiding this comment.
SQL Injection Risk: Similar to the grant_schema_permissions function, the schema, type_name, and role variables are directly interpolated into SQL strings within the DO block. While schema and type_name come from the CUSTOM_TYPES array, they should still be validated, and the role parameter needs proper sanitization before being used in format strings.
| _run_psql "${db}" <<SQL | ||
| GRANT ALL PRIVILEGES ON DATABASE "${db}" TO "${owner}"; | ||
| SQL |
There was a problem hiding this comment.
SQL Injection Risk: The db and owner variables are directly interpolated into the GRANT statement without validation. These values should be validated to ensure they contain only safe characters before being used in SQL statements.
scripts/lib/permissions.sh
Outdated
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | ||
| else | ||
| # Running via compose_exec (manage.sh) | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" |
There was a problem hiding this comment.
Error Handling Issue: ON_ERROR_STOP=0 disables error stopping, which means SQL errors will be silently ignored. This could mask permission grant failures and lead to incomplete permission setup. Consider using ON_ERROR_STOP=1 and implementing proper error handling to ensure all grants succeed or the operation fails clearly.
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| # Running via compose_exec (manage.sh) | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| psql --set ON_ERROR_STOP=1 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| # Running via compose_exec (manage.sh) | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=1 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" |
scripts/manage.sh
Outdated
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") |
There was a problem hiding this comment.
SQL Injection Risk: Similar to the permissions-validate command, the db variable from user input is directly interpolated into the SQL query without validation. This creates a SQL injection vulnerability.
| permissions-validate) | ||
| ensure_env | ||
| ensure_postgres_running | ||
| db="" | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --db) | ||
| db=$2 | ||
| shift 2 | ||
| ;; | ||
| --db=*) | ||
| db=${1#*=} | ||
| shift | ||
| ;; | ||
| -h | --help) | ||
| echo "Usage: ${0##*/} permissions-validate [--db NAME]" >&2 | ||
| exit 0 | ||
| ;; | ||
| --) | ||
| shift | ||
| break | ||
| ;; | ||
| *) | ||
| echo "Unknown option for permissions-validate: $1" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done | ||
| if [[ -n "${db}" ]]; then | ||
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | ||
| if [[ -z "${owner}" ]]; then | ||
| echo "[core_data] Database '${db}' not found." >&2 | ||
| exit 1 | ||
| fi | ||
| if ! validate_all_permissions "${db}" "${owner}"; then | ||
| exit 1 | ||
| fi | ||
| else | ||
| if ! startup_permission_healthcheck "false"; then | ||
| exit 1 | ||
| fi | ||
| fi | ||
| ;; | ||
| permissions-repair) | ||
| ensure_env | ||
| ensure_postgres_running | ||
| db="" | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --db) | ||
| db=$2 | ||
| shift 2 | ||
| ;; | ||
| --db=*) | ||
| db=${1#*=} | ||
| shift | ||
| ;; | ||
| -h | --help) | ||
| echo "Usage: ${0##*/} permissions-repair [--db NAME]" >&2 | ||
| exit 0 | ||
| ;; | ||
| --) | ||
| shift | ||
| break | ||
| ;; | ||
| *) | ||
| echo "Unknown option for permissions-repair: $1" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done | ||
| if [[ -n "${db}" ]]; then | ||
| owner=$(compose_exec env PGHOST="${POSTGRES_HOST}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${POSTGRES_DB:-postgres}" \ | ||
| --command "SELECT pg_catalog.pg_get_userbyid(datdba) FROM pg_database WHERE datname = '${db}';") | ||
| if [[ -z "${owner}" ]]; then | ||
| echo "[core_data] Database '${db}' not found." >&2 | ||
| exit 1 | ||
| fi | ||
| repair_permissions "${db}" "${owner}" | ||
| else | ||
| startup_permission_healthcheck "true" | ||
| fi | ||
| ;; | ||
| permissions-report) | ||
| ensure_env | ||
| ensure_postgres_running | ||
| db="${POSTGRES_DB:-postgres}" | ||
| output="" | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --db) | ||
| db=$2 | ||
| shift 2 | ||
| ;; | ||
| --db=*) | ||
| db=${1#*=} | ||
| shift | ||
| ;; | ||
| --output) | ||
| output=$2 | ||
| shift 2 | ||
| ;; | ||
| --output=*) | ||
| output=${1#*=} | ||
| shift | ||
| ;; | ||
| -h | --help) | ||
| echo "Usage: ${0##*/} permissions-report [--db NAME] [--output PATH]" >&2 | ||
| exit 0 | ||
| ;; | ||
| --) | ||
| shift | ||
| break | ||
| ;; | ||
| *) | ||
| echo "Unknown option for permissions-report: $1" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done | ||
| generate_permission_report "${db}" "${output}" | ||
| ;; |
There was a problem hiding this comment.
Missing Test Coverage: The new permission management commands (permissions-validate, permissions-repair, permissions-report) lack automated test coverage. Given that the codebase has comprehensive tests in tests/test_manage.py, these critical commands should have tests to ensure they work correctly, especially since permission errors could lead to production issues. Consider adding tests that:
- Verify the commands execute without errors
- Test validation detects missing permissions
- Test repair correctly fixes permission issues
- Test report generation produces valid CSV output
Security fixes: - Add _psql_quote_literal() helper to prevent SQL injection - Use quoted literals in validate_schema_permissions - Use quoted literals in validate_function_permissions - Use quoted literals in validate_sequence_permissions - Use quoted literals in startup_permission_healthcheck db lookup - Use quoted literals in manage.sh permissions-validate command - Use quoted literals in manage.sh permissions-repair command Code quality improvements: - Harden set_role_search_path() by removing unsafe search_path parameter - Fix RAISE NOTICE placeholder usage with quote_ident() - Add 60-second timeout to PostgreSQL wait loop in healthcheck script - Update function documentation comments for clarity (outputs vs returns) - Extract _parse_db_argument() helper to reduce argument parsing duplication - Simplify permissions-validate, permissions-repair, permissions-report commands
- Fix _run_psql to handle both $2 argument and heredoc/stdin input
- Use ${2:-} to allow unset second argument when using heredocs
- Register 'permissions' pytest marker in pyproject.toml to fix warnings
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | ||
| else | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | ||
| fi | ||
| else | ||
| # Running via compose_exec (manage.sh) | ||
| if [[ -n "${sql}" ]]; then | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | ||
| else | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" |
There was a problem hiding this comment.
Using --set ON_ERROR_STOP=0 means psql will continue executing even when SQL errors occur, which could lead to silent failures when granting permissions. This could cause the permission system to appear successful even when some grants fail. Consider using ON_ERROR_STOP=1 (or omitting the flag entirely as it's the default) to ensure the script fails fast on any SQL error, or add explicit error checking after each SQL operation.
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | |
| fi | |
| else | |
| # Running via compose_exec (manage.sh) | |
| if [[ -n "${sql}" ]]; then | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" | |
| psql --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| psql --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | |
| fi | |
| else | |
| # Running via compose_exec (manage.sh) | |
| if [[ -n "${sql}" ]]; then | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" |
| # Check and repair schema USAGE | ||
| if ! validate_schema_permissions "${db}" "${schema}" "${role}"; then | ||
| _permissions_log " [REPAIR] Granting permissions on schema '${schema}'" | ||
| grant_schema_permissions "${db}" "${schema}" "${role}" | ||
| fi |
There was a problem hiding this comment.
The repair logic only checks schema USAGE permissions (line 356) before deciding to call grant_schema_permissions(). This means if a role has schema USAGE but is missing function EXECUTE or sequence permissions, those won't be repaired. The validate_all_permissions() function checks all three (schema, functions, sequences), but repair_permissions() only checks one. This creates an inconsistency where validation can detect issues that repair won't fix. Consider checking all permission types or always calling grant_schema_permissions() to ensure complete repair.
| # Check and repair schema USAGE | |
| if ! validate_schema_permissions "${db}" "${schema}" "${role}"; then | |
| _permissions_log " [REPAIR] Granting permissions on schema '${schema}'" | |
| grant_schema_permissions "${db}" "${schema}" "${role}" | |
| fi | |
| _permissions_log " [REPAIR] Granting permissions on schema '${schema}'" | |
| grant_schema_permissions "${db}" "${schema}" "${role}" |
| # Run permission repair | ||
| result = run_manage(env, "permissions-repair", "--db", "repair_test_db") | ||
| assert result.returncode == 0 | ||
| assert "permissions" in result.stderr.lower() or "repair" in result.stderr.lower() |
There was a problem hiding this comment.
[nitpick] The assertion on line 1887 checks for "permissions" or "repair" in stderr, but this test might be too lenient. If the repair command fails silently or outputs to stdout instead of stderr, the test could pass incorrectly. Consider checking for a more specific success message or verifying actual permission changes.
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | ||
| else | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | ||
| fi | ||
| else | ||
| # Running via compose_exec (manage.sh) | ||
| if [[ -n "${sql}" ]]; then | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | ||
| else | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" |
There was a problem hiding this comment.
Using --set ON_ERROR_STOP=0 means psql will continue executing even when SQL errors occur, which could lead to silent failures when granting permissions. This could cause the permission system to appear successful even when some grants fail. Consider using ON_ERROR_STOP=1 (or omitting the flag entirely as it's the default) to ensure the script fails fast on any SQL error, or add explicit error checking after each SQL operation.
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | |
| fi | |
| else | |
| # Running via compose_exec (manage.sh) | |
| if [[ -n "${sql}" ]]; then | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --set ON_ERROR_STOP=0 --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" | |
| psql --username "${POSTGRES_USER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| psql --username "${POSTGRES_USER:-postgres}" --dbname "${db}" | |
| fi | |
| else | |
| # Running via compose_exec (manage.sh) | |
| if [[ -n "${sql}" ]]; then | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" <<< "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" |
| # Source the permissions library if not already loaded | ||
| local lib_dir | ||
| lib_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | ||
| # shellcheck source=scripts/lib/permissions.sh | ||
| source "${lib_dir}/permissions.sh" | ||
|
|
There was a problem hiding this comment.
[nitpick] Sourcing the permissions library inside a function means it will be re-sourced every time grant_db_owner_privileges is called. While Bash allows this, it's inefficient and could cause issues if the library has side effects. Since scripts/manage.sh already sources permissions.sh at line 53, this redundant sourcing is unnecessary. Consider removing this source statement and relying on the top-level sourcing in manage.sh.
| # Source the permissions library if not already loaded | |
| local lib_dir | |
| lib_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | |
| # shellcheck source=scripts/lib/permissions.sh | |
| source "${lib_dir}/permissions.sh" |
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | ||
| else | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null |
There was a problem hiding this comment.
Redirecting stderr to /dev/null (via 2>/dev/null) suppresses all error messages from psql queries. This could hide important errors during permission validation, making it difficult to diagnose permission issues. For example, if the database doesn't exist or there's a connection error, these would be silently ignored. Consider either removing the redirection or logging errors to help with debugging.
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | |
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" |
| _run_psql "${db}" <<SQL | ||
| DO \$perm\$ | ||
| BEGIN | ||
| IF EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = '${schema}') THEN |
There was a problem hiding this comment.
In the WHERE clause condition, ${schema} is directly interpolated into the SQL string without proper escaping. Although the schema names come from a controlled list (EXTENSION_SCHEMAS), if this function is ever called with user-controlled input, it could be vulnerable to SQL injection. Consider using the _psql_quote_literal function or ensuring the value is passed through format() with %L for proper escaping: WHERE nspname = format('%L', '${schema}')
| IF EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = '${schema}') THEN | |
| IF EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = format('%L', '${schema}')) THEN |
| IF EXISTS ( | ||
| SELECT 1 FROM pg_type t | ||
| JOIN pg_namespace n ON t.typnamespace = n.oid | ||
| WHERE n.nspname = '${schema}' AND t.typname = '${type_name}' |
There was a problem hiding this comment.
In the WHERE clause conditions, ${schema} and ${type_name} are directly interpolated into the SQL string without proper escaping. Although these values come from a controlled list (CUSTOM_TYPES), if this function is ever called with user-controlled input, it could be vulnerable to SQL injection. Consider using the _psql_quote_literal function or ensuring the values are passed through format() with %L for proper escaping.
| WHERE n.nspname = '${schema}' AND t.typname = '${type_name}' | |
| WHERE n.nspname = quote_literal('${schema}')::text AND t.typname = quote_literal('${type_name}')::text |
|
|
||
| # 1. Database-level privileges | ||
| _run_psql "${db}" <<SQL | ||
| GRANT ALL PRIVILEGES ON DATABASE "${db}" TO "${owner}"; |
There was a problem hiding this comment.
The ${db} and ${owner} variables are directly interpolated into the GRANT statement without proper escaping. Although these values come from trusted sources, if they contain special characters, the SQL could be malformed. Consider using PostgreSQL's format() function with %I identifiers for proper escaping within a DO block, similar to how it's done in grant_schema_permissions().
| GRANT ALL PRIVILEGES ON DATABASE "${db}" TO "${owner}"; | |
| DO \$\$ | |
| BEGIN | |
| EXECUTE format('GRANT ALL PRIVILEGES ON DATABASE %I TO %I', '${db}', '${owner}'); | |
| END | |
| \$\$; |
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | ||
| else | ||
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | ||
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null |
There was a problem hiding this comment.
Redirecting stderr to /dev/null (via 2>/dev/null) suppresses all error messages from psql queries. This could hide important errors during permission validation, making it difficult to diagnose permission issues. For example, if the database doesn't exist or there's a connection error, these would be silently ignored. Consider either removing the redirection or logging errors to help with debugging.
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" 2>/dev/null | |
| psql --tuples-only --no-align --username "${POSTGRES_USER:-postgres}" --dbname "${db}" --command "${sql}" | |
| else | |
| compose_exec env PGHOST="${POSTGRES_HOST:-localhost}" PGPASSWORD="${POSTGRES_SUPERUSER_PASSWORD:-}" \ | |
| psql --tuples-only --no-align --username "${POSTGRES_SUPERUSER:-postgres}" --dbname "${db}" --command "${sql}" |
1 participant
Summary
Implements a robust permission management system that ensures database owners have complete access to all extension schemas, custom types, sequences, and functions across all 46 PostgreSQL extensions.
/scripts/lib/permissions.sh) with functions for granting schema, table, sequence, function, and type permissions/postgres/initdb/03-permission-healthcheck.sh) that validates and auto-repairs permissions on container startuppermissions-validate,permissions-repair,permissions-reportgrant_db_owner_privileges()to use the new comprehensive permission systemPermission Coverage
Extension Schemas:
public,ag_catalog(AGE),cron(pg_cron),squeeze(pg_squeeze),partman(pg_partman)topology,tiger,tiger_data(PostGIS),address_standardizer*,core_data_adminCustom Types:
ag_catalog.agtype,ag_catalog.graphidpublic.geometry,public.geography,public.box2d,public.box3d,public.vectortopology.topogeometryConfiguration
New environment variables in
.env.example:CORE_DATA_PERMISSION_HEALTHCHECK=1- Enable/disable startup validationCORE_DATA_PERMISSION_REPAIR=1- Enable/disable auto-repairUsage
Test plan
permissions-validatecommand workspermissions-repaircommand applies missing grantspermissions-reportgenerates valid CSVSELECT * FROM ag_catalog.create_graph('test');CORE_DATA_PERMISSION_HEALTHCHECK=0disables the check🤖 Generated with Claude Code