Closed alexAubin closed 4 years ago
Here are some results of the new package linter (c.f. original post)
For now I split the nginx path traversal issue detection in a separate column (even though the current code report it as an error)
So: if we do not report nginx traversal issue as error for now only rss-bridge and portainer will drop to level 4 (though there's a pending PR for rss-bridge if I remember correctly)
However, we should consider reporting this nginx path traversal issue as an actual issue considering many apps are affected and not getting fixed ...
Note that the current CI can miss some of them for example, for transmission/ ... because the nginx issue found is about transmission/downloads, but the CI only test the root of the app ...
Any thoughts on this specific item, and the PR in general, @YunoHost/apps folks ?
However, we should consider reporting this nginx path traversal issue as an actual issue considering many apps are affected and not getting fixed ...
This is already tested by the CI, I don't know how do you test it, but rather than a syntax check I do prefer an actual test. Otherwise looks good. Hope there's less false positive.
This is already tested by the CI, I don't know how do you test it, but rather than a syntax check I do prefer an actual test.
It's not a syntax check, it's an actual parsing of the code ... Of course considering that there are placeholder variables that may still get replaced with something with/without a trailing slash, that's still not 100% accurate and has to be confirmed by an actual test.
But the test on the CI isn't 100% accurate either and has false-positive. As said, it doesn't cover other locations inside the nginx conf (c.f. transmission/downloads) and it assumes that the alias
folder is going to be in /var/www/
which is not always the case.
So with that in mind to me the two approaches are simply complementary...
(Edit: note also that the current code of package_check doesn't cap the level of apps that are vulnerable ... so for example we have wemawema being level 7 and in fact vulnerable to this)
Indeed, the level is not set lower because of this error, at the time we add this check, we wasn't sure. Could be modified.
Regarding linter, a warning, but not an error before we're sure there's no false positive with it. Otherwise apps will finish with the level 5 forced, which is worse...
Well yolo, let's merge as is
I changed the nginx path traversal issue to still be a warning ... but it's been a warning for a long time (in the linter and the CI) and people ain't necessarily fixing it, so we gotta consider making it an error at some point .. and/or make some PR on the repo ... (at least for apps level 5 to 8)
Let's see if anybody's screaming about the other changes for now.
yunohost app setting,initdb,checkport,checkurl
and report them as error. From what I tested so far, should not affect any app except maybe rss-bridge but there's a PR on the way.