fix(security): prevent path traversal in /web-app/ route

[superm1]

The /web-app/(.+) route concatenated the regex capture directly onto web_app_dir with no canonicalization or .. filtering. cpp-httplib URL-decodes %2f → / and %2e → . before regex matching, allowing GET /web-app/..%2f..%2f..%2fetc%2fpasswd to read arbitrary files.

Add path canonicalization via std::filesystem::weakly_canonical and verify the resolved path is confined under web_app_dir before opening. Reject any path that escapes the base directory with 403 Forbidden.

[fl0rianr]

Thanks for fixing the traversal issue. I think the approach is directionally right, but I would not merge this as-is yet.

The confinement check currently compares candidate.string() against base.string() + "/". That is likely broken on Windows because std::filesystem::path::string() uses native separators, so a valid candidate like C:\...\web-app\renderer.bundle.js will not start with C:\...\web-app/ and may be rejected as 403.

Could we avoid string-prefix checks here and compare path components or use std::filesystem::relative(candidate, base) instead?

Also, since this is a security fix, would you consider adding a regression tests for:

• a normal /web-app/... asset still returning 200,
• encoded traversal such as /web-app/..%2f..%2f... not leaking files,
• nonexistent assets returning 404,
• ideally Windows coverage or at least a Windows validation note.

[fl0rianr]

Thanks for addressing the Windows separator issue and adding regression tests. This is much closer.

I think two things still need tightening before merge:

1. The new test/server_webapp.py does not appear to be wired into CI. Since this is a security fix, could we add it to the relevant Linux/Windows hosted test jobs?

2. The traversal assertions are currently too weak because they only assert status_code != 200. A vulnerable implementation could still pass if the attempted target does not exist or the path is normalized differently before reaching the server. Could we make the test use a known existing file outside web-app (for example a sibling resource file) and assert 403 specifically?

Small code nit: rel_str.find("..") will also reject legitimate asset names containing ... It would probably be safer to check path components instead:

for (const auto& part : relative) {
    if (part == "..") {
        res.status = 403;
        res.set_content("Forbidden", "text/plain");
        return;
    }
}

[fl0rianr]

Thanks, this is much improved. The production-side fix looks good to me now: using std::filesystem::relative plus component-wise .. checking addresses the Windows separator issue and avoids rejecting legitimate filenames containing ..

The main traversal regression test is also much stronger now because it targets a known existing file outside web-app and asserts 403.

One remaining CI coverage issue before I’d approve: test/server_webapp.py appears to be wired only into the macOS .dmg build’s unsigned path:

if: steps.check_signing.outputs.has_signing != 'true'

In the current PR run the signed .pkg path is active, so that unsigned-path block is skipped. The regular Linux/Windows CLI/Endpoints jobs also don’t appear to run server_webapp.py.

Would you consider to add test/server_webapp.py to a PR job that always runs, ideally the hosted Linux and/or Windows CLI/Endpoints job?

Finally a minor nit - non-blocking: test_002_normal_css_asset_returns_200 actually tests a hashed SVG asset, so renaming it to SVG or using a more stable asset would be cleaner.

[fl0rianr]

Thanks! Looks good to me now.

[fl0rianr]

CI found a real issue with the new regression test. The production-side path traversal fix still looks fine to me, but test_002_normal_svg_asset_returns_200 is too brittle: it hardcodes ecd3a2194d527cfbf0bc.svg, and that file is not present in the Windows package/run, so the test fails with 404.

I’d suggest removing this SVG-specific test or changing it to discover an asset from index.html dynamically. For the security regression, we already have the important coverage:

• renderer.bundle.js verifies normal asset serving,
• /web-app/renderer.bundle.js verifies the prefixed route,
• the encoded traversal test verifies a known existing file outside web-app returns 403.

So I don’t think we need a hardcoded hashed SVG asset here. Once that brittle test is removed or made dynamic, this should be good again.