feat: preventing ddl and dml statements from autosubmiting #6105

vlastahajek · 2024-09-12T16:00:17Z

This pull request introduces improvement of query editor to disable auto submission dangerous queries such as DROP, DELETE, ALTER, ...

CHANGELOG.md updated with a link to the PR (not the Issue)
Well-formatted commit messages
Rebased/mergeable
Tests pass
Sign CLA (if not already signed)

vlastahajek · 2024-10-09T12:28:58Z

PR is now ready for early review.
There is still some work needed:

excluded statements show 'skipped' when the query is returned to the previous state
submit button sometimes doesn't submit an excluded query
Cleaning - remove logging

karel-rehor · 2024-10-10T15:20:49Z

Preliminary feedback
PR-6105-Testing241010.csv
Preliminary tests raise a few questions. Mainly with content of queryFilter.ts

UPDATE statement does not appear in the Influxql documentation.
INSERT appears as a keyword but is not documented. When used in the QueryEditor, after clicking submit an error message appears error parsing query: found INSERT, expected SELECT, DELETE, SHOW, CREATE, DROP, EXPLAIN, GRANT, REVOKE, ALTER, SET, KILL at line 1, char 1
DELETE statement appears and is documented. The following was attempted, along with variations of specifying the database and measurement: DELETE FROM wellhead.autogen.flow WHERE celsius > 40.0, however when clicking "Submit" the QueryEditor always returns an error of either database not found: or error parsing query: retention policy not supported. Perhaps there's a trick I'm missing or a work around.
Maybe the following should be added to this file:
1. REVOKE
2. GRANT
3. SET - not well documented
4. KILL

https://docs.influxdata.com/enterprise_influxdb/v1/query_language/spec/#keywords

The following non-excluded DQL statements work as expected:

SHOW - e.g. SHOW DATABASES or SHOW MEASUREMENTS ON wellhead
SELECT - e.g. SELECT * FROM wellhead.autogen.flow

karel-rehor · 2024-10-15T15:12:17Z

Baseline Test

I've checked a revised test plan against the latest distribution (wget https://download.influxdata.com/chronograf/releases/chronograf_1.10.5_amd64.deb)

The test plan is based on the commands found in queryFilter.ts and suggestions in previous comment

SFL = Submit on Focus Loss (Query Editor)
SRR = Submit automatic on Reload
SBC = Submit on Button Click

Universal Statements: EXPLAIN, SELECT, SHOW
Restricted Statements (Proposed): ALTER, CREATE, DELETE, DROP, GRANT, KILL, INSERT(?), REVOKE, SET, UPDATE(?), USE(*)

(*) DO we want to restrict the USE statement automatically. Statements like USE hosts; SELECT * FROM cpu can be handy.

Universal statements can be submitted arbitrarily on focus change or page reload (to restore state of previous session).

Restricted statements should not be submitted arbitrarily through focus loss or on page reload. FAIL below indicates that such behavior occurred. Still the submit button should work.

Command Type	Tab 1 - SFL	Tab N - SFL	Tab 1 - SRR	Tab N - SRR	Tab 1 - SBC	Tab N - SBC
Universal - SELECT	OK	OK	OK	OK	OK	OK
Restricted - CREATE	FAIL	FAIL	FAIL	FAIL	OK	OK

Command	SFL	SRR	SBC
DROP	FAIL	OK(1)	OK
DELETE	FAIL	FAIL	OK
ALTER	FAIL	FAIL	OK
CREATE	FAIL	FAIL	OK
UPDATE	N/A(2)	N/A	N/A
INSERT	N/A(3)	N/A	N/A
Proposed
GRANT	FAIL	FAIL	OK
REVOKE	FAIL	FAIL	OK
SET	FAIL	FAIL	OK

(1) - DROP Statements disappear on reload
(2) - UPDATE is not listed as a statement in InfluxQL documentation
(3) - INSERT statement results in error message that INSERT not found among supported statements in Chronograph

