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

[WIP] Added lint. #1788

Merged
merged 10 commits into from Jul 18, 2015

Conversation

Projects
None yet
3 participants
@udan11
Member

udan11 commented Jul 13, 2015

Added lint.

Signed-off-by: Dan Ungureanu udan1107@gmail.com

Work in progress.

  • Basic support.
  • Add configuration entries.
  • Write more strict rules for parsing depending on context (see udan11/sql-parser#2).
  • Reorganize lint.php.



@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 15, 2015

Interesting feature. I tested typing errors in the data types and fixing them. I fear about the performance (calling a script almost once per character typed).

Also, something curious: if I type CREATE TBLE then continue with my definition, I have an error below on my VARCHAR not being a correct statement. I guess that this will be fixed with more parsing rules.

@lem9 lem9 self-assigned this Jul 15, 2015

@udan11

This comment has been minimized.

Member

udan11 commented Jul 15, 2015

The accuracy will improve when more rules will be written. I already wrote a couple of new ones and I'll commit the new library to phpMyAdmin soon.

Regarding performance, CodeMirror will make a new request only when you pause typing. For example, if you were to type continuously and only stop when the query is finished, only one request would be made. Also, long queries aren't analyzed.

I guess that I could set a timer to send a request only a couple of seconds (maybe 10) and only if the contents changed. Would that be better?

udan11 added some commits Jul 13, 2015

Added lint.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
Updated sql-parser library to udan11/sql-parser@56ce2d7.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
Style improvements.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
Reorganized code.
Fixed a bug that miscalculated the position of the tokens.
Added tests.

Signed-off-by: Dan Ungureanu <udan1107@gmail.com>

@udan11 udan11 force-pushed the udan11:lint branch from ce0675a to 024d967 Jul 15, 2015

Fixed a few of Scrutinizer's issue.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 15, 2015

I was not typing fast enough in my test! So I withdraw my comment about performance.

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 16, 2015

Please use the normal token protection inside lint.php; we don't want XSS via this script.

@lem9

This comment has been minimized.

lem9 commented on libraries/Linter.class.php in 024d967 Jul 16, 2015

We normally use this instead of strlen(), any reason not to do it here?

/*overload*/mb_strlen

This comment has been minimized.

Owner

udan11 replied Jul 16, 2015

mb_strlen would return the number of characters, but in this case the number of bytes is required.

If I were to use mb_strlen, the following situation may occur: given the following string: ????+, where ? represents a multi-byte character (lets assume that every ? is a 2-byte character) and + is a newline, the first value of $i is 0 and the last one is 4 (because there are 5 characters). Bytes $str[0] and $str[1] are the first character, $str[2] and $str[3] are the second one and $str[4] is going to be the first byte of the third character. The fourth and the last one (which is actually a new line) aren't going to be processed at all.

I could have used the SqlParser\UtfString class, but it causes unnecessary overhead.

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 16, 2015

Not sure if it's because you're using strlen() instead of mb_strlen(), but I can generate a js error by typing

select from école
Updated sql-parser library to udan11/sql-parser@9e3a9ee.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
@udan11

This comment has been minimized.

Member

udan11 commented Jul 16, 2015

I believe there is absolutely no vulnerability in this script. All it does is to accept one user input, parse it and send back a JSON response. Before displaying the result to the user, the response is escaped and any tag that may cause issues is escaped. I'll look into it.

Also, I fixed the issue with UTF-8 strings.

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 16, 2015

About strlen(), see 66b4be9 and look at my comments. Can you use mb_strlen() with the '8bit' parameter?

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 16, 2015

If you believe there is no vulnerability, please indicate your reasoning as comments at the beginning of lint.php to document why there is no token protection.

udan11 added some commits Jul 17, 2015

Use mb_strlen to match the other code base.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
Do not process unauthorized requests.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
Updated sql-parser library to udan11/sql-parser@8835414.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 18, 2015

Changes are fine; awaiting completion of the WIP.

Updated sql-parser library to udan11/sql-parser@e713c9f.
Signed-off-by: Dan Ungureanu <udan1107@gmail.com>
@udan11

This comment has been minimized.

Member

udan11 commented Jul 18, 2015

I believe this is ready to be merged. The rules have been improved significantly and further updates will come in the future versions of the library.

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 18, 2015

Nice work. I'm merging but I hope you are aware that some mistakes like "WHRE" are not detected. Also, looks like the linter does not have effect when typing in the console?

lem9 added a commit that referenced this pull request Jul 18, 2015

Merge pull request #1788 from udan11/lint
[WIP] Added lint.

@lem9 lem9 merged commit e4d4349 into phpmyadmin:master Jul 18, 2015

2 of 3 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
Scrutinizer 6 new issues, 12 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@udan11

This comment has been minimized.

Member

udan11 commented Jul 18, 2015

"WHRE" might not be a mistake. The following query: "SELECT * FROM actor WHRE" is valid. WHRE is an alias for actor.

@lem9

This comment has been minimized.

Contributor

lem9 commented Jul 19, 2015

True about the alias case. Now, what about typing in the console?

@udan11

This comment has been minimized.

Member

udan11 commented Jul 19, 2015

I'll take care of that. :)

@udan11 udan11 deleted the udan11:lint branch Jul 20, 2015

@madhuracj

This comment has been minimized.

Member

madhuracj commented on f0667aa Jul 27, 2015

@udan11
Please have a look at #11325 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment