web: Fix unsafe HTTP script loading in Web GUI - #11183
web: Fix unsafe HTTP script loading in Web GUI#11183jorge-ferreira-pii wants to merge 2 commits into
Conversation
The viewer and the saved report loaded leaflet, golden-layout, three, elkjs and netlistsvg from CDNs, one of them over plain http (issue The-OpenROAD-Project#11065). They are vendored under src/web/third-party, embedded in the binary and served by web_server, so nothing is downloaded at build time or at run time. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request vendors several third-party browser libraries (leaflet, golden-layout, three, elkjs, netlistsvg) to ensure the web viewer and saved reports can run completely offline without fetching code from CDNs. It introduces an asset-locking script, updates both Bazel and CMake build systems to embed these assets, and implements C++ logic to inline stylesheets and images as data URIs into saved reports. The review feedback highlights two important improvements: first, using generic_string() instead of string() in resolveAssetPath to prevent path resolution failures on Windows due to backslash separators; second, using the noexcept overload of std::filesystem::remove with std::error_code to avoid potential crashes from thrown exceptions.
| return (std::filesystem::path(base_dir) / std::filesystem::path(reference)) | ||
| .lexically_normal() | ||
| .string(); |
There was a problem hiding this comment.
On Windows, std::filesystem::path::string() uses backslashes (\) as path separators. Since the embedded asset lookup table uses forward slashes (/) as keys, resolving paths with string() will cause lookups to fail on Windows. Using generic_string() ensures forward slashes are used on all platforms.
| return (std::filesystem::path(base_dir) / std::filesystem::path(reference)) | |
| .lexically_normal() | |
| .string(); | |
| return (std::filesystem::path(base_dir) / std::filesystem::path(reference)) | |
| .lexically_normal() | |
| .generic_string(); |
|
|
||
| if (assets.missing()) { | ||
| // The warnings above name what was missed; no one is told this was saved. | ||
| std::filesystem::remove(filename); |
There was a problem hiding this comment.
Using the throwing overload of std::filesystem::remove can cause the application to crash if an exception is thrown (e.g., due to permission issues or file locks). It is safer to use the noexcept overload that takes a std::error_code reference.
std::error_code ec;
std::filesystem::remove(filename, ec);The lookup key is built with generic_string(), since the asset table is '/'-separated whatever the host uses, and removing a report that could not be assembled no longer throws over the error that has to be reported. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
jorge-ferreira-pii
changed the title
1 participant
Fixes #11065