PR-6105-Testing241014-standard-product.csv

N.B. Should statements SHOW USERS and SHOW GRANTS also be restricted from arbitrary loading for security reasons.

karel-rehor · 2024-10-16T12:31:20Z

Test of Commit 8808fee

Tested 2024.10.16

Local Build running on port 7777 - to avoid conflicts with product build listening on 8888
Test Env:

OS - Ubuntu 22.04
Browser - Opera as a neutral but very stable baseline browser - Opera 114.0.5

SFL = Submit on Focus Loss (Query Editor)
SRR = Submit automatic on Reload
SBC = Submit on Button Click

Universal Statements: EXPLAIN, SELECT, SHOW
Restricted Statements (Proposed): ALTER, CREATE, DELETE, DROP, GRANT, KILL, INSERT(?), REVOKE, SET(3 - below), UPDATE(?), USE

Universal statements can be submitted arbitrarily on focus change or page reload (to restore state of previous session).

Restricted statements should not be submitted arbitrarily through focus loss or on page reload. FAIL below indicates that such behavior occurred. Still the submit button should work.

Command Type	Tab 1 - SFL	Tab N - SFL	Tab 1 - SRR	Tab N - SRR	Tab 1 - SBC	Tab N - SBC
Universal - SELECT	PASS	PASS	PASS	PASS	PASS	PASS
Restricted - CREATE	PASS	PASS	PASS	PASS	PASS	PASS

Restricted Commands

Command	SFL	SRR	SBC
DROP	PASS	PASS	PASS
DELETE	PASS	PASS	PASS
ALTER	PASS	PASS	PASS
CREATE	PASS	PASS	PASS
UPDATE	N/A(1)	N/A	N/A
INSERT	PASS	PASS	PASS(2)
GRANT	PASS	PASS	PASS
REVOKE	PASS	PASS	PASS
USE	PASS	PASS	PASS
SET(3)	FAIL	FAIL	PASS

(1) UPDATE does not appear as a keyword in InfluxQL, however it has been added as a placeholder in the event of future changes.
(2) Test was of USE ephemeral; INSERT gopher,location=pond nuts=42i, so more a second test of the USE statement. INSERT works in CLI influx client, but is not supported in chronograf, which returns the error message error parsing query: found INSERT, expected SELECT, DELETE, SHOW, CREATE, DROP, EXPLAIN, GRANT, REVOKE, ALTER, SET, KILL at line 1, char 1. Indicated as PASS here, because behavior has not changed.
(3) SET can be used to set/change passwords e.g. SET PASSWORD FOR "my-user" = 'SuperSecretPW'. Undesired submits via chronograf could be an issue. Whether this statement should be restricted or not is still up for consideration, but not a blocker to closing this PR.

Universal Commands

Command	SFL	SRR	SBC
SELECT	PASS	PASS	PASS
SHOW	PASS	PASS	PASS

PR-6105-Testing241016-8808fee4.csv

karel-rehor · 2024-10-21T12:31:48Z

Test of Commit e904a25

Tested 2024.10.21

New Issue

With restricted statements, discovered that after a restricted statement is sent successfully, the statement can then be resubmitted when another statement in another tab is sent, or when another tab is closed. See below COTB-Resub record of test of restricted statements.

Use Case:

In tab 1 submit CREATE DATABASE test1 and submit with the submit button
In tab 2 submit SHOW DATABASES
In tab 3 submit DROP DATABASE test1
In influx cli verify database dropped with SHOW DATABASE
In Chronograf delete tab 3
The statement in tab 1 is resubmitted and the database is recreated

Recommendation

The SET statement should also be restricted.

Test Run Details

Local Build running on port 7777 - to avoid conflicts with product build listening on 8888
Test Env:

OS - Ubuntu 22.04
Browser - Opera as a neutral but very stable baseline browser - Opera 114.0.5

SFL = Submit on Focus Loss (Query Editor)
SRR = Submit automatic on Reload
SBC = Submit on Button Click
COTB-Resub = Change in other tab resubmit – means command already sent successfully, and is resubmitted after change in another tab. It is expected that restricted statements will not be resubmitted in this way.

Universal Statements: EXPLAIN, SELECT, SHOW
Restricted Statements (Proposed): ALTER, CREATE, DELETE, DROP, GRANT, KILL, INSERT(?), REVOKE, SET(3 - below), UPDATE(?), USE

Universal statements can be submitted arbitrarily on focus change or page reload (to restore state of previous session).

Restricted statements should not be submitted arbitrarily through focus loss or on page reload. FAIL below indicates that such behavior occurred. Still the submit button should work.

Command Type	Tab 1 - SFL	Tab N - SFL	Tab 1 - SRR	Tab N - SRR	Tab 1 - SBC	Tab N - SBC	COTB-Resub
Universal - SELECT	PASS	PASS	PASS	PASS	PASS	PASS	PASS
Restricted - CREATE	PASS	PASS	PASS	PASS	PASS	PASS	FAIL

Restricted Commands

Command	SFL	SRR	SBC
DROP	PASS	PASS	PASS
DELETE	PASS	PASS	PASS
ALTER	PASS	PASS	PASS
CREATE	PASS	PASS	PASS
UPDATE	N/A(1)	N/A	N/A
INSERT	PASS	PASS	PASS(2)
GRANT	PASS	PASS	PASS
REVOKE	PASS	PASS	PASS
USE	PASS	PASS	PASS
SET(3)	FAIL	FAIL	PASS

(1) UPDATE does not appear as a keyword in InfluxQL, however it has been added as a placeholder in the event of future changes.
(2) Test was of USE ephemeral; INSERT gopher,location=pond nuts=42i, so more a second test of the USE statement. INSERT works in CLI influx client, but is not supported in chronograf, which returns the error message error parsing query: found INSERT, expected SELECT, DELETE, SHOW, CREATE, DROP, EXPLAIN, GRANT, REVOKE, ALTER, SET, KILL at line 1, char 1. Indicated as PASS here, because behavior has not changed.
(3) SET can be used to set/change passwords e.g. SET PASSWORD FOR "my-user" = 'SuperSecretPW'. Undesired submits via chronograf could be an issue. Whether this statement should be restricted or not is still up for consideration, but not a blocker to closing this PR.

Universal Commands

Command	SFL	SRR	SBC
SELECT	PASS	PASS	PASS
SHOW	PASS	PASS	PASS

PR-6105-Testing241021-e904a259.csv

karel-rehor

See comment following manual test in general coversation. #6105 (comment)

A new minor issue was uncovered.

vlastahajek · 2024-10-23T12:29:49Z

Fixed auto-submitting, when no change was done and also when the Refresh button is clicked.

There is still a problem in the query status text in case of an excluded statement is still in the submitted form or returned to the submitted form, but an other statement is submitted. This results in "Unsubmitted" state.

karel-rehor · 2024-10-23T15:10:58Z

Test of Commit 630d521

Tested 2024.10.23

Previous Issue

See comment from 10.21 above. Seems to work correctly now

Recommendation

The SET statement should also be restricted.

Test Run Details

Local Build running on port 7777 - to avoid conflicts with product build listening on 8888
Test Env:

OS - Ubuntu 22.04
Browser - Opera as a neutral but very stable baseline browser - Opera 114.0.5

SFL = Submit on Focus Loss (Query Editor)
SRR = Submit automatic on Reload
SBC = Submit on Button Click
COTB-Resub = Change in other tab resubmit – means command already sent successfully, and is resubmitted after change in another tab. It is expected that restricted statements will not be resubmitted in this way. This also includes a check of the manual refresh button.

