Skip to content

Conversation

gmcgibbon
Copy link
Member

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

Choose a reason for hiding this comment

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

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

@tenderlove @matthewd @jeremy WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I agree

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

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

@matthewd
Copy link
Member

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 Compare November 20, 2018 16:39
Copy link
Member Author

Choose a reason for hiding this comment

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

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

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 Compare November 20, 2018 23:23
Copy link
Member

Choose a reason for hiding this comment

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

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 Compare November 22, 2018 17:42
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 Compare November 22, 2018 18:53
@rafaelfranca rafaelfranca merged commit 246ee77 into rails:master Nov 23, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2018
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants