updated postgres_session resource properly escape queries #1939

aaronlippold · 2017-06-18T22:05:42Z

updated resource to use 'shellwords' module to escape the query
fixed a small courner case in the error detection - error: vs error

Fixes: #1814

Signed-off-by: Aaron Lippold lippold@gmail.com

aaronlippold · 2017-06-19T12:22:43Z

@chris-rock not sure how we would do unit tests for this one? Suggestions?

chris-rock · 2017-06-19T12:24:28Z

We simulate a sample call and add those result to: https://github.com/chef/inspec/blob/master/test/helper.rb#L164

adamleff · 2017-06-19T15:17:28Z

To test that we're building the correct command, it would be best to break out the building of the psql command string into a separate method and test that method.

Then we can test the resource itself by doing what @chris-rock suggested and ensure we take the output received from psql query and treat it properly.

aaronlippold · 2017-06-19T16:52:55Z

I was thinking the same thing. I will work on that update. It should not be too big to break it out.

…

-------- Aaron Lippold lippold@gmail.com 260-255-4779 twitter/aim/yahoo,etc. 'aaronlippold'

On Mon, Jun 19, 2017 at 11:17 AM, Adam Leff ***@***.***> wrote: To test the actual query, it would be best to break out the building of the psql command string into a separate method and test that method. Then we can test the resource itself by doing what @chris-rock <https://github.com/chris-rock> suggested and ensure we take the output received from psql query and treat it properly. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABauaP4C0UzmhjNKlPvhqIfCC0aXXov_ks5sFpEKgaJpZM4N9noh> .

aaronlippold · 2017-06-19T17:04:55Z

So something like this @adamleff? def initialize(user, pass, host = nil) @user = user || 'postgres' @pass = pass @host = host || 'localhost' end def escaped_query(query) return warn 'no query specificed' if query.empty? || query.nil? Shellwords.escape(query) end def create_psql_cmd(query, db = []) dbs = db.map { |x| "-d #{x}" }.join(' ') @escaped_query = escaped_query(query) "PGPASSWORD='#{@pass}' psql -U #{@user} #{dbs} -h #{@host} -A -t -c #{@escaped_query}" end def query(query, db = []) @psql_cmd = create_psql_cmd(query,db) cmd = inspec.command(@psql_cmd) out = cmd.stdout + "\n" + cmd.stderr if cmd.exit_status != 0 || out =~ /could not connect to .*/ || out.downcase =~ /^error:.*/ skip_resource "Can't read run query #{query.inspect} on postgres_session: #{out}" else Lines.new(cmd.stdout.strip, "PostgreSQL query: #{query}") end end

…

-------- Aaron Lippold lippold@gmail.com 260-255-4779 twitter/aim/yahoo,etc. 'aaronlippold'

On Mon, Jun 19, 2017 at 8:24 AM, Christoph Hartmann < ***@***.***> wrote: We simulate a sample call and add those result to: https://github.com/chef/inspec/blob/master/test/helper.rb#L164 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABauaG-j9olUiFGvOQZZ-F36r9a-yv4Hks5sFmh9gaJpZM4N9noh> .

adamleff · 2017-06-19T17:43:18Z

Possibly? It's really hard to tell when it's in a comment, and isn't a good facility with which to comment on individual lines. Go ahead and add it as a commit and we can work through it that way, please.

adamleff

@aaronlippold thanks for your PR. This is a good first step. I left you a bunch of feedback that we'd need to see addressed before we can consider merging. Please add comments to any of the feedback items if you have questions.

adamleff · 2017-06-23T14:57:47Z

lib/resources/postgres_session.rb


    def initialize(user, pass, host = nil)
      @user = user || 'postgres'
      @pass = pass
      @host = host || 'localhost'
    end

-    def query(query, db = [])
+    def escaped_query(query)
+      return warn 'no query specificed' if query.empty? || query.nil?


You don't need to do this. In the #query method, the query argument is required so you'll never end up in this method with an empty string.

If someone deliberately passes nil to #query then they deserve to get a Ruby exception :)

adamleff · 2017-06-23T14:58:37Z

lib/resources/postgres_session.rb

@@ -39,25 +41,32 @@ class PostgresSession < Inspec.resource(1)
        its('output') { should eq('') }
      end
    "
+    attr_reader :user, :pass, :host, :db, :escaped_query, :psql_cmd


We should only be adding attribute reader methods for parameters we want to expose externally, or if we need it for testing purposes (i.e. you need to mock something that would otherwise be an instance variable... in which case, helper methods are probably the order of the day, or writing different tests). Why do these need to be exposed?

I expose user, pass and db check the constructor. I exposed db, escaped_query and psql_cmd to test that each of the helper functions was working as expected.

I guess I could just expose user, pass, host and psql_cmd and that would enable us to check that all the parts are working as expected over time.

adamleff · 2017-06-23T14:59:37Z

lib/resources/postgres_session.rb

-      escaped_query = query.gsub(/\\/, '\\\\').gsub(/"/, '\\"').gsub(/\$/, '\\$')
-      # run the query
-      cmd = inspec.command("PGPASSWORD='#{@pass}' psql -U #{@user} #{dbs} -h #{@host} -A -t -c \"#{escaped_query}\"")
+      @escaped_query = escaped_query(query)


Why are you setting this to an instance variable? It doesn't look like it's used anywhere outside of this create_psql_cmd method...

You should be able to just do this:

"PGPASSWORD='#{@pass}' psql -U #{@user} #{dbs} -h #{@host} -A -t -c #{escaped_query(query)}"

adamleff · 2017-06-23T15:00:17Z

lib/resources/postgres_session.rb

+    end
+
+    def query(query, db = [])
+      @db = db


Why is this being set to an instance variable?

I mainly wanted to be able to test the main parts of the construction process to what I was passing to psql. i.e. that the query escaped correctly with the corner case tests - like : SELECT current_setting('client_min_messages') .

I guess if I just expose @psql_cmd I would be able to look at the parts of that and ensure that the functions are all producing what we are expecting.

Worked out a solution that doesn't require anything to be exposed.

adamleff · 2017-06-23T15:00:29Z

lib/resources/postgres_session.rb

+
+    def query(query, db = [])
+      @db = db
+      @psql_cmd = create_psql_cmd(query, @db)


Why is this being set to an instance variable instead of a local variable?

adamleff · 2017-06-23T15:27:19Z

test/unit/resources/postgres_session_test.rb

+  it 'verify postgres_session create_psql_cmd function' do
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.create_psql_cmd("SELECT * FROM STUDENTS;",['testdb'])).must_equal "PGPASSWORD='mypass' psql -U myuser -d testdb -h 127.0.0.1 -A -t -c SELECT\\ \\*\\ FROM\\ STUDENTS\\;"
+      _(resource.db).must_equal ['testdb']


This isn't something this function should be responsible for, and therefore shouldn't be tested. Just worry about testing that create_psql_cmd generates the appropriate output based on a set of input.

adamleff · 2017-06-23T15:27:30Z

test/unit/resources/postgres_session_test.rb

+
+  it 'verify postgres_session create_psql_cmd function' do
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.create_psql_cmd("SELECT * FROM STUDENTS;",['testdb'])).must_equal "PGPASSWORD='mypass' psql -U myuser -d testdb -h 127.0.0.1 -A -t -c SELECT\\ \\*\\ FROM\\ STUDENTS\\;"


Check your indentation... you're 2-spaces indented too far.

adamleff · 2017-06-23T15:37:12Z

test/unit/resources/postgres_session_test.rb

+  it 'verify postgres_session escaped_query function' do
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.escaped_query("SELECT * FROM STUDENTS;")).must_equal "SELECT\\ \\*\\ FROM\\ STUDENTS\\;"
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')


This is a duplicate line and is not needed.

adamleff · 2017-06-23T15:37:27Z

test/unit/resources/postgres_session_test.rb

+
+  it 'verify postgres_session escaped_query function' do
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.escaped_query("SELECT * FROM STUDENTS;")).must_equal "SELECT\\ \\*\\ FROM\\ STUDENTS\\;"


Indentation needs to be fixed -- indented too far.

adamleff · 2017-06-23T15:37:34Z

test/unit/resources/postgres_session_test.rb

+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.escaped_query("SELECT * FROM STUDENTS;")).must_equal "SELECT\\ \\*\\ FROM\\ STUDENTS\\;"
+    resource = load_resource('postgres_session','myuser','mypass','127.0.0.1')
+      _(resource.escaped_query("SELECT current_setting('client_min_messages')")).must_equal "SELECT\\ current_setting\\(\\'client_min_messages\\'\\)"


Indentation needs to be fixed -- indented too far.

adamleff · 2017-06-23T15:39:19Z

@aaronlippold Aaron, also, to make the reviews easier until we're finished with the review, please do not rebase your changes. Just add additional commits without pulling in any merge commits, etc. This will make the review process faster, and we'll squash before we merge.

adamleff

This looks pretty close - one more cleanup item.

adamleff · 2017-06-26T13:45:13Z

lib/resources/postgres_session.rb

+
+    def create_psql_cmd(query, db = [])
+      dbs = db.map { |x| "-d #{x}" }.join(' ')
+      @escaped_query = escaped_query(query)


This line can be eliminated, correct? You're not using this variable anywhere, and you're calling escaped_query directly in the returned string in the next line.

I think so, done and pushed.

chris-rock · 2017-06-28T11:21:06Z

@aaronlippold we switched our merge process in inspec. we do not need to rebase anymore since we use squash and merge only now.

fixed resource to use 'shellwords' module to escape the query requested chances in method architecture for testing added unit tests Fixes: inspec#1814 Signed-off-by: Aaron Lippold <lippold@gmail.com>

Signed-off-by: Aaron Lippold <lippold@gmail.com>

adamleff · 2017-06-30T15:11:12Z

@aaronlippold just FYI, I force-pushed to your branch so that I could fix the merge conflict for you. Just a heads-up that you'll need to force-pull or destroy your branch and pull it down from the remote again.

chris-rock

Thank you @aaronlippold

adamhjk added the in progress label Jun 18, 2017

aaronlippold force-pushed the al/postgres_session_query_escape branch from f37aad3 to 5c3ba24 Compare June 19, 2017 03:18

aaronlippold changed the title ~~updated resource to use 'shellwords' module~~ updated potgres_session resource to use 'shellwords' module Jun 19, 2017

aaronlippold force-pushed the al/postgres_session_query_escape branch from 5c3ba24 to f7ea527 Compare June 19, 2017 15:01

aaronlippold force-pushed the al/postgres_session_query_escape branch 3 times, most recently from 70ad996 to 4d23415 Compare June 19, 2017 18:20

adamleff suggested changes Jun 23, 2017

View reviewed changes

adamleff suggested changes Jun 26, 2017

View reviewed changes

aaronlippold added 4 commits June 30, 2017 11:00

fixed a small courner case in the error detection - error: vs error

fd83a2b

fixed resource to use 'shellwords' module to escape the query requested chances in method architecture for testing added unit tests Fixes: inspec#1814 Signed-off-by: Aaron Lippold <lippold@gmail.com>

updated resource and tests with requested review changes

e510168

Signed-off-by: Aaron Lippold <lippold@gmail.com>

removed unneeded call to escaped_query in the create_sql_cmd.

9737770

Signed-off-by: Aaron Lippold <lippold@gmail.com>

removed license info

7d60aa7

Signed-off-by: Aaron Lippold <lippold@gmail.com>

adamleff force-pushed the al/postgres_session_query_escape branch from bc8adf6 to 7d60aa7 Compare June 30, 2017 15:02

adamleff changed the title ~~updated potgres_session resource to use 'shellwords' module~~ updated postgres_session resource properly escape queries Jun 30, 2017

adamleff added the Type: Bug Feature not working as expected label Jun 30, 2017

adamleff approved these changes Jun 30, 2017

View reviewed changes

adamleff requested review from chris-rock and arlimus June 30, 2017 15:12

chris-rock approved these changes Jul 3, 2017

View reviewed changes

chris-rock merged commit 1b58763 into inspec:master Jul 3, 2017

chris-rock removed the in progress label Jul 3, 2017

aaronlippold mentioned this pull request Aug 19, 2019

MssqlSession.query is not escaping double quote correctly #4392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated postgres_session resource properly escape queries #1939

updated postgres_session resource properly escape queries #1939

aaronlippold commented Jun 18, 2017

aaronlippold commented Jun 19, 2017

chris-rock commented Jun 19, 2017

adamleff commented Jun 19, 2017 •

edited

Loading

aaronlippold commented Jun 19, 2017 via email

aaronlippold commented Jun 19, 2017 via email

adamleff commented Jun 19, 2017

adamleff left a comment

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

adamleff Jun 23, 2017

aaronlippold Jun 24, 2017

adamleff Jun 23, 2017

adamleff commented Jun 23, 2017

adamleff left a comment

adamleff Jun 26, 2017

aaronlippold Jun 26, 2017

chris-rock commented Jun 28, 2017

adamleff commented Jun 30, 2017

chris-rock left a comment

updated postgres_session resource properly escape queries #1939

updated postgres_session resource properly escape queries #1939

Conversation

aaronlippold commented Jun 18, 2017

aaronlippold commented Jun 19, 2017

chris-rock commented Jun 19, 2017

adamleff commented Jun 19, 2017 • edited Loading

aaronlippold commented Jun 19, 2017 via email

aaronlippold commented Jun 19, 2017 via email

adamleff commented Jun 19, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff commented Jun 23, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock commented Jun 28, 2017

adamleff commented Jun 30, 2017

chris-rock left a comment

Choose a reason for hiding this comment

adamleff commented Jun 19, 2017 •

edited

Loading