Universal Statements: EXPLAIN, SELECT, SHOW
Restricted Statements (Proposed): ALTER, CREATE, DELETE, DROP, GRANT, KILL, INSERT(?), REVOKE, SET(3 - below), UPDATE(?), USE

Universal statements can be submitted arbitrarily on focus change or page reload (to restore state of previous session).

Restricted statements should not be submitted arbitrarily through focus loss or on page reload. FAIL below indicates that such behavior occurred. Still the submit button should work.

Command Type	Tab 1 - SFL	Tab N - SFL	Tab 1 - SRR	Tab N - SRR	Tab 1 - SBC	Tab N - SBC	COTB-Resub
Universal - SELECT	PASS	PASS	PASS	PASS	PASS	PASS	PASS
Restricted - CREATE	PASS	PASS	PASS	PASS	PASS	PASS	PASS

Restricted Commands

Command	SFL	SRR	SBC
DROP	PASS	PASS	PASS
DELETE	PASS	PASS	PASS
ALTER	PASS	PASS	PASS
CREATE	PASS	PASS	PASS
UPDATE	N/A(1)	N/A	N/A
INSERT	PASS	PASS	PASS(2)
GRANT	PASS	PASS	PASS
REVOKE	PASS	PASS	PASS
USE	PASS	PASS	PASS
SET(3)	FAIL	FAIL	PASS

(1) UPDATE does not appear as a keyword in InfluxQL, however it has been added as a placeholder in the event of future changes.
(2) Test was of USE ephemeral; INSERT gopher,location=pond nuts=42i, so more a second test of the USE statement. INSERT works in CLI influx client, but is not supported in chronograf, which returns the error message error parsing query: found INSERT, expected SELECT, DELETE, SHOW, CREATE, DROP, EXPLAIN, GRANT, REVOKE, ALTER, SET, KILL at line 1, char 1. Indicated as PASS here, because behavior has not changed.
(3) SET can be used to set/change passwords e.g. SET PASSWORD FOR "my-user" = 'SuperSecretPW'. Undesired submits via chronograf could be an issue. Whether this statement should be restricted or not is still up for consideration, but not a blocker to closing this PR.

Universal Commands

Command	SFL	SRR	SBC
SELECT	PASS	PASS	PASS
SHOW	PASS	PASS	PASS

PR-6105-Testing241023-630d5211.csv

vlastahajek · 2024-10-30T13:34:46Z

The correct query state when returning to the resubmitted state has been fixed.
Also, a new issue with clicking on the Time refresh button in the Cell editor after submitting the query resulted in submitting it again was discovered and fixed.
However, the fix could be improved by properly propagating it to the query state hold in the DataExplorer and CellEditorOverlay

vlastahajek · 2024-11-15T16:54:12Z

PR was updated with better query state management. All issues should be resolved.
It's now read for the final review

karel-rehor · 2024-11-18T12:54:35Z

Test of Commit 630d521

Tested 2024.11.18

Recommendation

The SET statement should also be restricted.

Test Run Details

Local Build running on port 7777 - to avoid conflicts with product build listening on 8888
Test Env:

OS - Ubuntu 22.04
Browser - Opera as a neutral but very stable baseline browser - Opera 114.0.5
local build - Chronograf 1.10.5 (git: 953bfd4)

SFL = Submit on Focus Loss (Query Editor)
SRR = Submit automatic on Reload
SBC = Submit on Button Click
COTB-Resub = Change in other tab resubmit – means command already sent successfully, and is resubmitted after change in another tab. It is expected that restricted statements will not be resubmitted in this way. This also includes a check of the manual refresh button.

Universal Statements: EXPLAIN, SELECT, SHOW
Restricted Statements (Proposed): ALTER, CREATE, DELETE, DROP, GRANT, KILL, INSERT(?), REVOKE, SET(3 - below), UPDATE(?), USE

Universal statements can be submitted arbitrarily on focus change or page reload (to restore state of previous session).

