Skip to content
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.

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

Redact SQL in errors #34468

Merged
merged 1 commit into from Nov 23, 2018
Merged

Redact SQL in errors #34468

merged 1 commit into from Nov 23, 2018

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Nov 16, 2018

Summary

Fixes #34316.

Allows redaction of SQL in translated SQL errors with config.active_record.redacted_sql_errors = true. I'm not a regex pro, so I think there's room for improvement on both expressions. Also, now that we don't use mocha, what's the best way to mock initializer that raises an error?

r? @rafaelfranca

Copy link
Member

@rafaelfranca rafaelfranca left a comment

I don't feel good about parsing SQL to redact it, it can become too complex.

@tenderlove @matthewd @jeremy WDYT?

end
end

def print_redaction_warning
Copy link
Member

@rafaelfranca rafaelfranca Nov 16, 2018

I think we don't need to warn. We can just skip the redact method if prepared statements is enabled.

Copy link
Member Author

@gmcgibbon gmcgibbon Nov 16, 2018

👍 I agree

return sql unless ActiveRecord::Base.redacted_sql_errors
print_redaction_warning if ActiveRecord::Base.connection.prepared_statements
if %w(SELECT UPDATE DELETE).any? { |type| sql.starts_with?(type) }
sql.gsub(/= ('.*'|TRUE|FALSE|\d*\.?\d*)/, "= <REDACTED>")
Copy link
Member

@rafaelfranca rafaelfranca Nov 16, 2018

I really think it is not a good idea to parse SQL. There are too many dialects, operation, operators. If people wants to redact SQL is better to not send the SQL.

Copy link
Member Author

@gmcgibbon gmcgibbon Nov 16, 2018

That's a fair point. I kind of agree, but I wanted to see how far I could get parsing it with regexes. I've tested this against MySQL, SQLite, and PG, but it can all goes out the window if we're expected to filter raw SQL strings crafted by developers.


setup do
@book = Book.first
ActiveRecord::Base.connection.instance_variable_set(:@prepared_statements, false)
Copy link
Member

@tenderlove tenderlove Nov 19, 2018

I'm not a big fan of setting ivars like this. If we decide to change the ivar name, this code will break. It's coupling the test to the internals of the connection object. Instead, can we set up a new connection that doesn't have prepared statements enabled?

Copy link
Member Author

@gmcgibbon gmcgibbon Nov 19, 2018

I agree that this isn't great. We have that, but it doesn't have any tables in it 😑. I will see what I can do.

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Nov 19, 2018

I don't feel good about parsing SQL to redact it, it can become too complex.

I kind of agree with this. We've had bad luck in the past with parsing SQL. OTOH, this should only happen when there are exceptions, so maybe it doesn't matter? I have no strong feelings either way, though I do think we should improve the test as my comment says. 😊

@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Nov 19, 2018

I'll update the PR to just not include the SQL in the error string if prepared statements is on.

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 19, 2018

Yeah, I too don't like the idea of parsing the SQL -- DBMSes don't agree on how to quote strings, so even though it sounds simple, it'd get messy fast.

I'm actually not a big fan of how we currently jam the query into the exception message, because it can make the message long and unwieldy (and very ugly if someone sticks it in a heading, say... which we do by default).

What if we move it into a separate property on StatementInvalid? That way it wouldn't leak by default when other systems blindly copy the message around, and our own error pages get a chance to format it better (read: separate from the message [it's really more like a stack trace entry, tbh], at a sensible font size, and in a fixed-width font). Possible extra credit: include the binds too.

@gmcgibbon gmcgibbon force-pushed the redact_sql_in_errors branch 3 times, most recently from fad4071 to b310150 Nov 20, 2018
message = "#{e.class.name}: #{e.message.force_encoding sql.encoding}: #{sql}"
end
def translate_exception_class(e, sql, binds)
message = "#{e.class.name}: #{e.message.force_encoding("UTF-8")}"
Copy link
Member Author

@gmcgibbon gmcgibbon Nov 20, 2018

We have coverage for forcing message encoding here, but since there's no more SQL interpolation, do we still need to encode this?

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Nov 20, 2018

What if we move it into a separate property on StatementInvalid? That way it wouldn't leak by default when other systems blindly copy the message around, and our own error pages get a chance to format it better (read: separate from the message [it's really more like a stack trace entry, tbh], at a sensible font size, and in a fixed-width font). Possible extra credit: include the binds too.

+1 on this. I like the idea of the statement being a property of the exception and not part of the message itself.

@gmcgibbon gmcgibbon force-pushed the redact_sql_in_errors branch 4 times, most recently from b480aba to d16f037 Nov 20, 2018
rescue Encoding::CompatibilityError
message = "#{e.class.name}: #{e.message.force_encoding sql.encoding}: #{sql}"
end
def translate_exception_class(e, sql, binds)
Copy link
Member

@rafaelfranca rafaelfranca Nov 20, 2018

This and all other methods arguments changes is a breaking change without deprecation. All adapters will have to change their code and all users subclassing any StatementInvalid exception will have to change their calls and definitions.

I think those changes are fine but I'd add all those breaking changes to the CHANGELOG.

@gmcgibbon gmcgibbon force-pushed the redact_sql_in_errors branch 3 times, most recently from 2e00260 to 0dbed36 Nov 22, 2018
Move `ActiveRecord::StatementInvalid` SQL to error property.
Also add bindings as an error property.
@gmcgibbon gmcgibbon force-pushed the redact_sql_in_errors branch from 0dbed36 to 192b7bc Nov 22, 2018
@rafaelfranca rafaelfranca merged commit 246ee77 into rails:master Nov 23, 2018
2 checks passed
yahonda added a commit to yahonda/oracle-enhanced that referenced this issue Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this issue Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this issue Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this issue Nov 27, 2018
@gmcgibbon gmcgibbon deleted the redact_sql_in_errors branch Dec 12, 2018
Hamms added a commit to code-dot-org/code-dot-org that referenced this issue Oct 8, 2021
Hamms added a commit to code-dot-org/code-dot-org that referenced this issue Oct 8, 2021
Hamms added a commit to code-dot-org/code-dot-org that referenced this issue Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants