Escape HTML output in builtin /flags service - #3529
Conversation
The ?setvalue= confirmation message and the ?setvalue&withform page wrote the gflag name/value into the html page without escaping, unlike the flag list page which already escapes values with HtmlReplace. Escape them with WebEscape like rpcz_service does. Plain text output is unchanged.
There was a problem hiding this comment.
🟡 Changes recommended
The new test mutates a global gflag and should use RAII (e.g., GFLAGS_NAMESPACE::FlagSaver) to guarantee flag restoration even on early ASSERT_* exits, preventing cross-test contamination.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the builtin /flags HTML pages against HTML injection by escaping user-controlled gflag names/values at two previously unescaped sinks, and adds a regression test to validate both reflected and stored rendering behavior.
Changes:
- Escape gflag name/value in the
?setvalue&withformHTML form page (set_value_page) usingWebEscape. - Escape gflag name/value in the HTML confirmation message for
?setvalue=...updates. - Add
BuiltinServiceTest.flags_escapingto cover reflected/stored HTML escaping and unchanged plain-text output.
File summaries
| File | Description |
|---|---|
src/brpc/builtin/flags_service.cpp |
Escapes gflag name/value when rendering the HTML set-value form and HTML confirmation output. |
test/brpc_builtin_service_unittest.cpp |
Adds a reloadable string flag + a new test that exercises both HTML sinks and verifies plain-text output remains unescaped. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replace the manual save/restore of the modified gflag with GFLAGS_NAMESPACE::FlagSaver so the flag value is restored on every exit of the test, following the established pattern in brpc_http_rpc_protocol_unittest.cpp.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| brpc::FlagsService service; | ||
| brpc::FlagsRequest req; | ||
| brpc::FlagsResponse res; |
There was a problem hiding this comment.
Done in 4619d7f: req/res are now declared inside each { ... } block so every default_method invocation gets fresh, empty request/response objects. Full brpc_builtin_service_unittest suite still passes (20/20).
🤖 This reply was automatically generated by brpc-oncall
| SetUpController(&cntl, true); | ||
| cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut"; | ||
| cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload); | ||
| service.default_method(&cntl, &req, &res, &done); |
There was a problem hiding this comment.
Done in 4619d7f: req/res are now declared inside each { ... } block so every default_method invocation gets fresh, empty request/response objects. Full brpc_builtin_service_unittest suite still passes (20/20).
🤖 This reply was automatically generated by brpc-oncall
| cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut"; | ||
| cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, ""); | ||
| cntl.http_request().uri().SetQuery("withform", ""); | ||
| service.default_method(&cntl, &req, &res, &done); |
There was a problem hiding this comment.
Done in 4619d7f: req/res are now declared inside each { ... } block so every default_method invocation gets fresh, empty request/response objects. Full brpc_builtin_service_unittest suite still passes (20/20).
🤖 This reply was automatically generated by brpc-oncall
| SetUpController(&cntl, false); | ||
| cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut"; | ||
| cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload); | ||
| service.default_method(&cntl, &req, &res, &done); |
There was a problem hiding this comment.
Done in 4619d7f: req/res are now declared inside each { ... } block so every default_method invocation gets fresh, empty request/response objects. Full brpc_builtin_service_unittest suite still passes (20/20).
🤖 This reply was automatically generated by brpc-oncall
Declare FlagsRequest/FlagsResponse inside each { ... } block so no
state can leak between the default_method invocations.
There was a problem hiding this comment.
🟢 Approval recommended
The escaping is applied at the identified HTML sinks without changing plain-text behavior, and the added unit test covers both reflected and stored rendering paths.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
What problem does this PR solve?
Problem Summary:
The builtin
/flagsservice wrote the gflag name and value into the HTML page without escaping in two places: the?setvalue=confirmation message and the?setvalue&withformform page (set_value_page). Unlike the flag list page, which already escapes values withHtmlReplace, these two sinks rendered the raw value, so a value containing HTML metacharacters was injected into the page markup.What is changed and the side effects?
Changed:
current_valueinset_value_pagewithWebEscape.?setvalue=confirmation message withWebEscape.Side effects:
Performance effects: none, only affects HTML rendering of the
/flagspages.Breaking backward compatibility: none. Plain-text output (non-browser requests) is unchanged.
Check List:
BuiltinServiceTest.flags_escapingcovering the reflected (?setvalue=<payload>) and stored (?setvalue&withform) HTML rendering plus the unchanged plain-text output; the test uses a reloadable string gflag and restores its value afterwards.test/brpc_builtin_service_unittestpasses (20 tests).🤖 This PR was automatically created by brpc-oncall