Restricted statements should not be submitted arbitrarily through focus loss or on page reload. FAIL below indicates that such behavior occurred. Still the submit button should work.

Command Type	Tab 1 - SFL	Tab N - SFL	Tab 1 - SRR	Tab N - SRR	Tab 1 - SBC	Tab N - SBC	COTB-Resub
Universal - SELECT	PASS	PASS	PASS	PASS	PASS	PASS	PASS
Restricted - CREATE	PASS	PASS	PASS	PASS	PASS	PASS	PASS

Restricted Commands

Command	SFL	SRR	SBC
DROP	PASS	PASS	PASS
DELETE	PASS	PASS	PASS
ALTER	PASS	PASS	PASS
CREATE	PASS	PASS	PASS
UPDATE	N/A(1)	N/A	N/A
INSERT	PASS	PASS	PASS(2)
GRANT	PASS	PASS	PASS
REVOKE	PASS	PASS	PASS
USE	PASS	PASS	PASS
SET(3)	FAIL	FAIL	PASS

(1) UPDATE does not appear as a keyword in InfluxQL, however it has been added as a placeholder in the event of future changes.
(2) Test was of USE ephemeral; INSERT gopher,location=pond nuts=42i, so more a second test of the USE statement. INSERT works in CLI influx client, but is not supported in chronograf, which returns the error message error parsing query: found INSERT, expected SELECT, DELETE, SHOW, CREATE, DROP, EXPLAIN, GRANT, REVOKE, ALTER, SET, KILL at line 1, char 1. Indicated as PASS here, because behavior has not changed.
(3) SET can be used to set/change passwords e.g. SET PASSWORD FOR "my-user" = 'SuperSecretPW'. Undesired submits via chronograf could be an issue. Whether this statement should be restricted or not is still up for consideration, but not a blocker to closing this PR.

Universal Commands

Command	SFL	SRR	SBC
SELECT	PASS	PASS	PASS
SHOW	PASS	PASS	PASS
EXPLAIN	PASS	PASS	PASS

Addenda
Refresh button

Test Case	Result State	Notes
Universal command 1 click	OK - submitted	command: `SELECT * FROM ephemeral.autogen.cpu`
Universal command 2 click	OK - submitted	command: `SELECT * FROM ephemeral.autogen.cpu`
Restricted command 1 click	OK - not submitted	command: `CREATE DATABASE test2`
Restricted command 2 click	OK - not submitted	command: `CREATE DATABASE test2`

PR-6105-Testing241118-953bfd40.ods

karel-rehor

Can you take a look at the imports? I'm continuing reviewing the rest of the code.

ui/src/shared/apis/query.ts

ui/src/dashboards/components/InfluxQLEditor.tsx

ui/src/shared/components/TimeMachine/TimeMachine.tsx

ui/src/shared/utils/TimeMachineContainer.ts

vlastahajek · 2024-11-19T12:58:29Z

I changed relative paths to absolute.

karel-rehor

Manual tests pass. Changes make general sense. Good to go.

ui/src/shared/apis/query.ts

wip: preventing ddl and dml statements from autosubmiting

c95de3f

vlastahajek marked this pull request as draft September 12, 2024 16:00

bednar self-requested a review September 13, 2024 06:18

vlastahajek and others added 4 commits September 17, 2024 14:28

chore: prepare distinguishing tabs

5b3fb4a

fix: don't use deprecated image

61710bb

fix: distinguish submitting state between tabs

f122302

Merge branch 'master' into fix/auto-execution

5652be8

bednar changed the title ~~wip: preventing ddl and dml statements from autosubmiting~~ feat: preventing ddl and dml statements from autosubmiting Oct 9, 2024

bednar requested review from karel-rehor and removed request for bednar October 9, 2024 12:17

chore: more logs, better naming

42332aa

vlastahajek marked this pull request as ready for review October 9, 2024 12:25

