fix: correct error messages in workloadMetricHandler and connectionMe…#1662
fix: correct error messages in workloadMetricHandler and connectionMe…#1662sahanaxzy wants to merge 1 commit intokmesh-net:mainfrom
Conversation
…tricHandler Signed-off-by: sahanaxzy <kbsahana2007@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a copy-paste error in the status server implementation. By correcting the error strings returned when invalid parameters are provided, the API now provides accurate feedback to users, improving debuggability and consistency. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A copy-paste error was found, where labels were wrongly unbound. The metrics were named, the error was blamed, now clarity finally is found. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Fixes incorrect copy/paste error strings returned by the status server when enable cannot be parsed for workload/connection metrics endpoints (aligning the error message with the handler/endpoint actually used), addressing #1660.
Changes:
- Update
workloadMetricHandlerparse error message toinvalid workload_metrics enable=... - Update
connectionMetricHandlerparse error message toinvalid connection_metrics enable=...
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid workload_metrics enable=%s", info))) | ||
| return |
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid connection_metrics enable=%s", info))) | ||
| return |
![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 updates error messages in the workload and connection metric handlers to correctly identify the metrics being toggled. The review feedback highlights a security concern regarding Reflected Cross-Site Scripting (XSS) because user-provided input is returned in the response without an explicit Content-Type. It is recommended to use http.Error to safely handle these errors and maintain consistency with other parts of the codebase.
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid workload_metrics enable=%s", info))) |
There was a problem hiding this comment.
The error message reflects the info query parameter without setting a proper Content-Type. This can lead to Reflected Cross-Site Scripting (XSS) if the response is rendered in a browser. Using http.Error is safer as it sets the Content-Type to text/plain; charset=utf-8 and X-Content-Type-Options: nosniff. Additionally, it is more consistent with the error handling used later in this function (e.g., line 292).
| w.WriteHeader(http.StatusBadRequest) | |
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | |
| _, _ = w.Write([]byte(fmt.Sprintf("invalid workload_metrics enable=%s", info))) | |
| http.Error(w, fmt.Sprintf("invalid workload_metrics enable=%s", info), http.StatusBadRequest) |
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | ||
| _, _ = w.Write([]byte(fmt.Sprintf("invalid connection_metrics enable=%s", info))) |
There was a problem hiding this comment.
The error message reflects the info query parameter without setting a proper Content-Type, posing a Reflected XSS risk. Using http.Error mitigates this by ensuring the response is treated as plain text. This also aligns with the error handling used elsewhere in this handler (e.g., lines 315 and 324).
| w.WriteHeader(http.StatusBadRequest) | |
| _, _ = w.Write([]byte(fmt.Sprintf("invalid accesslog enable=%s", info))) | |
| _, _ = w.Write([]byte(fmt.Sprintf("invalid connection_metrics enable=%s", info))) | |
| http.Error(w, fmt.Sprintf("invalid connection_metrics enable=%s", info), http.StatusBadRequest) |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
This fixes a small copy-paste error in
status_server.go. TheworkloadMetricHandlerandconnectionMetricHandlerwere returning "invalid accesslog enable=..." when passed a bad enable value, instead of their respective handler names. This updates the error strings to say "invalid workload_metrics" and "invalid connection_metrics".Which issue(s) this PR fixes:
Fixes #1660
Special notes for your reviewer:
none
Does this PR introduce a user-facing change?: