Skip to content

fix: migration postgres azure #681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2025
Merged

fix: migration postgres azure #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5e9718
fix: migration using azure postgres with no superuser provileges
gfyrag Feb 12, 2025
ab5804d
fix: next migration
gfyrag Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
do $$
declare
_ledger record;
_vsql text;
_batch_size integer := 1000;
_date timestamp without time zone;
_count integer := 0;
begin
--todo: take explicit advisory lock to avoid concurrent migrations when the service is killed
set search_path = '{{.Schema}}';

-- cannot disable triggers at session level on Azure Postgres with no superuser privileges.
-- so we modify the trigger acting on transaction update to be triggered only if the metadata column is updated.
-- by the way, it's a good move to not trigger the update_transaction_metadata_history function on every update if not necessary.
for _ledger in select * from _system.ledgers where bucket = current_schema loop
_vsql = 'create or replace trigger "update_transaction_metadata_history_' || _ledger.id || '" after update of metadata on "transactions" for each row when (new.ledger = ''' || _ledger.name || ''') execute procedure update_transaction_metadata_history()';
execute _vsql;
end loop;
Comment on lines +14 to +17
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Review SQL injection risk in dynamic trigger creation.

The dynamic SQL construction using string concatenation with _ledger.id and _ledger.name could be vulnerable to SQL injection if these values contain malicious content.

Consider using proper quoting functions. Run this script to check if PostgreSQL's quote_ident and quote_literal functions are available in your environment:

If available, consider this safer approach:

_vsql = format(
  'create or replace trigger %s after update of metadata on transactions for each row when (new.ledger = %s) execute procedure update_transaction_metadata_history()',
  quote_ident('update_transaction_metadata_history_' || _ledger.id),
  quote_literal(_ledger.name)
);

🏁 Script executed:

#!/bin/bash
# Description: Check if proper quoting functions are available
# Expected: Functions should be available in PostgreSQL

curl -s https://www.postgresql.org/docs/current/functions-string.html | grep -A 2 "quote_ident"

Length of output: 1275

SQL Injection Risk: Use PostgreSQL Quoting Functions for Safe Dynamic SQL Generation

  • File: internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (Lines 14-17)

  • Issue: The dynamic SQL construction concatenates _ledger.id and _ledger.name directly, which may lead to SQL injection if these values ever contain unexpected or malicious content.

  • Verification: A shell script confirmed that PostgreSQL’s quote_ident function is available (and by extension, quote_literal is available), so safe quoting functions can be used.

  • Recommendation: Replace the current string concatenation with a safer version using the format function combined with quote_ident and quote_literal. For example:

    _vsql = format(
  'create or replace trigger %s after update of metadata on transactions for each row when (new.ledger = %s) execute procedure update_transaction_metadata_history()',
  quote_ident('update_transaction_metadata_history_' || _ledger.id),
  quote_literal(_ledger.name)
);

Please update the trigger creation code to use this format, ensuring that all identifiers and literals are safely quoted to mitigate any SQL injection risks.


-- select the date where the "11-make-stateless" migration has been applied
select tstamp into _date
from _system.goose_db_version
Expand All @@ -25,9 +34,6 @@ do $$
perform pg_notify('migrations-{{ .Schema }}', 'init: ' || _count);

for i in 0.._count-1 by _batch_size loop
-- disable triggers
set session_replication_role = replica;

with _rows as (
select *
from logs_transactions
Expand All @@ -40,9 +46,6 @@ do $$
from _rows
where transactions.ledger = _rows.ledger and transactions.id = _rows.transaction_id;

-- enable triggers
set session_replication_role = default;

commit;

perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size);
Expand Down
6 changes: 0 additions & 6 deletions internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ do $$

perform pg_notify('migrations-{{ .Schema }}', 'init: ' || (select count(*) from moves_view));

-- disable triggers
set session_replication_role = replica;

loop
with data as (
select transactions_id, volumes
Expand All @@ -54,9 +51,6 @@ do $$
commit;
end loop;

-- enable triggers
set session_replication_role = default;

drop table if exists moves_view;

alter table transactions
Expand Down