Differential D15189

[KRun] Don’t follow redirection to speed up and avoid incorrect behavior
ClosedPublic
Actions

Authored by achauvel on Aug 31 2018, 3:50 PM.

Details

Reviewers
dfaure
cfeck
Group Reviewers
Frameworks
Commits
R126:484a7dc89a75: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior
Summary

BUG: 354246

Test Plan

Launch kde-open5 <URL to a website you have an account and that redirects when not connected>, e.g. https://twitter.com/i/notifications.
If it works, we see the correct URL, and not https://twitter.com/login?redirect_after_login=%2Fi%2Fnotifications.

Diff Detail

Repository
R126 KDE CLI Utilities
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
achauvel created this revision.Aug 31 2018, 3:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 31 2018, 3:50 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
achauvel requested review of this revision.Aug 31 2018, 3:50 PM
ngraham added reviewers: Frameworks, dfaure, cfeck.Aug 31 2018, 4:04 PM
ngraham added a subscriber: ngraham.

Can you change Fix https://bugs.kde.org/show_bug.cgi?id=354246 to BUG: 354246? Thanks!

See https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords
It would also be nice if you could update the Test Plan section with an indication of how you tested this change.

achauvel updated this revision to Diff 40784.Aug 31 2018, 5:53 PM
achauvel edited the test plan for this revision. (Show Details)

Previous was incorrect, this one works.

anthonyfieroni added a subscriber: anthonyfieroni.Aug 31 2018, 6:29 PM
anthonyfieroni added inline comments.
src/widgets/krun.cpp
1319 ↗(On Diff #40784)

You cannot remove it, since it can be a local file, remote file with scheme that differs http and should be redirect then. So you can make it like that

if (!job->url().scheme().startsWith("http")) {
    setUrl(job->url());
}

Now it will not redirect only http and https.

anthonyfieroni added inline comments.Aug 31 2018, 6:32 PM
src/widgets/krun.cpp
1319 ↗(On Diff #40784)

Also you can add Qt::CaseInsensitive as second parameter to startsWith for scheme with capital letters.

achauvel updated this revision to Diff 40897.Sep 3 2018, 9:16 AM
ngraham edited the summary of this revision. (Show Details)Sep 4 2018, 3:06 AM
dfaure requested changes to this revision.Sep 8 2018, 8:02 AM

I'm really not convinced about this. What about all the cases where you do want to see the redirection happen?

  • Redirection from http to https
  • Redirection from some outdated URL to the new correct URL
  • Redirection from google.com to www.google.com

I think this needs more research. Why do other browsers show the new URL in all of the above cases and not for the twitter testcase? Different HTTP error code maybe?
(I vaguely remember something about permanent redirections vs normal redirections?)

src/widgets/krun.cpp
1319 ↗(On Diff #40784)

Not necessary, scheme() always returns the scheme lower-cased.

This revision now requires changes to proceed.Sep 8 2018, 8:02 AM
dfaure added a comment.Sep 8 2018, 8:05 AM

OK after reading the bug report, I think this is more about "KRun as used by kde-open5 vs KRun as used by other apps such as konqueror".
We want e.g. konqueror to follow the redirection. On the other hand we don't want kde-open5 to follow the redirection. Then what we might need is a KRun::setFollowRedirections(bool). On by default, set to off by kde-open5.

achauvel added a comment.Nov 13 2018, 6:13 PM

Is anybody interested in helping me figure out how I can implement that properly? I would be glad to start working again on it, but I don’t know enough about KIO right now.

achauvel updated this revision to Diff 47987.EditedDec 22 2018, 6:03 AM

Uses new function added in D17726

The commit message I made contains, after title:

Often, trying to access a restricted webpage redirect to a login page.
If you’re connected to the website, you don’t want to be redirected.




Before, kde-open5 followed the redirection and then opened URL.

 Now, it opens the browser as soon as possible and open the correct page.



Restricted Application added a project: Plasma.  ·  View Herald TranscriptDec 22 2018, 6:03 AM
Restricted Application added a subscriber: plasma-devel.  ·  View Herald Transcript
dfaure added a comment.Dec 22 2018, 11:27 AM
Please add a KIO version ifdef to avoid breaking compilation with older versions of KIO.
achauvel added a comment.Jan 30 2019, 8:43 PM
@dfaure I don’t see how to do that, since there does not seem to have such ifdef in the actual codebase. However I can make set(KF5_MIN_VERSION "5.55.0") instead of 5.54.0 in the CMakeLists.txt.
dfaure added a comment.Jan 30 2019, 10:00 PM
#include <kio_version.h>

#if KIO_VERSION >= QT_VERSION_CHECK(5,55,0)
...
#endif
achauvel updated this revision to Diff 50580.Jan 30 2019, 11:21 PM
Added an ifdef so that it could compile with older version of KIO.
achauvel updated this revision to Diff 50581.Jan 30 2019, 11:22 PM
dfaure accepted this revision.Jan 31 2019, 8:27 AM
This revision is now accepted and ready to land.Jan 31 2019, 8:27 AM
ngraham added a comment.Jan 31 2019, 9:52 PM
Can you provide your email address so we can land this patch for you with proper authorship information? Thanks!
achauvel added a comment.Feb 1 2019, 1:49 PM
It’s perso at hack-libre dot org
Closed by commit R126:484a7dc89a75: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior (authored by achauvel, committed by ngraham).  ·  Explain WhyFeb 1 2019, 6:28 PM
This revision was automatically updated to reflect the committed changes.
Revision Contents
Changeset List
PathPackages
Mkioclient/kioclient.cpp (6 lines)
Diff 50664
View Options
kioclient/kioclient.cpp
Loading...