Fix asset_uri_whitelist regex backtracking issue, add more extensions
#270
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Add TIFF and WEBP as well? |
jhurliman
reviewed
Oct 24, 2023
README.md
Outdated
| @@ -77,7 +77,7 @@ Parameters are provided to configure the behavior of the bridge. These parameter | |||
| * __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,assets]`. | |||
| * __asset_uri_allowlist__: List of regular expressions ([ECMAScript grammar](https://en.cppreference.com/w/cpp/regex/ecmascript)) of allowed asset URIs. Uses the [resource_retriever](https://index.ros.org/p/resource_retriever/github-ros-resource_retriever) to resolve `package://`, `file://` or `http(s)://` URIs. Note that this list should be carefully configured such that no confidential files are accidentally exposed over the websocket connection. As an extra security measure, URIs containing two consecutive dots (`..`) are disallowed as they could be used to construct URIs that would allow retrieval of confidential files if the allowlist is not configured strict enough (e.g. `package://<pkg_name>/../../../secret.txt`). Defaults to `["package://(\w+/?)+\.(dae|stl|urdf|xacro)"]`. | |||
| * __asset_uri_allowlist__: List of regular expressions ([ECMAScript grammar](https://en.cppreference.com/w/cpp/regex/ecmascript)) of allowed asset URIs. Uses the [resource_retriever](https://index.ros.org/p/resource_retriever/github-ros-resource_retriever) to resolve `package://`, `file://` or `http(s)://` URIs. Note that this list should be carefully configured such that no confidential files are accidentally exposed over the websocket connection. As an extra security measure, URIs containing two consecutive dots (`..`) are disallowed as they could be used to construct URIs that would allow retrieval of confidential files if the allowlist is not configured strict enough (e.g. `package://<pkg_name>/../../../secret.txt`). Defaults to `["^package://(?:\w+/)*\w+\.(?:dae|stl|urdf|xacro|png|jpg|jpeg)$"]`. | |||
There was a problem hiding this comment.
Would it make sense to add obj, mtl, fbx, glb, gltf to this default list?
There was a problem hiding this comment.
Yeah, we could do that. Plus tiff and webp?
There was a problem hiding this comment.
I thought Chrome didn't support tiff: https://stackoverflow.com/a/2177012/23649
There was a problem hiding this comment.
Do these mtl, fbx, etc. files all have separate three.js loaders? I doubt they would be supported in studio out of the box, right?
There was a problem hiding this comment.
I thought Chrome didn't support tiff: https://stackoverflow.com/a/2177012/23649
There is a special loader in three.js for TIFF support
There was a problem hiding this comment.
Do these mtl, fbx, etc. files all have separate three.js loaders? I doubt they would be supported in studio out of the box, right?
Yes. MTL files accompany OBJ, and are supported by three.js along with FBX and GLTF (text and binary)
achim-k
changed the title
asset_uri_whitelistasset_uri_whitelist, add more extensions
jtbandes
approved these changes
Oct 24, 2023
achim-k
changed the title
asset_uri_whitelist, add more extensionsasset_uri_whitelist regex backtracking issue, add more extensions
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Public-Facing Changes
Fix exponential regex backtracking issue with default
asset_uri_whitelistDescription
When fetching the asset
package://foxglove_simulation/meshes/kinect_jpgI noticed that I never got a response. It turned out that the reason for this was due to catastrophic backtracking caused by the defaultasset_uri_whitelistregular expression pattern. The default pattern contains nested quantifiers with alternations, which can lead to exponential backtracking. This can cause the program to hang or freeze when trying to match certain input strings.This PR changes the default
asset_uri_whitelistregex pattern to avoid catastrophic backtracking. It additionally adds a couple more extensions to e.g. allow for retrieval of texture files.