-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block External Content from Zim Web Pages #1116
Conversation
0558bd9
to
f46e0df
Compare
@veloman-yunkan A review please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the history of this PR, I think that blocking of external resources must be introduced unconditionally in the first commit (so that it is immediately obvious how ExternReqInterceptor
is going to be utilized) and then made optional in subsequent commit.
src/webview.h
Outdated
{ | ||
Q_OBJECT | ||
public: | ||
ExternalReqInterceptor(bool isActive, QObject* parent=nullptr) : QWebEngineUrlRequestInterceptor(parent), m_isActive(isActive) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Make
explicit
- Fit into line length of 80 chars
src/webview.h
Outdated
qDebug() << "Blocked external request to URL >" | ||
<< reqUrl << "<"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a strange way of quoting the URL (as >URL<
)
src/portutils.h
Outdated
#if QT_VERSION < QT_VERSION_CHECK(5, 13, 0) // Earlier than Qt 5.13 | ||
inline void setWebViewRequestInterceptor(WebView* webView, QWebEngineUrlRequestInterceptor* interceptor) { | ||
webView->page()->profile()->setRequestInterceptor(interceptor); | ||
} | ||
#else // Qt 5.13 and later | ||
inline void setWebViewRequestInterceptor(WebView* webView, QWebEngineUrlRequestInterceptor* interceptor) { | ||
webView->page()->setUrlRequestInterceptor(interceptor); | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a non-portable functionality is used in a single place (and is not likely to be used somewhere else in the future) it doesn't need to go into portutils
src/webview.h
Outdated
virtual void interceptRequest(QWebEngineUrlRequestInfo &info) override | ||
{ | ||
if (!m_isActive) | ||
return; | ||
|
||
const QString reqUrl = info.requestUrl().toString(); | ||
if (!reqUrl.startsWith("zim://")) | ||
{ | ||
qDebug() << "Blocked external request to URL >" | ||
<< reqUrl << "<"; | ||
info.block(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the implementation into webview.cpp
.
I'd rather test using a ZIM file with references to external resources. |
Could you point me to one? I am not aware of how to filter Zims to find ones with external resources. |
I don't know of any such ZIM file off the top of my head and I don't even remember if I have seen one. As long as Kiwix is concerned a non-fully-self-contained ZIM file could only be created due to a bug in the scraper. @kelson42 and/or @rgaudin are more likely to point to such a file. If no such test data is available it can be created using zim-tools:
Or you can take https://github.com/openzim/zim-tools/blob/main/test/data/zimfiles/create_test_zimfiles as a starting point and modify its source data to meet your requirements. |
We surely have some zimit files that includes this but I can't point to one in particular. Best is probably as suggested to create a custom one. zimwriterfs might even be simpler for this. |
@ShaopengLin Sorry but I only see that properly now. I see no scenario where this option should allow external loading. Do you? I believe this should not be any option. Sorry I shoukd have seen this "detail" in the original feature request. |
I agree. we shouldn't really allow external loading in the first place. Zim files are supposed to be the 'offline' versions of websites after all and are not supposed to generate any network traffic. |
f46e0df
to
ce8aef3
Compare
I used singleton for the class since it is more space-efficient. Here is the test file, where the main page should try to load an image externally. |
The commit description better be changed too (in this case it will be quite appropriate to tell what you have achieved instead of how you did it). |
ce8aef3
to
fcfa124
Compare
fcfa124
to
33582f3
Compare
Webpages blocks all requests with Url not starting with "zim://", i.e. native to the Zim file.
33582f3
to
f8d646d
Compare
CI broken because of #1121, but this has nothing to do with this PR. Therefore merging. |
Fix #904
Changes:
You can add this code to TabBar::openUrl to add an image, sourced online, to the top of each article.