Skip to content

fix(builtin): escape directory and profiler names - #3743

Open
songgaoye wants to merge 3 commits into
apache:masterfrom
songgaoye:fix_buitin_escape_profiler
Open

songgaoye wants to merge 3 commits into
apache:masterfrom
songgaoye:fix_buitin_escape_profiler

Conversation

@songgaoye

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:
Builtin /dir and hotspots pages render filesystem-derived names into HTML.
A malicious directory entry or historical profiler filename could inject HTML
into the builtin management UI.

What is changed and the side effects?

Changed:

  • Escape HTML link text and href values emitted by Path.
  • Escape profiler filenames in HTML option values and visible text.
  • Encode profiler view and base paths in generated URLs, then decode and
    validate them in both profiler handlers.
  • Add regression tests for hostile names and encoded profile paths.

Side effects:

  • Performance effects: Negligible; escaping/encoding only occurs while
    rendering builtin management pages.

  • Breaking backward compatibility: No. Existing raw view and base query
    parameters remain supported.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An active moderate issue remains: non-responsive diff/base profile requests encode the base path but DisplayResult does not decode it, causing those requests to fail with an invalid base query.

1 open issue
What changed in this PR

This PR hardens built-in directory and profiler management pages against HTML injection from filesystem-derived names and safely encodes profiler paths in generated URLs.

Changes:

  • Escape directory links, profiler option values, and visible profiler names.
  • Add prefixed percent encoding and validation for profiler view and base query paths.
  • Add regression tests for hostile names, encoded paths, and rendered links.
File Description
test/​brpc_builtin_service_unittest.cpp Adds regression tests covering hostile directory and profiler names, encoded profiler paths, and HTML link rendering.
src/​brpc/​builtin/​hotspots_service.cpp Encodes and validates profiler paths in generated URLs and escapes profiler filenames in HTML output.
src/​brpc/​builtin/​common.cpp Escapes directory link URLs and displayed text when rendering HTML.

Review effort: Lite

Comment thread src/brpc/builtin/hotspots_service.cpp Resolved

@wwbmmm wwbmmm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[replied by brpc-oncall robot] The added HTML escaping is the right direction, but the new prefixed path encoding is applied asymmetrically (Copilot already flagged that base is encoded into the non-responsive URL yet never decoded before validation in DisplayResult, so every diff/base view fails), and the encoding layer duplicates a hard-coded prefix in the inline JS while adding no protection beyond what ValidProfilePath already enforces.

Comment thread test/brpc_builtin_service_unittest.cpp Resolved
Comment thread src/brpc/builtin/hotspots_service.cpp Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two moderate active issues remain: generated URLs do not percent-encode reserved filename and query characters, so valid paths cannot be selected reliably and may be misinterpreted by browsers.

2 open issues
1 resolved since last review

Review effort: Lite

Comment thread src/brpc/builtin/common.cpp Resolved
Comment thread src/brpc/builtin/hotspots_service.cpp Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Approval is blocked by unresolved path round-trip and query-encoding issues, a critical profiling-directory traversal risk, and a regression test that does not exercise the escaping behavior.

2 open issues
1 previously missed issue

In code that hasn't changed since last review

High severity Path-prefix validation allows access outside the profiling directory

src/​brpc/​builtin/​hotspots_service.cpp:1069

ValidProfilePath only checks starts_with(FLAGS_rpc_profiling_dir), so a sibling such as ./rpc_data/profiling_evil/... also passes when the configured directory is ./rpc_data/profiling. The view/base values are later passed to pprof, allowing an HTTP caller to select files outside the profiling directory; checking the enumerated names here does not fix that handler path. Require a directory boundary (or canonicalize and verify the parent) before accepting the path.

2 resolved since last review

Review effort: Lite

if (link.text) {
os << link.text;

const std::string escaped_uri = WebEscape(EncodeUriPath(link.uri));
Comment on lines +831 to +846
const std::string filename = "profile'><img src=x onerror=alert(1)>.cpu";
const butil::FilePath profile_path = profile_dir.Append(filename);
ASSERT_EQ(0, butil::WriteFile(profile_path, "", 0));

ClosureChecker done;
brpc::Controller cntl;
brpc::HotspotsRequest req;
brpc::HotspotsResponse res;
brpc::HotspotsService service;
SetUpController(&cntl, true);
service.cpu(&cntl, &req, &res, &done);

const std::string& body = cntl.response_attachment().to_string();
EXPECT_EQ(std::string::npos, body.find(filename))
<< "unescaped profile filename in html: " << body;
EXPECT_EQ(std::string::npos, body.find(brpc::WebEscape(filename)));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants