An alternative to the web interface
Needs ReviewPublic

Authored by eje211 on Mar 5 2018, 8:34 PM.

Details

Summary

First of all, this is my first main contribution to an open-source project and my first main C++ project. (Also, as you will see, there is also some C in there.)This is a proposal for a replacement for the old Web Interface. This is at a very early stage but it already has basic features: it's possible to monitor and control torrents and it's possible to add torrents through magnet links.

The interface is smoother and uses more modern web features. It's based on Google's Angular 4. The source for the web interface itself it in another repo, eje211/ktorrent-web . The link to this repo is also in the source.

This is by no means feature-complete, and there are already a few kinks to be ironed out, but it sort of works and the code is (I think) at lot simpler than the old web interface.

I was not going to write a whole HTML server from scratch. All the C++ servers I saw gave me a lot of dependency problems, so I decided to just use Mongoose, a minimalist, pure C web server instead. If anyone has a better suggestion, let me know.

Of course, the final code will be fully commented. Currently, I'm still figuring out how the whole codebase will be organized. If there's a way for KTorrent to tell the web interface when it has information rather than for the web interface to ask at a regular interval, that would be a huge improvement. Just asking. The KTorrent interface is not really well documented.

You can reach me at eje211@gmail.com .

SVN_SILENT made messages (.desktop file) - always resolve ours

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"

First working version.

Diff Detail

Branch
htmlinterface
Lint
No Linters Available
Unit
No Unit Test Coverage
eje211 requested review of this revision.Mar 5 2018, 8:34 PM
eje211 created this revision.
eje211 retitled this revision from First of all, this is my first main contribution to an open-source project and my first main C++ project. (Also, as you will see, there is also some C in there.) This is a proposal for a replacement for the old Web Interface. This is at a very... to An alternative to the web interface.Mar 8 2018, 7:53 AM
eje211 edited the summary of this revision. (Show Details)
eje211 added a reviewer: stikonas.
eje211 added a subscriber: eje211.

I doubt I could be useful as a reviewer here bcs of lack of web/JS knowledge. Just wan't to say that in my opinion security evaluation should be prioritised. For a last few months I saw at least two news about hacker attacks on linux-based torrents via their web interfaces.

First one was transmission that could be used to execute attackers code on torrent's machine:
https://github.com/transmission/transmission/pull/468
Then same approach has been used to attack rTorrent and mine crypto currency:
https://f5.com/labs/articles/threat-intelligence/malware/rtorrent-client-exploited-in-the-wild-to-deploy-monero-crypto-miner
Analytics say attackers mine 40$ a day and already got 4k$
So would be great if one could make sure KTorrent doesn't already have such problem and won't become vulnerable at least to this kind of attack after this commit.

I doubt I could be useful as a reviewer here bcs of lack of web/JS knowledge. Just wan't to say that in my opinion security evaluation should be prioritised. For a last few months I saw at least two news about hacker attacks on linux-based torrents via their web interfaces.

First one was transmission that could be used to execute attackers code on torrent's machine:
https://github.com/transmission/transmission/pull/468
Then same approach has been used to attack rTorrent and mine crypto currency:
https://f5.com/labs/articles/threat-intelligence/malware/rtorrent-client-exploited-in-the-wild-to-deploy-monero-crypto-miner
Analytics say attackers mine 40$ a day and already got 4k$
So would be great if one could make sure KTorrent doesn't already have such problem and won't become vulnerable at least to this kind of attack after this commit.

I don't have enough knowledge about web/JS. Well, should try to find somebody in KDE who knows these things...

eje211 added a comment.Mar 8 2018, 4:21 PM

I'll look into those. You should know that I'm using an existing,
well-known web server library for this, though. But there's always a
possibility of a vulnerability in HOW I use it.

I added you as a reviewer because you seemed to be the main developer. I
don't know the team. Let me know who would be best to review this.

I'll look into those. You should know that I'm using an existing,
well-known web server library for this, though. But there's always a
possibility of a vulnerability in HOW I use it.

I added you as a reviewer because you seemed to be the main developer. I
don't know the team. Let me know who would be best to review this.

