fix: Validate the log_level severity and partition parameters - #7922
Closed
SoubeDev wants to merge 2 commits into
Closed
fix: Validate the log_level severity and partition parameters#7922SoubeDev wants to merge 2 commits into
SoubeDev wants to merge 2 commits into
Conversation
doLogLevel read both parameters with an unguarded asString(), which throws for arrays and objects and silently stringifies every scalar. The throw was swallowed by callMethod's catch-all, so a malformed value produced a generic internal error rather than invalidParams, and the request was recorded as a server fault in the perf log. The severity path was only half broken: a coerced scalar still failed Logs::fromString and came back as invalidParams, so just arrays and objects misbehaved. The partition path had no downstream validation at all, and Logs::get creates a sink on demand, so a coerced value permanently added a partition named "42", "true" or "" to the map that log_level itself then reported on every subsequent call. Guard both conversions with an isString() check and return invalidParams when it fails. Reject the empty partition name for the same reason the type check exists: it is not a partition anyone can have meant, and accepting it wedges a nameless sink into the listing forever. This makes the handler stricter: a severity or partition given as a number, a bool, or null was previously coerced and is now rejected. The commandline builds both fields from argv via asString(), so they are always strings there and the parser is unaffected. Also drop the second isMember(partition) test, which was unconditionally true because the preceding branch already returned, along with the unreachable return it guarded. No behavior change. Add a LogLevel test suite, which did not exist; the RPCCall cases cover only the commandline-to-JSON parse and never reach the handler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry was added with a placeholder because the pull request did not exist when the fix was committed. Point it at XRPLF#7922. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
doLogLevel read both parameters with an unguarded asString(), which throws for arrays and objects and silently stringifies every scalar. The throw was swallowed by callMethod's catch-all, so a malformed value produced a generic internal error rather than invalidParams, and the request was recorded as a server fault in the perf log.
The severity path was only half broken: a coerced scalar still failed Logs::fromString and came back as invalidParams, so just arrays and objects misbehaved. The partition path had no downstream validation at all, and Logs::get creates a sink on demand, so a coerced value permanently added a partition named "42", "true" or "" to the map that log_level itself then reported on every subsequent call.
Guard both conversions with an isString() check and return invalidParams when it fails. Reject the empty partition name for the same reason the type check exists: it is not a partition anyone can have meant, and accepting it wedges a nameless sink into the listing forever.
This makes the handler stricter: a severity or partition given as a number, a bool, or null was previously coerced and is now rejected. The commandline builds both fields from argv via asString(), so they are always strings there and the parser is unaffected.
Also drop the second isMember(partition) test, which was unconditionally true because the preceding branch already returned, along with the unreachable return it guarded. No behavior change.
Add a LogLevel test suite, which did not exist; the RPCCall cases cover only the commandline-to-JSON parse and never reach the handler.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)