I don't think we should leak our address
(which mostly is behind a WOPI host and end-user
has no idea of what host LibreOffice Online is running at) in the
Referer header. Lets be more strict here and don't leak our address
at all.
Change-Id: Ibc30e9b64e2e06e2e8d541c5f089320ecb11412b
Though this guard the user against MITM attacks, but enabling this also
has the potential to brick your websites. So, do not use it/enable it
without understanding what it actually is.
See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Public_Key_Pinning
Though this should work, but I have not been able to test it because of
Firefox and Chrome's limitation/feature that key validation is not done
when certificate chain terminates at a user-defined trust anchor and I
couldn't find any way to temporarily enable the HPKP key validation for
such CA chains.
Change-Id: I64d4ff82b04c59642fa7b8bac2f8788a03950b28
Reviewed-on: https://gerrit.libreoffice.org/36357
Reviewed-by: pranavk <pranavk@collabora.co.uk>
Tested-by: pranavk <pranavk@collabora.co.uk>
This reverts commit de2bc17c04af088d9c7e18a97216b174494e1a9c.
Lets not introduce any cleanup commits while we are near a release, will
apply it again after the release. The cleanup is supposed to not handle
the custom file server root correctly, so don't forget to test it with
a custom file server root before re-reverting.
It changes the path where loleaflet.html is searched for from
/usr/share/loolwsd/loleaflet/... to /usr/share/loleaflet/...
and doesn't find it there.
Change-Id: I23940e9a3e06721f0a8b7493a526f42d2072cfa4
There was an interesting race when we cleared the
inBuffer after the WS upgrade. Since during the
upgrade we also transfer the socket to the DocBroker,
which has its own poll thread, the DocBroker poll
could trigger a POLLIN event if data comes
while the handler (that is handling the WS upgrad
and transfer to DocBroker) hasn't got to the point
where it clears the inBuffer of the data we just
read (i.e. the HTTP GET request). Even if not
the case, after transfering a socket to another
poll thread the socket buffers should not be
touched.
Here we move the inBuffer clearing to be as soon
as we have successfully parsed the request and
are ready to process it.
Also, we don't clear the full buffer, in case
we had read into the buffer both the requst
and the first message, if the thread was switched
out right after getting the POLLIN but before
reading from the socket, giving enough time to
receive more data and reading it together with
first read (which is the request).
Change-Id: I9888d4c2b70d2e433824818bbe7f69f13742486c
Reviewed-on: https://gerrit.libreoffice.org/36326
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Don't think it is necessary/useful to have this header at other places.
This is the most important and perhaps the only where presence of this
header is required and seems sensible to prevent potential attacks.
Change-Id: Iad318e4b83264ac83620b86a40a49e7384e4015e
No idea why it was here in the first place, but download requests are
only made from frames with same origin, so there should be no need to
specify such headers which allow anyone (with other origins) to make
download requests to us.
Change-Id: I314a7ad4c6df8664b1d191cb88ae42c4248ff517
insertfile post requests should be made only from our origin.
Mentioning a '*' against allow-access-allow-origin allows other origins
to be able to make requests to insertfile too provided the attacker
knows the doc key which is not very hard to guess/get.
Change-Id: If98351df48935cfcdc18d6879167c0ac6089796c
Remove unnecessary checks
Rename preprocessFile -> preprocessAndSendLoleafletHtml and
Rename isAdminLoggedIn -> tryAdminLogin
so that their name matches the actual reality of what these
function really does.
Change-Id: I549eae31f8ab0a320bb3ff8ecd17a282b8f91e1a
ie/edge ignores frame-ancestor directive of CSP (yet). Mention X-Frame-Options
for them. Similary, X-Frame-Options allow-from attribute is not
supported by Chrome:
(see https://bugs.chromium.org/p/chromium/issues/detail?id=511521)
In that case, we already have frame-ancestor CSP directive for it.
Change-Id: Ide00c4db88c438de5e9c679360b3da6f4eb4a1be
These conditions must be checked together. Otherwise we might
set _stop prematurely.
Change-Id: I3de0d2b3833959593315669ad245f94c1243f7f7
Reviewed-on: https://gerrit.libreoffice.org/36242
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Tests should have sensible limits so they don't
go overboard and fail needlessly causing noise.
Change-Id: Idd556c348cc0e97e38c710fdbf76fe20c76d8f9b
Reviewed-on: https://gerrit.libreoffice.org/36241
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Plenty of time to do that next time around the cleanup.
We should still, really be doing the majority of the timeout work
inside the DocumentBroker poll itself.
When shutting down accept_poll from
main, we can't remove sockets or cleanup.
That work needs to be done fro within accept_poll's
thread. This is different from when DocBroker's
poll needs to cleanup its own sockets before
it exists.
So we split the stop and removeSockets so they
can each be called in the proper way.
For accept_poll and others that joinThread
we queue a callback to cleanup before stopping.
Change-Id: If780d6a97ac0fc6da6897f895d5b4dda443f9e73
Reviewed-on: https://gerrit.libreoffice.org/36186
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
LOOLWSDServer::stop() now removes the accept_poll
socket, which will assertCorrectThread. So we need
to disable checks before it.
Change-Id: I3445610c1c48c2b4c23bcfcbc87e236b36d18c0b
Reviewed-on: https://gerrit.libreoffice.org/36185
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
By stopping the poll we fail to notify the clients
of the shutdown. Let the DocBroker poll thread
take care of the poll stopping when it's ready.
Change-Id: I2cb4c76da2722ce41a60fc1983b10dc8b18b4cab
Reviewed-on: https://gerrit.libreoffice.org/36184
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And assume correct thread if poll thread is
not running (i.e. no race).
Change-Id: I17958e682aba434ebb47fe0de199b9f530b54dee
Reviewed-on: https://gerrit.libreoffice.org/36183
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This simplifies things, and keeps process management in one thread.
Also - wakeup the DocumentBroker when we want to stop it.
Change-Id: I597ba4b34719fc072a4b4ad3697442b5eebe5784
Reviewed-on: https://gerrit.libreoffice.org/36182
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
assert()'s are no-op in the release builds, but we still want to see threading
problems in the log at least.
Change-Id: Idb02bb018e8f2d628a57ab570249613ad00bcff2
And move more into the callback to ensure
thread affinity.
Change-Id: I1d6985716d0d36aa488b65263ecb41f444f77255
Reviewed-on: https://gerrit.libreoffice.org/36115
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Search for the next 100 ports for a usable one
and pass the one found to forkit so it connects
on that one instead of the default.
Change-Id: I26697dd8b5a35992f9e000a35ad5b44c3a3699dd
Reviewed-on: https://gerrit.libreoffice.org/36114
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And insert sockets after starting the
thread so we poll the socket immediately.
Change-Id: Id336e1838f2f624ebfe59c4c2caf33eaa1a638c9
Reviewed-on: https://gerrit.libreoffice.org/36110
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This was a workaround to Poco's limitation
of requiring socket receiveFrame be given
preallocated buffer, which couldn't be
exceeded by a larger payload. This meant
the receiver had to know the maximum
payload in advance.
Since only the Kit uses Poco sockets,
and the Kit never receives large payloads,
this preamble is now obsolete.
100% (94/94) of old-style tests PASS.
Change-Id: I76776f89497409e5755e335a3e25553e91cf0876
Reviewed-on: https://gerrit.libreoffice.org/36037
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This matches the document between WSD and kit,
making logs much easier to read.
Change-Id: If55a9eb84b4a22d2dc4dd53f5f6ab322ebc3646e
Reviewed-on: https://gerrit.libreoffice.org/36028
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
==20033== Invalid read of size 4
==20033== at 0x466504: ChildProcess::close(bool) (DocumentBroker.hpp:111)
==20033== by 0x44EA28: DocumentBroker::terminateChild(std::string const&, bool) (DocumentBroker.cpp:1313)
==20033== by 0x45F70E: DocumentBroker::pollThread() (DocumentBroker.cpp:264)
==20033== by 0x504B2F: SocketPoll::pollingThreadEntry() (Socket.hpp:486)
==20033== by 0x7310E6F: execute_native_thread_routine (thread.cc:84)
==20033== by 0x7AF60A3: start_thread (pthread_create.c:309)
==20033== by 0x7DF002C: clone (clone.S:111)
==20033== Address 0x0 is not stack'd, malloc'd or (recently) free'd
With this every other client would be able to know about other client's
permission i.e whether they have opened the document as readonly. This
could be important eg: to hide the cursor overlay of readonly users in
the UI or to mark these users as readonly in the userlist.
Change-Id: I5dcb1b4e5a22c9b546d16b69b9216cc7653cff04
Remove locks and replace with isCorrectThread
assertions instead.
Crash recovery still needs some work, but
otherwise tests are clean (91/94 pass).
Change-Id: I9ac3e21854447d19a8e6106487dfd8be00fcf5ef
Since this doesn't necessary mean the document
was loaded completely (as the similarly named
flag in DocumentBroker signifies) rather that
the session was added to DocumentBroker.
Change-Id: Ibfc702bbd111ade2715dcb28ac3aa4e9e8e025dd
It is accessed from the PrisonerPoll
when cleaning up.
Change-Id: Ieb57cdd63cc08632bcdaa4fc5ccd4a1a53c06fe7
Reviewed-on: https://gerrit.libreoffice.org/35788
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
error: no viable conversion from returned value of type 'bool' to
function return type 'std::shared_ptr<DocumentBroker>'
Change-Id: I5d4215ce61e5327ab702dbf6e4cc7be1330afed2
Now the poll thread can exit immediately when loading
fails, since we don't think there is an active
saving on going.
Change-Id: I602b2b506f92309d34ec697553bac05523d70e20
Reviewed-on: https://gerrit.libreoffice.org/35714
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
ClientSession::onDisconnect might not always be
called. The disymmetry between incrementing in
the ctor and decrementing in onDisconnect always
ran the risk of mismatch and leaking connection
counts, eventually blocking new clients.
Change-Id: I39ec65016984c0cddd0e16cfbf201049ced53713
Reviewed-on: https://gerrit.libreoffice.org/35713
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Since this a fake session that doesn't
have a client socket, we push the
messages directly into the ClientSession.
But since the DocBroker poll thread will
probably not be ready by then, there
is no child process and the other
document bits needed to load (or indeed
process any client messages).
So we defer all the fake messages in
a poll callback to insure they are done
in the correct order.
Change-Id: Id81dc4288b305829149e6e9c81d0f7da719c59ad
Reviewed-on: https://gerrit.libreoffice.org/35712
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Sessions are now added to the DocBroker
_sessions map and loaded from the poll
thread first before processing their
messages.
Since messages are not read from the
sockets outside of the poll thread,
there is no reason to queue them
at all. The only exception is when
messages are passed directly
to ClientSession during convert-to
requests. That will be handled
separately (for now convert-to test
fails).
Change-Id: I798166b9e45b5707a33d31137e01a32ce63630b1
Reviewed-on: https://gerrit.libreoffice.org/35705
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Since these numbers are against the thread start time,
they aren't useful for loading subsequent views.
Change-Id: Ib5cf580282841e5b2dbb71c7db4a1868f7eb29e1
Reviewed-on: https://gerrit.libreoffice.org/35704
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Detection of modified documents used the
directory path rather than the document
path, which obviously wasn't correct.
Change-Id: I4a054a9ce2b64d70cd7a0a1c488dcc38ef46a581
Reviewed-on: https://gerrit.libreoffice.org/35526
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This can happen most notably when the kit crashes.
In this case the DocBroker is terminated so all
client connections are closed, allowing clients
to re-connect and re-open the doc.
Change-Id: I9854e11b002ca6e3c2a1a0bbb91ca087679d25bb
The rendering options must be last in the load
command. This is because it could contain
characters that interfere with the parsing.
Change-Id: I14930d580d59014532d07410251acbd1316b4f61
When not sending ping the ping time is not set
which results in the setting the poll timeout to
a negative value, forcing it to return immediately.
This happens when sending ping before upgrading
to WebSocket, which isn't common. One way to
reproduce it, however, is to connect to the
admin console with an unauthenticated socket.
Change-Id: I9f3db1a02b8f8e2781d23d843e848068ad434958
Normally the last session either stopped and terminated
the DocBroker upon saving the doc, or immediately upon
removal when nothing to save.
However, saving could fail, or the session could be
disconnected by the client, or saving could timeout, etc.
In all those failure scenarios the DocBroker should
not linger as a zombie (alive but without sessions).
Here we detect that we are left with no sessions
and terminate correctly.
Change-Id: I31862234e321f63e686f54fa69daacc1fa06ae75
The prisoner poll should wake every so often
to check and rebalance the new children.
However this didn't happen correctly and
WSD would starve of children every so often.
The frequency of checking and rebalancing of
children should be reviewed and optimized.
Also simplified the code to avoid rebalancing
DocBrokers and only do NewChildren.
Change-Id: Id3be34ed3a47c739b606ee7969088397d3807e7a
Termination should normally be initiated by the
DocumentBroker in question, so sending of termination
message on the sockets come from the correct thread.
When termination happens from elsewhere
(f.e. cleanupDocBrokers) we cannot send socket
messages, and have to resort to rude termination.
Change-Id: I94acb7b314f5dbdc45c57049fc1ac8527ba72fb9
Once a socket has changed ownership to a new
poll it will assert thread affinity with said
new poll. So we cannot do any IO on the old
poll's thread at that point and on.
Change-Id: I662f188dea7c377a18f3e546839ec43f2875dc7b
We can calculate the timeout ourselves easily and add it to the
polling loop simplifying life.
Also ensure we never send messages to a non-authenticated thread.
While message emplacement happens in the DocumentBroker poll, we
can be sure that the next iteration of the poll will call
hasQueuedWrites before polling.
ClientSession still isn't getting the notification,
so document is not uploaded to the client just yet.
Change-Id: Ifda8ed394f6df1ec1a5bc1975d296dea496c0aed
This reverts commit 388d7b1dbf.
It is vital to have clean control of thread start. By starting
a thread during init. of a member (or base-clase) we loose lots of
control, some examples:
Any members initialized after a member that auto-starts a
thread, is effectively un-defined, and cannot be accessed
reliably.
This is particularly problematic for sub-classes.
I've seen threads started before the base-class has
finished constructing in the original creating thread -
such that the vtable was not yet updated to the sub-class
causing the transient parent vtable used during construction
to be used in-error.
Now that DocumentBroker has SocketPoll thread,
it's isAlive() must be defined by the lifetime of
both the SocketPoll thread and the ChildProcess,
which it previously did.
Change-Id: I093f8774cf4374d01729a383f6c535de4143fec6
Reviewed-on: https://gerrit.libreoffice.org/35122
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Since all TerminatingPoll instances need to fire
a thread, no reason to do it manually and risk
races. Now TerminatingPoll does it in the ctor.
Change-Id: I59850ad48b3789f3a23d01abb05a7f28e5717031
Reviewed-on: https://gerrit.libreoffice.org/35114
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
DocumentBrokerPoll is always owned by a
single DocumentBroker instance, so we
can hold a reference to it. This eliminates
the need to hold a shared_ptr to the owner
which, in turn, eliminates the need for
a create wrapper.
Change-Id: I954c9dddcc3b2cfdd5dfcc8248ab3d47a897f684
Reviewed-on: https://gerrit.libreoffice.org/35113
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When we read data, we must also handle it,
otherwise the next poll might have no
more data (the request data was read
completely the first time) and we dead
lock waiting for data to process.
Change-Id: I26c69ecc1f0550e8371cf77a6f3928a7a877eff7
Reviewed-on: https://gerrit.libreoffice.org/35080
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When the last client session is disconnected
docBroker must first issue a save and wait
until the kit processes the save and sends
back notfication. Since said notification
goes to the ChildSession (which is the last)
and said ChildSession is the one that signals
to docBroker to persist the doc to the Storage,
we need to keep all components alive and kicking
during this final saving.
As such, when the last session is to be removed
from docBroker, we instead issue an autosave and
continue everything as normal. When the save
notification even arrives and ChildSession signals
docBroker to persist the doc, we check if we were
destroying and in that even remove that last session
and stop the polling thread.
The docBroker instance itself will get cleaned up
in due time.
Change-Id: Ie84e784284e1ec12b0b201d6bf75170b31f66147
As there isn't support (yet) to send files
asynchronously, when the socket native buffer
is small, asynchronous writes naturally return
EWOULDBLOCK. As a temp solution, we send files
synchronously, so there is no need to poll.
This should be replaced witha file-server
polling/serving thread that is dedicated to
sending files only (which closes the connection
when done).
Change-Id: I062fea44bfe54ab8d147b745da97bd499bf00657
So startDestroy must not assume the session
was already added at that point, since addSession
is done when we poll for reading, but not when
the socket has already disconnected.
Change-Id: I7bb7222604269c1cc9f2f4b4dad3ea1054b3e0c9
