-
Notifications
You must be signed in to change notification settings - Fork 68
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
Implement ws-protocol's fetchAsset
specification
#232
Conversation
1a04c50
to
d5150d5
Compare
README.md
Outdated
@@ -76,7 +76,8 @@ Parameters are provided to configure the behavior of the bridge. These parameter | |||
* __client_topic_whitelist__: List of regular expressions ([ECMAScript grammar](https://en.cppreference.com/w/cpp/regex/ecmascript)) of whitelisted client-published topic names. Defaults to `[".*"]`. | |||
* __send_buffer_limit__: Connection send buffer limit in bytes. Messages will be dropped when a connection's send buffer reaches this limit to avoid a queue of outdated messages building up. Defaults to `10000000` (10 MB). | |||
* __use_compression__: Use websocket compression (permessage-deflate). Suited for connections with smaller bandwith, at the cost of additional CPU load. | |||
* __capabilities__: List of supported [server capabilities](https://github.com/foxglove/ws-protocol/blob/main/docs/spec.md). Defaults to `[clientPublish,parameters,parametersSubscribe,services,connectionGraph]`. | |||
* __capabilities__: List of supported [server capabilities](https://github.com/foxglove/ws-protocol/blob/main/docs/spec.md). Defaults to `[clientPublish,parameters,parametersSubscribe,services,connectionGraph,assets]`. | |||
* __asset_uri_whitelist__: List of regular expressions ([ECMAScript grammar](https://en.cppreference.com/w/cpp/regex/ecmascript)) of allowed asset URIs. Defaults to `["package://.*"]`. |
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.
How does a user know what patterns would be valuable to put in here? There is no documentation explaining how file://
urls work.
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.
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.
It uses resource_retriever under the hood which is able to resolve package://
URLs but also file://
and http(s)://
. Will extend the documentation
throw std::runtime_error("Asset URI not allowed: " + uri); | ||
} | ||
|
||
const resource_retriever::MemoryResource memoryResource = _resource_retriever.get(uri); |
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.
Is there any way to limit the ability to access local files within a certain directory? It seems like a big security hole if this can access any file on the local machine.
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.
Is there any way to limit the ability to access local files within a certain directory? It seems like a big security hole if this can access any file on the local machine.
Agreed, that's why I added the allowlist parameter. If you only want to specify certain package folders, you would have to list them manually. file://
URIs are by default not allowed.
Example
['package://robot_description/.+']
- allow only access torobot_description
package folder['package://.+[.]dae$','package://.+[.]stl$']
- only allow fetching ofdae
&stl
files from package folders
Maybe the default ['package://.*']
is too permissive. Should I change it to only allow urdf, dae & stl files?
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.
Bummer: with the default ['package://.*']
one can specify relative paths to retrieve files that are not intended to be fetched as assets:
"uri": "package://ur_description/../../../secret.txt"
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.
Yep ..
is what I was thinking of.
If there is some upstream infrastructure we depend on then maybe there's not a lot we can do about this, but we should at least provide people with a warning in documentation before they enable this feature.
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 there is some upstream infrastructure we depend on then maybe there's not a lot we can do about this, but we should at least provide people with a warning in documentation before they enable this feature.
The default regex is now a lot stricter. Do you think that's sufficient or should we the asset feature by default?
d5150d5
to
38efea3
Compare
dbfd384
to
9a206a0
Compare
@@ -145,4 +147,16 @@ struct ServiceResponse { | |||
|
|||
using ServiceRequest = ServiceResponse; | |||
|
|||
enum FetchAssetStatus { |
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.
- Prefer
enum class
to avoid polluting the namespace - Add
: uint8_t
to match the spec
const auto uri = payload.at("uri").get<std::string>(); | ||
const auto requestId = payload.at("requestId").get<uint32_t>(); |
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 happens if these are missing or the wrong type? if an exception is thrown here, will it be caught somewhere?
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.
Yes, any exception that is not handled in the operation handler would be caught here:
ros-foxglove-bridge/foxglove_bridge_base/include/foxglove_bridge/websocket_server.hpp
Lines 560 to 571 in 9a206a0
case OpCode::TEXT: { | |
_handlerCallbackQueue->addCallback([this, hdl, msg]() { | |
try { | |
handleTextMessage(hdl, msg); | |
} catch (const std::exception& e) { | |
sendStatusAndLogMsg(hdl, StatusLevel::Error, e.what()); | |
} catch (...) { | |
sendStatusAndLogMsg(hdl, StatusLevel::Error, | |
"Exception occurred when executing text message handler"); | |
} | |
}); | |
} break; |
return; | ||
} | ||
|
||
const size_t dataSize = response.status == FetchAssetStatus::Success ? response.data.size() : 0ul; |
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.
do we need to similarly check if status==Error before adding errorMessage.size()
to the messageSize
?
} | ||
|
||
std::future<FetchAssetResponse> waitForFetchAssetResponse(std::shared_ptr<ClientInterface> client) { | ||
auto promise = std::make_shared<std::promise<FetchAssetResponse>>(); |
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.
nit: is make_shared required?
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.
You left a similar comment here: #239 (comment)
Will create an issue to keep track of this
const auto payloadLength = dataLength - offset; | ||
response.data.resize(payloadLength); | ||
std::memcpy(response.data.data(), data + offset, payloadLength); | ||
promise->set_value(response); |
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.
nit: maybe std::move(response)?
throw std::runtime_error("Asset URI not allowed: " + uri); | ||
} | ||
|
||
const resource_retriever::MemoryResource memoryResource = _resource_retriever.get(uri); |
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.
so this will block, even for HTTP fetches? Is there any built-in way to do this in a non-blocking fashion or should we consider doing it in a thread (if the API is thread safe)?
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.
You are right, this will block and prevent any other client operation from being handled since we use only a single thread for handling client requests. I guess that this wouldn't be much of a problem for file:// or package:// URIs as these will succeed/fail fairly quickly, but for http it might indeed be problematic. I will see what I can do
@@ -846,7 +856,9 @@ void FoxgloveBridge::fetchAsset(const std::string& uri, uint32_t requestId, | |||
response.errorMessage = "Failed to retrieve asset " + uri; | |||
} | |||
|
|||
_server->sendFetchAssetResponse(clientHandle, response); | |||
if (_server) { | |||
_server->sendFetchAssetResponse(clientHandle, response); |
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.
Is this method thread safe?
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.
Yes. It's basically the same as _server->sendMessage
which is also called by multiple threads (if subscribed to multiple topics)
### Public-Facing Changes Avoid usage of tmpnam() for creating random filename ### Description Follow up of #232. ROS build farm complained about usage of `tmpnam`. This PR replaces it by using the epoch time in milliseconds to create a somewhat random filename (used for tests only) https://build.ros2.org/job/Idev__foxglove_bridge__ubuntu_jammy_amd64/9/
Public-Facing Changes
Description
Implements the above mentioned spec to fetch assets which will allow Foxglove Studio to request assets (such as URDF mesh files) over the websocket connection.