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.)
This commit is contained in:
parent
b29ae3c032
commit
bb36ca79d4
4 changed files with 33 additions and 2 deletions
1
loleaflet/dist/errormessages.js
vendored
1
loleaflet/dist/errormessages.js
vendored
|
@ -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/');
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -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.");
|
||||
|
|
|
@ -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.");
|
||||
|
|
Loading…
Reference in a new issue