Well, I did work on porting away from KDELibs4Support and then made a release as there is no release manager for KTorrent (e.g. 5.0 was release by another guy).
I am not even sure if what KTorrent has can be called team. Just KDE developers occasionally contribute to KTorrent.

I think it's better to use existing web library like you said but it's still better if somebody with more knowledge looks at it. I'll probably try to test it later too,
but not right now, maybe in 3 weeks...

pino added a comment.Mar 10 2018, 11:41 AM

Not sure why I was added for this review, since I'm not a ktorrent developer...

Anyway, since I'm added, I can provide some general feedback (not on the web part though, as I'm not a web developer):

  • please do not add translations to .desktop and .notifyrc files: there is an automated KDE service that handles them from/to PO files
  • there is lots of commented code in the added files: please remove it if it is not needed, otherwise it is just clutter
  • is the angularjs favicon.ico really needed? if so, please at least use the ktorrent icon (building the .ico file from the .png sources)
  • html/source.txt says that the sources are at https://github.com/eje211/ktorrent-web, which makes shipping only the generated files a license issue (since there is no preferred form of modification); also, the files in ktorrent-web have no license specified (big red issue)
  • the plugin embeds a copy of the Mongoose library: https://github.com/cesanta/mongoose -- is there no other library for this, that can be used as system library (i.e. without copying it statically in the sources)? ktorrent is not much actively developed, so the big risk here is that the embedded copy would just rot (like already happening with the copy of GeoIP.c); also Mongoose is GPL-2only, which is a long-term licensing issue (since it then forces the license of the resulting work to be GPL-2only)
  • what @trufanov and @stikonas already said regarding the usage of a web framework

Also, I posted various reviews of the actual C++ code used.

plugins/htmlinterface/torrentlistgenerator.cpp
58

QJsonArray array; is enough (also for other variables in this file).

88–89

This converts the document to QByteArray twice.
Also, when the scope of this block ends, the data to which *str_p points to becomes a dangling pointer.
Why not just return the QByteArray?

92

Since it does not modify its argument, make the json parameter of post as const char * (and remove the cast later on).

98

An null QString is also empty, so just check for isEmpty (also in other places in this file).

117–129

Chain the various if's together, so not all the checks will be performed every time:

if (action == QLatin1String("start")) {
  ...
} else if (action == QLatin1String("pause")) {
  ...
} ...
140

hash is leaked. Just return it by value in QStringToSHA1Hash.

155–181

Instead of shamelessly copying code from stackoverflow, use at least code already written in libktorrent: in src/dht/tests/keytest.cpp, see HexCharToUint8 (usable as-is) and KeyFromHexString (mostly similar).

plugins/htmlinterface/torrentlistgenerator.h
55–58

No need for these functions to be exposed, just make them static inside the .cpp file.

plugins/htmlinterface/webserver.cpp
32

Twice?

52

The hardcoded port is a no-go.

54–56

Forever loop? How does it exit cleanly, then?

65

Hardcoded path.

69–71

method is leaked; considering its only use is to compare it to "GET", then just use memcmp with message->method.len instead.

73–75

Ditto (same as method for uri).

80

size is not used. Considering my suggestion earlier regarding the return value of TorrentListGenerator::get, then that would solve this issue too.

88–90

body is leaked, and it is even used only in a specific situation (so no need to create it always).
Also, IMHO it would be much better to wrap the body as QByteArray, using fromRawData to avoid copying it, and pass that to TorrentListGenerator::post.

96

len is not used.

pino requested changes to this revision.Mar 10 2018, 11:42 AM
This revision now requires changes to proceed.Mar 10 2018, 11:42 AM
eje211 added a comment.EditedMar 11 2018, 5:12 AM
In D11070#222461, @pino wrote:

Not sure why I was added for this review, since I'm not a ktorrent developer...

Anyway, since I'm added, I can provide some general feedback (not on the web part though, as I'm not a web developer):

  • please do not add translations to .desktop and .notifyrc files: there is an automated KDE service that handles them from/to PO files
  • there is lots of commented code in the added files: please remove it if it is not needed, otherwise it is just clutter
  • is the angularjs favicon.ico really needed? if so, please at least use the ktorrent icon (building the .ico file from the .png sources)
  • html/source.txt says that the sources are at https://github.com/eje211/ktorrent-web, which makes shipping only the generated files a license issue (since there is no preferred form of modification); also, the files in ktorrent-web have no license specified (big red issue)
  • the plugin embeds a copy of the Mongoose library: https://github.com/cesanta/mongoose -- is there no other library for this, that can be used as system library (i.e. without copying it statically in the sources)? ktorrent is not much actively developed, so the big risk here is that the embedded copy would just rot (like already happening with the copy of GeoIP.c); also Mongoose is GPL-2only, which is a long-term licensing issue (since it then forces the license of the resulting work to be GPL-2only)
  • what @trufanov and @stikonas already said regarding the usage of a web framework

    Also, I posted various reviews of the actual C++ code used.

Thanks for all the comments. This is really a first draft and it's far from finished. The hard-coded port, the favicon, all of that is temporary. I just wanted to get a feel of whether C++ was up to the task. And, honestly, it's not nearly as bad as I had feared. But there were things I just didn't know and I had to ask and posting code was the only way I knew how. To answer your comments:

I have no particular preference for Mongoose; it "just worked." I'm ready to offer two alternatives: libmicrohttpd https://www.gnu.org/software/libmicrohttpd/, which is a C library that's available as a dependency in Debian and Ubuntu and Pion https://github.com/splunk/pion, which is a C++ library also available in Debian and Ubuntu. Keep in mind that Pion requires Boost. I don't know if that's a problem. Both are LGPL 2. Is there a process to know which distributions should make the libraries available? Is either of these alternatives acceptable? Unlike Mongoose, neither provides websockets, but for now, I'm not using websockets, so it should be okay.

I'm not sure I understood what you mean "about web frameworks." It's usually the preferred way of doing a web app. In terms of security, I looked at the articles, and I'm sure that neither of these exploits are possible here. I'm adding extra security in the next patch, but I'm not completely sure what I'm securing against. This plugin only allows very restricted operations to be performed. Should the source of the Angular app be included in a subdirectory of the plugin? If not, where should it go? Compiling the Angular app is a completely different process from compiling C++. It requires node.js, not CMake and G++.

I'll address your notes and several other revisions in a new patch soon. There were other big problems with this version, but I need your feedback for things just like those: the licensing issues, the static libraries and the inclusion of the web framework. I know the code is far from ready but I thought it was better to get feedback early. Thank you for all of your help!

Regarding HTTP libraries, it seems to me that libmicrohttpd has more activity. There is even a github repo with HTTP2 implementation. And it is also much better packaged in distributions. I think using boost for pion is OK if you choose pion, some KDE dependencies use boost.

I think cmake should be able to call nodejs and build html/js files for angular.

eje211 updated this revision to Diff 29883.Mar 19 2018, 6:20 AM

Several of the concerns in the previous reviews have been addressed. Most
notably, the Mongoose web server has been replaced by the GNU's libmicrohttpd,
which is available as a shared library.

There is also a basic security feature which can be improved later: any
message that makes a change in KTorrent requires a verificacion key. One thing
that could make this system safer could be to randomize that key from time
to time.

One problem I could not crack is that adding a mangnet link does not work as
expected. The code that adds magnet links is the same as the callback from
the code that adds magnet links from the callback menus, but when my plugin
tries to add a magnet link, they just appear in the "Magnet Downloads" panel
and they there with zero peers. I have no idea why.

eje211 updated this revision to Diff 29884.Mar 19 2018, 6:43 AM
  • Still trying to fix the desktop and notfiyrc issue.
eje211 updated this revision to Diff 29885.Mar 19 2018, 6:47 AM

O - Still trying to fix the desktop and notfiyrc issue.

pino added a comment.Mar 19 2018, 7:14 AM

Note that correct way is:

  • squash (i.e. "merge") all the commits in your local htmlinterface branch into a single commit
  • rebase your branch on current master
  • push the single commit as update to this revision

All the revisions of your work are not useful in case of the final merge, since they carry copy of 3rdparty sources, experiments, commented stuff, etc, which would make things like bisecting and debugging much harder.

True. I'll do it once I've fixed the desktop problem. I can't find what the
base commit was, so I just gave up and I'm just replacing the wrong lines
by hand. I know it's stupid but all the commits I've tried have turned out
to be wrong.

eje211 updated this revision to Diff 29886.Mar 19 2018, 7:30 AM

Squashed commits.

eje211 updated this revision to Diff 29887.Mar 19 2018, 7:32 AM

Squashed commits.

eje211 updated this revision to Diff 29888.Mar 19 2018, 7:35 AM

Squashed commits.

pino added a comment.Mar 19 2018, 7:37 AM

True. I'll do it once I've fixed the desktop problem.

This can be fixed while rebasing on master.

I can't find what the base commit was

git merge-base branch1 branch2

I'm just replacing the wrong lines by hand.

No need to do that, this can be fixed easily -- try the follow steps (feel free to do a backup before):

  1. git checkout htmlinterface -- switch to the local htmlinterface branch
  2. make sure everything is clean: commit fixes that that need to be committed, or stash them away for now
  3. git merge-base master htmlinterface -- take the base commit of htmlinterface
  4. git rebase -i BASE..htmlinterface -- just rework your local branch without actually rebasing it: here, in all the commits after the first select fixup as action, so they will be all squashed into the first commit
  5. git checkout master && git pull -- switch to master, and update it
  6. git checkout htmlinterface -- back to htmlinterface
  7. git rebase -i origin/master -- rebase htmlinterface on top of uptodate master: here, solve the conflicts with desktop/etc files with git checkout --ours FILE + git add FILE (--ours because we want the versions from master`)
  8. fining the rebase of (6)

Now check that your branch does not have extra changes anymore:

  1. git diff origin/master..htmlinterface | filterdiff -i '*.desktop' | patch -R -p1 -F3`
  2. if after (1) there are changed files, then git add them, followed by git commit --amend -v

BTW, your plugin lacks a desktop file.

eje211 updated this revision to Diff 29893.Mar 19 2018, 9:21 AM
  • SVN_SILENT made messages (.desktop file) - always resolve ours
  • Merge branch '5.1'
  • SVN_SILENT made messages (.desktop file) - always resolve ours
eje211 updated this revision to Diff 29895.Mar 19 2018, 9:25 AM
  • SVN_SILENT made messages (.desktop file) - always resolve ours
eje211 updated this revision to Diff 29898.Mar 19 2018, 9:41 AM
  • Merge branch 'htmlinterface' of github.com:eje211/ktorrent into htmlinterface
eje211 updated this revision to Diff 29899.Mar 19 2018, 9:58 AM
  • Merge branch 'htmlinterface' of github.com:eje211/ktorrent into htmlinterface
stikonas requested changes to this revision.Mar 19 2018, 10:26 AM
stikonas added inline comments.
plugins/logviewer/ktorrent_logviewer.desktop
3

I think you mismerged something.

This revision now requires changes to proceed.Mar 19 2018, 10:26 AM

One problem I could not crack is that adding a mangnet link does not work as
expected. The code that adds magnet links is the same as the callback from
the code that adds magnet links from the callback menus, but when my plugin
tries to add a magnet link, they just appear in the "Magnet Downloads" panel
and they there with zero peers. I have no idea why.

I would compare this behaviour with magnet links added manually via UI. If they have 0 peers too then it's fine.
The root of this problem is that KTorrent have no DHT Bootstrapping implemented. That's why it may take hours before it finds any peers and, for example, qBittorrent needs minutes for that.

There was a pull request adding DHT Bootstrapping in old KDE review board: https://git.reviewboard.kde.org/r/130083/
but it require a bit more efforts from his author to get into codebase.

If I add the same torrent through the UI, it takes seconds, most of the
time, not minutes. There's more: if I add a torrent from the plugin, the
magnet downloader stays at zero peers (unlike with the UI), if I stop and
restart the magnet download, the number of peers goes up but the magnet
still does not download. If I delete the magnet link and add it though the
UI, it downloads find, often in second. Keep in mind that all of this is
called from anther thread. I don't see why it would be a problem but maybe
that's the cause of the issue.

eje211 updated this revision to Diff 30061.Mar 20 2018, 10:30 PM
  • Merge branch 'htmlinterface' of github.com:eje211/ktorrent into htmlinterface