fix: fixing manually submitting not sometimes doesn't work

99ec4c7

fix: invalid loading state, allow submitting

883ae6a

vlastahajek added 4 commits October 15, 2024 23:02

chore: better filter keywords set

5323644

fix: better manage states, keep unsubmited status

8808fee

chore: removed logs

d450d22

chore: eslint fixes

0580c59

vlastahajek added 2 commits October 17, 2024 14:07

fix: flux query doesn't have queryConfig

2711c1a

fix: filter excluded queries at higher level

e904a25

karel-rehor requested changes Oct 21, 2024

View reviewed changes

fix: Filtering at lower level to filter also manual refreshing

630d521

fix: Handling query state and auto-submitting refresh workaround

dcbaaaa

vlastahajek added 2 commits November 15, 2024 17:35

fix: Better submitting state handling

1d746a1

docs: Update Changelog

953bfd4

karel-rehor requested changes Nov 19, 2024

View reviewed changes

chore: Using absolute import path

b427ae3

karel-rehor approved these changes Nov 19, 2024

View reviewed changes

bednar reviewed Nov 20, 2024

View reviewed changes

ui/src/shared/apis/query.ts Show resolved Hide resolved

Merge branch 'master' into fix/auto-execution

fbaa580

bednar approved these changes Dec 4, 2024

View reviewed changes

vlastahajek merged commit 126af48 into master Dec 4, 2024
4 checks passed

vlastahajek deleted the fix/auto-execution branch December 4, 2024 08:23

BrewTestBot mentioned this pull request Dec 16, 2024

chronograf 1.10.6 Homebrew/homebrew-core#201345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preventing ddl and dml statements from autosubmiting #6105

feat: preventing ddl and dml statements from autosubmiting #6105

vlastahajek commented Sep 12, 2024 •

edited by bednar

Loading

vlastahajek commented Oct 9, 2024 •

edited

Loading

karel-rehor commented Oct 10, 2024

karel-rehor commented Oct 15, 2024 •

edited

Loading

karel-rehor commented Oct 16, 2024

karel-rehor commented Oct 21, 2024

karel-rehor left a comment

vlastahajek commented Oct 23, 2024

karel-rehor commented Oct 23, 2024 •

edited

Loading

vlastahajek commented Oct 30, 2024

vlastahajek commented Nov 15, 2024 •

edited

Loading

karel-rehor commented Nov 18, 2024 •

edited

Loading

karel-rehor left a comment

vlastahajek commented Nov 19, 2024

karel-rehor left a comment

feat: preventing ddl and dml statements from autosubmiting #6105

feat: preventing ddl and dml statements from autosubmiting #6105

Conversation

vlastahajek commented Sep 12, 2024 • edited by bednar Loading

vlastahajek commented Oct 9, 2024 • edited Loading

karel-rehor commented Oct 10, 2024

karel-rehor commented Oct 15, 2024 • edited Loading

karel-rehor commented Oct 16, 2024

karel-rehor commented Oct 21, 2024

karel-rehor left a comment

Choose a reason for hiding this comment

vlastahajek commented Oct 23, 2024

karel-rehor commented Oct 23, 2024 • edited Loading

vlastahajek commented Oct 30, 2024

vlastahajek commented Nov 15, 2024 • edited Loading

karel-rehor commented Nov 18, 2024 • edited Loading

karel-rehor left a comment

Choose a reason for hiding this comment

vlastahajek commented Nov 19, 2024

karel-rehor left a comment

Choose a reason for hiding this comment

vlastahajek commented Sep 12, 2024 •

edited by bednar

Loading

vlastahajek commented Oct 9, 2024 •

edited

Loading

karel-rehor commented Oct 15, 2024 •

edited

Loading

karel-rehor commented Oct 23, 2024 •

edited

Loading

vlastahajek commented Nov 15, 2024 •

edited

Loading

karel-rehor commented Nov 18, 2024 •

edited

Loading