Review Request 106083: More message filters.
Lasath Fernando
kde at lasath.org
Mon Feb 25 21:31:11 UTC 2013
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 47
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line47>
> >
> > The %1 = url.queryItemValue(QLatin1String("v")) is not escaped here, and can contain ",<,> symbols (and any other complex html).
> >
> > Don't forget that queryItemValue decodes percent-encoded strings.
>
> Nikita Skovoroda wrote:
> I'd make a separate variable for the link and escape it using KUrl.
> Crude example follows:
>
> html template:
> …iframe src=\"%1\"…
> url:
> KUrl frameUrl(QLatin1String("http://www.youtube.com/embed/%1").arg(id));
> inserting the url into the html template:
> .arg(QString::fromAscii(frameUrl.toEncoded()))
>
> Or something like that. That should fix things.
>
> Nikita Skovoroda wrote:
> I missed one thing: ID can contain something like percent-encoded "../html5" string, and http://www.youtube.com/html5 would open inside the iframe in this case. That's not good, either.
>
> Maybe it would be better to just pass the id through QUrl::toPercentEncoding and use the original template, but you need to specify what symbols to keep depending on how you extracted the id.
> If the id was extracted using queryItemValue, then you want just to encode everything (because it decodes everything that was previosly encoded).
> If the id was extracted using a regexp, then you probably wish to keep the % symbols (because you don't want to do double-encoding in case if it's already done).
I added a check to make sure the video id matches a regex for a valid video id. That way nothing can creep in masquerading as a video id.
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 54
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line54>
> >
> > What happens to http://youtube.com/watch?v=someid ?
>
> Nikita Skovoroda wrote:
> Another possible format: http://youtu.be/someid , this is equivalent to http://www.youtube.com/watch?v=someid
>
> Nikita Skovoroda wrote:
> You could add an array/list of structs, for example:
>
> {
> QString urlRegexp; // Url for matching and extracting the video id
> QString urlSubstitute; // Url for embedding, for example, iframe url
> QString html; // The html template that contains the code that needs to be embeded.
> }
>
> This way, it would be easy to support different video hosting services and several possible prefixes for each one.
>
> For example, youtube would have:
>
> urlRegexp = "^(?:https?://)?(?:www\.)(?:youtube\.com/watch.*[&\?]v=|youtu\.be/)([^&]+)" // Or something like that, i haven't checked this one, written down without testing.
> urlSubstitute = "http://www.youtube.com/embed/%1"
> html = "…iframe src=\"%1\"…" // Your html, it's too big to include it here.
>
> Nikita Skovoroda wrote:
> Forgot an «?» after «(?:www\.)»:
> urlRegexp = "^(?:https?://)?(?:www\.)?(?:youtube\.com/watch.*[&\?]v=|youtu\.be/)([^&]+)"
>
> Nikita Skovoroda wrote:
> My wrong, separation of urlSubstitute and html parameters is not needed, because the video id can be encoded separately as described above.
> So, the whole thing could just be a QMap(fromRegex => toHtmlTemplate).
At this stage I don't want to support youtube.be, although I did add a check for youtube.com (without the www) URLs.
The usecase for this plugin is when you copy-paste the video of a URL that you are watching to a friend. Since youtube _allways_ expands to http://www.youtube.com/, there's no point in supporting the former two.
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 58
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line58>
> >
> > What happens if «v» parameter is empty?
>
> Nikita Skovoroda wrote:
> Parsing the url with regular expressions (see above) would solve this with no extra code involved.
it won't match the regex.
- Lasath
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106083/#review18419
-----------------------------------------------------------
On Sept. 11, 2012, 10:19 p.m., Lasath Fernando wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106083/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2012, 10:19 p.m.)
>
>
> Review request for Telepathy.
>
>
> Description
> -------
>
> Right, so here's a big fat review containing many many things, including all the remaining plugins and a few tweaks to Message.
>
> As per usual, all the code is in my scratch repo: http://quickgit.kde.org/index.php?p=clones%2Fktp-text-ui%2Ffernando%2Fgsoc.git&a=summary
>
> Now, I'm going to take a well earned nap.
>
>
> Diffs
> -----
>
> filters/CMakeLists.txt ee7c23d
> filters/bugzilla/CMakeLists.txt PRE-CREATION
> filters/bugzilla/bugzilla-filter.h PRE-CREATION
> filters/bugzilla/bugzilla-filter.cpp PRE-CREATION
> filters/bugzilla/ktptextui_message_filter_bugzilla.desktop PRE-CREATION
> filters/highlight/CMakeLists.txt PRE-CREATION
> filters/highlight/highlight-filter.h PRE-CREATION
> filters/highlight/highlight-filter.cpp PRE-CREATION
> filters/highlight/ktptextui_message_filter_highlight.desktop PRE-CREATION
> filters/latex/CMakeLists.txt PRE-CREATION
> filters/latex/ktp_message_filter_latex_converter.sh PRE-CREATION
> filters/latex/ktptextui_message_filter_latex.desktop PRE-CREATION
> filters/latex/latex-filter.h PRE-CREATION
> filters/latex/latex-filter.cpp PRE-CREATION
> filters/pipes/CMakeLists.txt PRE-CREATION
> filters/pipes/kcm_ktp_filter_config_pipes.desktop PRE-CREATION
> filters/pipes/ktptextui_message_filter_pipes.desktop PRE-CREATION
> filters/pipes/pipes-config.h PRE-CREATION
> filters/pipes/pipes-config.cpp PRE-CREATION
> filters/pipes/pipes-config.ui PRE-CREATION
> filters/pipes/pipes-delegate.h PRE-CREATION
> filters/pipes/pipes-delegate.cpp PRE-CREATION
> filters/pipes/pipes-filter.h PRE-CREATION
> filters/pipes/pipes-filter.cpp PRE-CREATION
> filters/pipes/pipes-model.h PRE-CREATION
> filters/pipes/pipes-model.cpp PRE-CREATION
> filters/pipes/pipes-prefs.h PRE-CREATION
> filters/pipes/pipes-prefs.cpp PRE-CREATION
> filters/pipes/pipes-prefstest.h PRE-CREATION
> filters/pipes/pipes-prefstest.cpp PRE-CREATION
> filters/substitution/CMakeLists.txt PRE-CREATION
> filters/substitution/kcm_ktp_filter_config_substitution.desktop PRE-CREATION
> filters/substitution/ktptextui_message_filter_substitution.desktop PRE-CREATION
> filters/substitution/substitution-config.h PRE-CREATION
> filters/substitution/substitution-config.cpp PRE-CREATION
> filters/substitution/substitution-config.ui PRE-CREATION
> filters/substitution/substitution-filter.h PRE-CREATION
> filters/substitution/substitution-filter.cpp PRE-CREATION
> filters/substitution/substitution-prefs.h PRE-CREATION
> filters/substitution/substitution-prefs.cpp PRE-CREATION
> filters/youtube/CMakeLists.txt PRE-CREATION
> filters/youtube/ktptextui_message_filter_youtube.desktop PRE-CREATION
> filters/youtube/youtube-filter.h PRE-CREATION
> filters/youtube/youtube-filter.cpp PRE-CREATION
> lib/abstract-message-filter.h 7b60d48
> lib/abstract-message-filter.cpp 2a3a897
> lib/message.h ef9530b
> lib/message.cpp 6db648e
> tests/message-processor-basic-tests.h 7dc99e4
> tests/message-processor-basic-tests.cpp 8546605
>
> Diff: http://git.reviewboard.kde.org/r/106083/diff/
>
>
> Testing
> -------
>
> Wrote/passed/failed unit tests; talked to myself so much to the point I swear I have mild schizophrenia now.
>
>
> Thanks,
>
> Lasath Fernando
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130225/0adad36e/attachment.html>
More information about the KDE-Telepathy
mailing list