WIP Add a global test for insecure http: URLs used in code or documentation
Needs ReviewPublic

Authored by vkrause on Mar 23 2019, 1:33 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Maniphest Tasks
T10716: Add check for accidental http: usage
Summary

This is supposed to trigger a unit test failure when using http: rather
than https: URLs. This is obviously imperfect, so it has support for
module or line specific overrides. This should also not get enabled by
default as long as this basically triggers everywhere.

The Python script is from Sandro, the CMake integration and URI blacklist
from me.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
arcpatch-D19996
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10005
Build 10023: arc lint + arc unit
vkrause created this revision.Mar 23 2019, 1:33 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptMar 23 2019, 1:33 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
vkrause requested review of this revision.Mar 23 2019, 1:33 PM
krop added a subscriber: krop.Mar 23 2019, 1:46 PM
krop added inline comments.
kde-modules/KDECMakeSettings.cmake
189

FindPython3.cmake only exists in CMake >= 3.12.0

knauss updated this revision to Diff 54612.Mar 23 2019, 1:48 PM
knauss added a subscriber: knauss.

update python script.

knauss updated this revision to Diff 54613.Mar 23 2019, 1:51 PM

update http regex

knauss updated this revision to Diff 54615.Mar 23 2019, 1:58 PM

add more excludes.

vkrause added inline comments.Mar 23 2019, 1:58 PM
kde-modules/KDECMakeSettings.cmake
189

Right, the feature is just silently ignored in older versions. We can probably switch that to PythonIterp if we want this for older versions as well. But let's first see if this approach actually works and if people will access getting that test forced upon them :)

krop added inline comments.Mar 23 2019, 2:04 PM
kde-modules/KDECMakeSettings.cmake
189

PythonInterp is also not a solution :) See 6c1db934e in ki18n.

CMake 3.5.0 was found acceptable for most frameworks, it would be nice to make it work with this version.

knauss updated this revision to Diff 54617.Mar 23 2019, 2:07 PM

search for http://\S

and update blacklist.

vkrause added inline comments.Mar 23 2019, 3:32 PM
kde-modules/KDECMakeSettings.cmake
189

ugh, I see... so how could we solve this here? run find_package in the ecm cmakelists.txt and configure_file the python exe path into a wrapper for the python script?

this would be a nice addition to Krazy. on my todo list.

this would be a nice addition to Krazy. on my todo list.

we are actually planing to push the list of blacklisted matches to a single file, so other scripts can take the list too.

knauss updated this revision to Diff 54639.Mar 23 2019, 10:14 PM

add httpupdate and split out blacklist file

knauss updated this revision to Diff 54640.Mar 23 2019, 10:17 PM

remove BLACKLSIT for httpcheck

knauss updated this revision to Diff 54662.Mar 24 2019, 12:19 PM

now with glob matching and another round of renaming blacklist files.

knauss updated this revision to Diff 54663.Mar 24 2019, 12:31 PM

complete diff.

knauss updated this revision to Diff 54679.Mar 24 2019, 2:29 PM

make parallel network requests.

knauss updated this revision to Diff 54680.Mar 24 2019, 3:11 PM

fixing a typo.

Any chance this could not be done by abusing KDECMakeSettings.cmake as injection vector? I know you are just following the example of what was done for appstreamcli, but IMHO this has already been a bad hack, screwing over the fine granular design of all the ECM modules trying to keep aspects separate. And yes, by the price of the overhead with more explicit module includes, but it's like that. Or we should just screw it and put everything in one big "KDEECMEverythingEvenKitchenSink.cmake" ;) And yes, one possible would like to have such a generic wrapper module in any case, for quick prototyping. But the individual modules should stay focussed.

The docs of KDECMakeSettings say: "Changes various CMake settings to what the KDE community views as more sensible defaults."
Thus adding automatic tests or macros for tests using external tools would not in the scope of this very module.

Any chance this could not be done by abusing KDECMakeSettings.cmake as injection vector?

I completely agree that this is a rather hacky approach. IMHO the challenge here is finding an approach that gives us very wide coverage. That's why I'm not too happy with e.g. an opt-in approach where we have to enable this per repo, even if that would be a lot cleaner from the ECM POV.

It however does not need to be ECM based at all, an alternative approach might be an EBN-like service or dedicated CI job scanning all our repos for this. That would have an even wider coverage (e.g. websites and translations), but it would somewhat decouple results from development. Failing unit tests both locally and on the CI are just jumping at you much more than yet another static analysis result site.

FYI: Today I added a Krazy checker to do this. Should see results on the EBN in a day or 2.

Although I am skipping the .htignore's, there will still be lots of false positives especially in the test programs.
Let's see what happens.
-Allen