loleaflet/dist/errormessages.js | 5 +++--
loleaflet/src/core/Socket.js | 3 +++
loolwsd/LOOLWSD.cpp | 17 ++++++++++++++++-
loolwsd/Storage.cpp | 14 +++++++++++++-
4 files changed, 35 insertions(+), 4 deletions(-)
New commits:
commit bb36ca79d4318d7916d74c587673f477659c4d0c
Author: Tor Lillqvist <t...@collabora.com>
Date: Mon Oct 17 16:55:20 2016 +0300
Attempt to handle unauthorized WOPI usage better
Use the previously unused UnauthorizedRequestException for this, and
throw a such in StorageBase::create() when the WOPI host doesn't match
any of those configured.
In a developer debug build, without access to any real WOPI
functionality, you can test by setting the FAKE_UNAUTHORIZED
environment variable and attempting to edit a plain local file:
URI. That will cause such an exception to be thrown in that function.
Catch that UnauthorizedRequestException in
ClientRequestHandler::handleGetRequest(), and send an 'error:
cmd=internal kind=unauthorized' message to the client. Handle that in
loleaflet in the same place where the 'error: cmd=internal
kild=diskfull' message is handled, and in the same fashion, giving up
on the document.
Actually, using exceptions for relatively non-exceptional situations
like this is lame and makes understanding the code harder, but that is
just my personal preference...
FIXME: By the time StorageBase::create() gets called we have already
sent three 'statusindicator:' messages ('find', 'connect', and
'ready') to the client. We should ideally do the checks we do in
StorageBase::create() much earlier.
Also consider that ClientRequestHandler::handleClientRequest() has
code that catches UnauthorizedRequestException and
BadRequestException, and tries to set the HTTP response in those
cases. I am not sure if that functionality has ever been exercised,
though. Currently, we upgrade the HTTP connection to WebSocket early,
and only after that we check whether the WOPI host is authorized
etc. By that time it is too late to return an HTTP response to the
user. If that even is what we ideally should do? If not, then we
probably should drop the code that constructs HTTP responses and
attempts to send them.
Also, if I, as a test, force an HTTPResponse::HTTP_BAD_REQUEST to be
sent before the HTTP connection is upgraded to WebSocket, loleaflet
throws up the generic "Well, this is embarrassing" dialog anyway. At
least in Firefox on Linux. (Instead of the browser showing some own
dialog, which I was half-expecting to happen.)
diff --git a/loleaflet/dist/errormessages.js b/loleaflet/dist/errormessages.js
index 8c97e00..9ccfaf8 100644
--- a/loleaflet/dist/errormessages.js
+++ b/loleaflet/dist/errormessages.js
@@ -2,4 +2,5 @@ exports.diskfull = _('No disk space left on server, please
contact the server ad
exports.emptyhosturl = _('The host URL is empty. The loolwsd server is
probably misconfigured, please contact the administrator.');
exports.limitreached = _('This development build is limited to %0 documents,
and %1 connections - to avoid the impression that it is suitable for deployment
in large enterprises. To find out more about deploying and scaling %2 checkout:
<br/><a href=\"%3\">%3</a>.');
exports.serviceunavailable = _('Service is unavailable. Please try again later
and report to your administrator if the issue persists.');
+exports.unauthorized = _('Unauthorized WOPI host. Please try again later and
report to your administrator if the issue persists.');
exports.wrongwopisrc = _('Wrong WOPISrc, usage: WOPISrc=valid encoded URI, or
file_path, usage: file_path=/path/to/doc/');
diff --git a/loleaflet/src/core/Socket.js b/loleaflet/src/core/Socket.js
index 1e44434..c0c9cdb 100644
--- a/loleaflet/src/core/Socket.js
+++ b/loleaflet/src/core/Socket.js
@@ -149,6 +149,9 @@ L.Socket = L.Class.extend({
if (command.errorKind === 'diskfull') {
this._map.fire('error', {msg:
errorMessages.diskfull});
}
+ else if (command.errorKind === 'unauthorized') {
+ this._map.fire('error', {msg:
errorMessages.unauthorized});
+ }
if (this._map._docLayer) {
this._map._docLayer.removeAllViews();
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 0631c6c..468135c 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -885,6 +885,13 @@ private:
{
throw;
}
+ catch (const UnauthorizedRequestException& exc)
+ {
+ Log::error("Error in client request handler: " +
std::string(exc.what()));
+ status = "error: cmd=internal kind=unauthorized";
+ Log::trace("Sending to Client [" + status + "].");
+ ws->sendFrame(status.data(), (int) status.size());
+ }
catch (const std::exception& exc)
{
Log::error("Error in client request handler: " +
std::string(exc.what()));
@@ -1096,10 +1103,18 @@ public:
response.setStatusAndReason(HTTPResponse::HTTP_SERVICE_UNAVAILABLE);
}
+ if (responded)
+ Log::debug("Already sent response!?");
if (!responded)
{
+ // I wonder if this code path has ever been exercised
+ Log::debug("Attempting to send response");
response.setContentLength(0);
- response.send();
+ std::ostream& os = response.send();
+ if (!os.good())
+ Log::debug("Response stream is not good after send");
+ else
+ Log::debug("Response stream *is* good after send");
}
Log::debug("Thread finished.");
diff --git a/loolwsd/Storage.cpp b/loolwsd/Storage.cpp
index ca4ca25..d500aae 100644
--- a/loolwsd/Storage.cpp
+++ b/loolwsd/Storage.cpp
@@ -130,6 +130,11 @@ bool isLocalhost(const std::string& targetHost)
std::unique_ptr<StorageBase> StorageBase::create(const Poco::URI& uri, const
std::string& jailRoot, const std::string& jailPath)
{
+ // FIXME: By the time this gets called we have already sent to the client
three
+ // 'statusindicator:' messages: 'find', 'connect' and 'ready'. We should
ideally do the checks
+ // here much earlier. Also, using exceptions is lame and makes
understanding the code harder,
+ // but that is just my personal preference.
+
std::unique_ptr<StorageBase> storage;
if (UnitWSD::get().createStorage(uri, jailRoot, jailPath, storage))
@@ -141,6 +146,13 @@ std::unique_ptr<StorageBase> StorageBase::create(const
Poco::URI& uri, const std
else if (uri.isRelative() || uri.getScheme() == "file")
{
Log::info("Public URI [" + uri.toString() + "] is a file.");
+#if ENABLE_DEBUG
+ if (std::getenv("FAKE_UNAUTHORIZED"))
+ {
+ Log::fatal("Faking an UnauthorizedRequestException");
+ throw UnauthorizedRequestException("No acceptable WOPI hosts found
matching the target host in config.");
+ }
+#endif
if (FilesystemEnabled)
{
return std::unique_ptr<StorageBase>(new LocalStorage(uri,
jailRoot, jailPath));
@@ -157,7 +169,7 @@ std::unique_ptr<StorageBase> StorageBase::create(const
Poco::URI& uri, const std
return std::unique_ptr<StorageBase>(new WopiStorage(uri, jailRoot,
jailPath));
}
- Log::error("No acceptable WOPI hosts found matching the target host ["
+ targetHost + "] in config.");
+ throw UnauthorizedRequestException("No acceptable WOPI hosts found
matching the target host [" + targetHost + "] in config.");
}
throw BadRequestException("No Storage configured or invalid URI.");
commit b29ae3c0327c0617afe4b59328b1b3f31855087c
Author: Tor Lillqvist <t...@collabora.com>
Date: Mon Oct 17 16:23:22 2016 +0300
Sort lines for clarity
diff --git a/loleaflet/dist/errormessages.js b/loleaflet/dist/errormessages.js
index 47a81fa..8c97e00 100644
--- a/loleaflet/dist/errormessages.js
+++ b/loleaflet/dist/errormessages.js
@@ -1,5 +1,5 @@
-exports.wrongwopisrc = _('Wrong WOPISrc, usage: WOPISrc=valid encoded URI, or
file_path, usage: file_path=/path/to/doc/');
-exports.emptyhosturl = _('The host URL is empty. The loolwsd server is
probably misconfigured, please contact the administrator.');
exports.diskfull = _('No disk space left on server, please contact the server
administrator to continue.');
+exports.emptyhosturl = _('The host URL is empty. The loolwsd server is
probably misconfigured, please contact the administrator.');
exports.limitreached = _('This development build is limited to %0 documents,
and %1 connections - to avoid the impression that it is suitable for deployment
in large enterprises. To find out more about deploying and scaling %2 checkout:
<br/><a href=\"%3\">%3</a>.');
exports.serviceunavailable = _('Service is unavailable. Please try again later
and report to your administrator if the issue persists.');
+exports.wrongwopisrc = _('Wrong WOPISrc, usage: WOPISrc=valid encoded URI, or
file_path, usage: file_path=/path/to/doc/');
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
