sql-obfuscation does not sanitize SQL that exceed size limits

[arielvalentin]

Description of the bug

A recent change to how SQL query comments are pre-prepended to the statement has resulted in triggering logic in the SQL obfuscation helper that bypasses executing the regular expression to sanitize the substring of query.

opentelemetry-ruby-contrib/helpers/sql-obfuscation/lib/opentelemetry/helpers/sql_obfuscation.rb

Line 118 in f817d6b

return truncate_statement(sql, regex, obfuscation_limit) if sql.size > obfuscation_limit

The regular expression does not seem to match on the comments index and ends up returning the raw contents of the SQL:

/*service.name:foo,deployment.environtment:production,tracecontext:00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-00,rails.route:examples/bars#index,host.name:baz-abc123.example.com*/ SELECT user.id FROM users where user.login = 'secretUserNameThatShouldBeObfuscated'... SQL truncated (> 2000 characters)

We must ensure that SQL is sanitized or omit the statement entirely.

Share details about your runtime

Operating system details: Linux, Ubuntu 20.04 LTS
RUBY_ENGINE: "ruby"
RUBY_VERSION: "3.3.4"
RAILS_VERSION: "8.0.0.alpha"

Share a simplified reproduction if possible

[arielvalentin]

Going to summarize our conversation from slack.

Here are a few things I would like for us to consider:

• The sanitizer does not work when comments appear first. We should probably revert the changes that returned the substring with the diagnostic message.

• We probably should not return diagnostic information in the attribute value anymore.

The OTel spec does not have limits unless specified by OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT.

Should we only apply a limit if this value is set?

In addition to that, if other attributes are truncated, the spec doesn't require that we add any diagnostic information for why an attribute was truncated.

• Allowing users to specify a db.statement attribute length limit should be ok. We may not want to take this functionality away, but again we probably should not enforce a default 2K limit anymore.

• We need some benchmarks that we could publish for some of these use cases. I'm not sure we can incorporate in the test suite but it would be good to have them at least available to run in an action.

[kaylareopelle]

Whoops, linked this to #1149, but since we have more ideas about how to fix obfuscation described in this issue, I reopened it.

[kaylareopelle]

@arielvalentin - New versions of the pg, mysql2, and trilogy gems have been released with the #1149 bugfix.

See: #1162

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.