Changeset View
Standalone View
lyricswidget.cpp
Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Line(s) | |||||
139 | void LyricsWidget::receiveLyricsReply(QNetworkReply* reply) | 139 | void LyricsWidget::receiveLyricsReply(QNetworkReply* reply) | ||
140 | { | 140 | { | ||
141 | disconnect(m_networkAccessManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(receiveLyricsReply(QNetworkReply*))); | 141 | disconnect(m_networkAccessManager, SIGNAL(finished(QNetworkReply*)), this, SLOT(receiveLyricsReply(QNetworkReply*))); | ||
142 | if (reply->error() != QNetworkReply::NoError) { | 142 | if (reply->error() != QNetworkReply::NoError) { | ||
143 | qCWarning(JUK_LOG) << "Error while fetching lyrics: " << reply->errorString(); | 143 | qCWarning(JUK_LOG) << "Error while fetching lyrics: " << reply->errorString(); | ||
144 | setHtml(i18n("<span style='color:red'>Error while retrieving lyrics!</span>")); | 144 | setHtml(i18n("<span style='color:red'>Error while retrieving lyrics!</span>")); | ||
145 | return; | 145 | return; | ||
146 | } | 146 | } | ||
147 | const QUrlQuery replyUrlQuery(reply->url()); | ||||
mpyne: If these three temporary variables can be declared as `const` I would recommend doing so as a… | |||||
148 | QString titlesUrlPart = replyUrlQuery.queryItemValue(QStringLiteral("titles"), QUrl::FullyEncoded); | ||||
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. mpyne: I think we need to specify `QUrl::FullyEncoded` as an additional parameter here to make sure… | |||||
149 | if (titlesUrlPart.isEmpty()) { | ||||
150 | // default homepage, but this code path should never happen at this point. | ||||
151 | titlesUrlPart = QStringLiteral("Lyrics_Wiki"); | ||||
152 | } | ||||
153 | const QString lyricsUrl = QStringLiteral("http://lyrics.wikia.com/wiki/") + titlesUrlPart; | ||||
147 | 154 | | |||
148 | QString content = QString::fromUtf8(reply->readAll()); | 155 | QString content = QString::fromUtf8(reply->readAll()); | ||
149 | int lIndex = content.indexOf("<lyrics>"); | 156 | int lIndex = content.indexOf("<lyrics>"); | ||
150 | int rIndex = content.indexOf("</lyrics>"); | 157 | int rIndex = content.indexOf("</lyrics>"); | ||
151 | if (lIndex == -1 || rIndex == -1) { | 158 | if (lIndex == -1 || rIndex == -1) { | ||
152 | qCWarning(JUK_LOG) << Q_FUNC_INFO << "Unable to find lyrics in text"; | 159 | qCWarning(JUK_LOG) << Q_FUNC_INFO << "Unable to find lyrics in text"; | ||
153 | setText(i18n("No lyrics available.")); | 160 | setText(i18n("No lyrics available.")); | ||
154 | return; | 161 | return; | ||
155 | } | 162 | } | ||
156 | lIndex += 15; // We skip the tag | 163 | lIndex += 15; // We skip the tag | ||
157 | content = content.mid(lIndex, rIndex - lIndex).trimmed(); | 164 | content = content.mid(lIndex, rIndex - lIndex).trimmed(); | ||
158 | content.replace('\n', "<br />"); | 165 | content.replace('\n', "<br />"); | ||
159 | //setText(content); | 166 | //setText(content); | ||
160 | setHtml("<h1>" + m_title + "</h1>" + | 167 | setHtml("<h1>" + m_title + "</h1>" + | ||
161 | content + | 168 | content + | ||
162 | i18n("<br /><br /><i>Lyrics provided by <a href='http://lyrics.wikia.com/Lyrics_Wiki'>LyricWiki</a></i>")); | 169 | i18n("<br /><br /><i>Lyrics provided by <a href='%1'>LyricWiki</a></i>", lyricsUrl)); | ||
163 | } | 170 | } |
If these three temporary variables can be declared as const I would recommend doing so as a micro-optimization.