Banning QNetworkAccessManager

[Ben Cooksley]

On Mon, Feb 3, 2020 at 7:42 AM Volker Krause <vkrause at kde.org> wrote:
>
> I agree on the problem of QNAM's default, see also https://conf.kde.org/en/
> akademy2019/public/events/135 on that subject.
>
> On Saturday, 1 February 2020 23:24:14 CET Ben Cooksley wrote:
> [...]
> > Prior to now, i've taken the approach of advertising that
> > QNetworkAccessManager is broken and needs a flag set within
> > applications when used, however unfortunately this seems to have been
> > an ineffective approach and cases of broken behaviour continue to
> > appear (despite this brokeness being known about and advertised since
> > back in the Qt 4.x series when this class was introduced)
> >
> > Therefore, given the Qt Project is unlikely to change their position
> > on this, I would like to propose the following:
>
> The Qt Project is actually in favor of changing at least the redirection
> default to exactly what we want it to be.
> https://codereview.qt-project.org/c/qt/qtbase/+/273175 has the change, and got
> approval both by Lars and one of the maintainers. It is merely stuck in
> dealing with the unit test fallout in Qt's CI that somebody has to take care
> of. Doesn't immediately help us of course, but claiming Qt is unwilling to
> change this is simply wrong.

Last I heard they were unwilling to change the default, so it's nice
to know they're looking at revisiting their error.

Back when this issue originally developed back in the Qt 4.x era (just
a couple of releases after QNAM appeared) the Qt Developers at the
time not only refused to change the default, but also refused to
consider adding any kind of convenience function. They considered that
their position was the most correct one and that following redirects
simply wasn't required (as it isn't a MUST in the RFCs).

It was later on (around Qt 5.7 I think) that a set of functions and
enums were added to Qt to make it convenient to change the behaviour.
When these were initially introduced the behaviour was changed to
follow redirects, however that was then overturned before it was
released (on the basis of an absolute need for "behavioural
compatibility", something that wasn't an issue when the change went
through initially, so my suspicion is a customer of Digia at the time
threw their toys)

I wasn't involved directly in the conversations in the Qt 5.7
timeframe, so don't have a link to that i'm afraid. The Qt 4.x one was
a conversation on IRC.

While that merge request is promising, it unfortunately is targeted at
the 'dev' branch, which if i'm not wrong is Qt 6.x, so it will be some
time before we get to see the benefit of that (more on that below).

>
> > 1) That effective immediately, QNetworkAccessManager and it's children
> > classes be banned and prohibited within KDE software, to be enforced
> > by server side hooks.
> > 2) That we fork QNetworkAccessManager and the associated classes
> > within the appropriate Framework (to be determined), where the
> > defective behaviour can then be corrected.
>
> While this might solve the redirection problem, it brings us yet another then
> unmaintained HTTP implementation next to the one in KIO, which is a far bigger
> problem long term. We need less of those, not more.
>

It is not ideal I agree.
What I would like to ensure however is that we are not impacted in the
future by any policy decisions made by the Qt Developers in respect of
this set of classes.

> To address the problem of people not using the correct defaults, I did propose
> a much less extreme approach in the above mentioned talk at Akademy, namely
> having a KNetworkAccessManager as a sub-class of QNAM in a low-tier frameworks
> that essentially just enables redirects and HSTS. QNAM's implementation isn't
> the problem after all, the defaults are.
>

This may work, but we would need to force people to move to using the
wrapper, and it still leaves us exposed to anywhere that retrieves a
QNAM constructed by code located elsewhere.

> Commit hooks warning about QNAM usage might still be a good idea, but as a
> warning only. Hard blocking QNAM-using code would block at least three repos I
> spend a lot of time on currently, none of which even talks to KDE servers.
>
> If you need to take more drastic measures against code not doing this
> correctly regardless, you can do that my dropping the server-side workarounds.
> Yes, that would break the affected application possibly, but at least it would
> not cause massive collateral damage for the people using this correctly.
>

Nicolas did a spot check of some of the places where QNAM is used in
KDE, and at first glance it appears that not a single one enabled the
following of redirects :(
It is likely that very few places handle this correctly (probably only
those who have interacted with Sysadmin on the matter)

> It would also help to know where specifically we have that problem, so we can
> actually solve it, and so we can figure out why we failed to fix this there
> earlier.

All of the places where we have actively hit this issue have already
been fixed (Marble and Attica come immediately to mind, not sure if
GCompris fixed their code).

The rest continue to be sleeping timebombs which we will only discover
when we change something on the server side and put in place a
redirect. They may never be triggered, or they could be triggered next
week. Even if we fix the code now, due to LTS distributions and people
using software for far longer than they should, it will take years for
the fixes to fully reach user systems.

To illustrate how long this takes, Marble moved from using
http://files.kde.org/marble/maps/ to https://maps.kde.org/ back in
January 2016. Four years later, we still get 13,000 hits to paths
under the old URL every day. The version numbers sent by Marble as
part of these requests indicate that some of them are from the version
released with KDE 4.14 which was released back in August 2014
(fortunately this code path was fixed to follow redirects prior to
that release)

>
> Regards,
> Volker

Cheers,
Ben