Skip to content

Conversation

@mtaylor-vailsys
Copy link

This is related to #1348.

ActiveRecord::ConnectionAdapters::SQLServer::DatabaseStatements#raw_execute previously transformed the query into a parameterized query, then called the method implementation from ActiveRecord::ConnectionAdapters::AbstractAdapter.

The AbstractAdapter impelementation logged the sql statement, then called the perform_query method.

Because the parameterized values were included in the sql statement before it was logged, ActiveRecord filter attributes were being bypassed, leaking values into logs that should otherwise have been filtered out.

This PR moves the application of parameters into the sql statement from the raw_execute method into the perform_query method (after the statement has been logged).
This allows SQL statements logged from activerecord-sqlserver-adapter to respect the ActiveRecord filter attributes.
This comes at the cost of no longer logging the exact sql that will be sent to the database server.

Previously, the adapter would have logged a statement like:

EXEC sp_executesql N'SELECT 1 AS one FROM [customers] WHERE [customers].[name] = @0 AND [customers].[id] = @1 ORDER BY [customers].[id] ASC OFFSET 0 ROWS FETCH NEXT @2 ROWS ONLY', N'@0 nvarchar(4000), @1 bigint, @2 int', @0 = N'David', @1 = 1, @2 = 1  [["name", "David"], ["id", 1], ["LIMIT", 1]]

The new logs would appear as:

SELECT 1 AS one FROM [customers] WHERE [customers].[name] = @0 AND [customers].[id] = @1 ORDER BY [customers].[id] ASC OFFSET 0 ROWS FETCH NEXT @2 ROWS ONLY  [["name", "David"], ["id", 1], ["LIMIT", 1]]

…eRecord

Prevents sensitive values from by-passing ActiveRecord filtered attributes.
Remove unnecessary duplication
Remove some lines added during debugging
@mtaylor-vailsys mtaylor-vailsys changed the title Apply parameterization to sql query after it has been logged by ActiveRecord Apply parameters to sql statement after it has been logged by ActiveRecord Oct 27, 2025
@aidanharan
Copy link
Contributor

@mtaylor-vailsys Thanks for the PR. It'll be a week or 2 before I have a chance to look at it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants