LyricWiki: link to the actual lyrics page
ClosedPublic

Authored by ltoscano on Sep 5 2018, 10:24 PM.

Details

Summary

Instead of pointing to the homepage of the wiki,
set the link to the actual page of the lyrics
which are being shown.

BUG: 332664
FIXED-IN: 18.12.0

Test Plan

The link to the LyricsWiki points to the actual page.
Nothing changes when the lyrics are not found.

Diff Detail

Repository
R344 Juk
Branch
lyricswiki-url
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2544
Build 2562: arc lint + arc unit
ltoscano requested review of this revision.Sep 5 2018, 10:24 PM
ltoscano created this revision.
mpyne requested changes to this revision.Sep 7 2018, 2:35 AM

Wikia doesn't support HTTPS so I'm at least a little leery of just dumping potentially-malicious output into a clickable link. A simple whitelist might be sufficient to at least make an attack along this vector more annoying than it would be worth.

lyricswidget.cpp
146

If these three temporary variables can be declared as const I would recommend doing so as a micro-optimization.

147

I think we need to specify QUrl::FullyEncoded as an additional parameter here to make sure that the output is a properly encoded URL that can be integrated into lyricsUrl below.

And in general I'm interested in whether we can try to detect invalid responses that try to introduce javascript or other malicious activity (if Wikia is subverted, or intercepted using a MITM attack since they don't support HTTPS).

Full encoding isn't enough for that. Whitelisting would probably be better, maybe using a regex to verify that the fully encoded sub-path matches ^[a-zA-Z0-9_]*$ on the whole string?

I'm not sure of the exact syntax we'd need to support, though if the regex above is good enough then it also means we can afford to ignore the different decoding options since there'd be no difference between FullyDecoded or the default.

This revision now requires changes to proceed.Sep 7 2018, 2:35 AM
mpyne accepted this revision.Sep 7 2018, 11:41 PM

It occurred to me while I was at work today that an attacker in position to MITM the search result we get back would also be able to MITM the lyrics page directly. Given that I'm less worried about whitelisting as that is barely even a speedbump. Encoding is technically still a good idea but I think the default mode should be enough for most normal pages.

This revision is now accepted and ready to land.Sep 7 2018, 11:41 PM

Thanks for the review, sorry for the long delay. I added the encoding parameter and the const keyword on two variables (titlesUrlPart can change)

ltoscano updated this revision to Diff 42272.Sep 24 2018, 9:42 PM

Implement the suggestions: encode the titles value; few const

mpyne accepted this revision.Sep 26 2018, 3:35 AM
This revision was automatically updated to reflect the committed changes.