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

Allow full search on devices page #8364

Merged
merged 9 commits into from Mar 25, 2018

Conversation

Projects
None yet
5 participants
@MrMaus13
Contributor

MrMaus13 commented Mar 13, 2018

Allows you to search all columns on the devices overview page instead of only the 'hostname' column.
Useful for searching locations, system names, ip addresses, hardware types, etc.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

MrMaus13 added some commits Mar 13, 2018

@laf

I'm fine with this, it does obviously change the default behaviour which might not be what people expect but the only way round that is to have an input box for devices which means more room taken up in the search bar.

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Mar 13, 2018

is there any kind of library which allows for an intelligen search a la:

os:linux and host:foobar and not location:London*

and so on....

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

Should just need _POST replacing with vars to be safe.

@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Mar 13, 2018

Ok, I changed it to vars, which seems to work fine.
How do 'vars' work? As i don't see them available anywhere?
Anyway, should i change all $_POST references with vars in the "devices.inc.php" file? Since they are really used all over the place.

If so, let me know, I'll submit a fix.

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

So $vars is what we set for data processed via _POST and _GET but we run some sanity checks over it. If you can fix all _POST use that would be ace.

Replace $_POST with $vars
Better protection for SQL injection attempts; Need to verify other files for same issue.
@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Mar 13, 2018

Ok, done. There are a lot of files with this issue as well. I'll take a look at them when i have some more time to spare.

MrMaus13 added some commits Mar 13, 2018

Fixed whitespace.
*sigh*
More search options & sql injection fixes.
+Allow full search on devices page;
+Allow sysName search on alertlog page;
+Allow sysName search on alerts page;
+Allow sysName search on eventlog page;
+Allow sysName search on poll-log page;
+Allow sysName search on ports page;

*Replaced all occurrences of $_POST with $vars in librenms/html/includes/table. ($vars are sanity-checked).
@murrant

This comment has been minimized.

Member

murrant commented Mar 19, 2018

I disagree with using $vars, I would prefer $_REQUEST if we need to allow both post and get.

I know that is what a lot of the code is now, but PHP already does this for us.

@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Mar 19, 2018

That would defy the point and still leave you with insecure and injectable code; $_REQUEST is just as insecure as $_POST and $_GET; PHP does not sanitize the input.

From PHP Manual:

Note:
The variables in $_REQUEST are provided to the script via the GET, POST, and COOKIE input mechanisms and therefore could be modified by the remote user and cannot be trusted. The presence and order of variables listed in this array is defined according to the PHP variables_order configuration directive.

The reason for using $vars is not because we have to allow both post and get, it's because the input is sanitized in html/includes/vars.inc.php.

Makes it harder for a guest to drop a table using sql injection. There is a lot more code not checking on sanity of the input but for my change it made sense to at least change the files in the html/includes/table directory to be more secure.

@murrant

This comment has been minimized.

Member

murrant commented Mar 19, 2018

Good point. I'm used to sanitizing the input at a different level. Proceed.

@laf

I'm fine with all this except the one change you need to go back to.

$where .= ' AND `D`.`hostname` LIKE ?';
$param[] = '%' . $_POST['hostname'] . '%';
if (!empty($vars['hostname'])) {
$where .= ' AND (D.hostname LIKE "%i'.$vars['hostname'].'%" OR D.sysName LIKE "%'.$vars['hostname'].'%")';

This comment has been minimized.

@laf

laf Mar 19, 2018

Member

You need to put this back to $where and $param

This comment has been minimized.

@MrMaus13

MrMaus13 Mar 19, 2018

Contributor

Ok I'll fix that;
Do you want the same for devices then?
Because it's (almost) the same query.

This comment has been minimized.

@laf

laf Mar 19, 2018

Member

Yeah I saw that, you can fix if you like but it's not necessary. Just don't regress the code :)

MrMaus13 added some commits Mar 22, 2018

@murrant

This comment has been minimized.

Member

murrant commented Mar 22, 2018

I wonder how this will perform with many devices? (say 10,000)

@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Mar 22, 2018

This one takes about 0.17sec, if you have any other queries to try, let me know.

MariaDB [librenms]> select count(*) from devices;
+----------+
| count(*) |
+----------+
|    51174 |
+----------+
1 row in set (0.01 sec)


MariaDB [librenms]> SELECT os FROM devices WHERE hostname LIKE "%rtm-vnn%" OR sysName LIKE "%rtm-vnn%" OR hardware LIKE "%rtm-vnn%" OR os LIKE "%rtm-vnn%" OR location like "%rtm-vnn%" GROUP BY os;
+---------+
| os      |
+---------+
| ciscosb |
| ios     |
| iosxe   |
+---------+
3 rows in set (0.17 sec)

MariaDB [librenms]> 
Whitespaces....
Sometimes you want'em, sometimes you hate'em.
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 22, 2018

The inspection completed: No new issues

@murrant

LGTM

@laf

laf approved these changes Mar 25, 2018

@laf laf merged commit 9f5b42b into librenms:master Mar 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Apr 3, 2018

This commit removes the ability to only see graphs for a subset of devices.

For instance, I like to search for 'esx' when looking at servers, and then hit 'Memory' to illustrate how the ESXes are doing in that department. Prior to this commit, I would get graphs for all hosts with 'esx' in the hostname. After the commit I get graphs for all devices of type server.

@laf

This comment has been minimized.

Member

laf commented Apr 3, 2018

@MrMaus13 I think it's because

if ($format == "graph") {
section needs updating to use the new searchquery param

@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Apr 3, 2018

Right, I see, never thought of that.
I'll fix that this week, sorry @dagbdagb :-)

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Apr 3, 2018

No worries.
Move fast, break often, break early.

@laf

This comment has been minimized.

Member

laf commented Apr 4, 2018

Thanks @MrMaus13

@MrMaus13

This comment has been minimized.

Contributor

MrMaus13 commented Apr 5, 2018

Should be fixed: #8509

@laf

This comment has been minimized.

Member

laf commented Apr 5, 2018

@MrMaus13 Thanks for fixing :)

@dagbdagb update and all should be well.

@dagbdagb

This comment has been minimized.

Contributor

dagbdagb commented Apr 6, 2018

Looks good! Thanks!

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

webui: Allow full search on devices page (librenms#8364)
* Update devices.inc.php

* Update devices.inc.php

* Replace $_POST with $vars

Better protection for SQL injection attempts; Need to verify other files for same issue.

* Fixed whitespace.

*sigh*

* More search options & sql injection fixes.

+Allow full search on devices page;
+Allow sysName search on alertlog page;
+Allow sysName search on alerts page;
+Allow sysName search on eventlog page;
+Allow sysName search on poll-log page;
+Allow sysName search on ports page;

*Replaced all occurrences of $_POST with $vars in librenms/html/includes/table. ($vars are sanity-checked).

* Whitespace fix

* Fixed $where & $param

* Add files via upload

* Whitespaces....

Sometimes you want'em, sometimes you hate'em.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018

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