RSPEED-2465: Add character pattern validation to rlsapi v1 fields#1225
Open
major wants to merge 1 commit intolightspeed-core:mainfrom
Open
RSPEED-2465: Add character pattern validation to rlsapi v1 fields#1225major wants to merge 1 commit intolightspeed-core:mainfrom
major wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces input validation for API request models by adding regex pattern constants and applying them to specific fields in RlsapiV1SystemInfo and RlsapiV1CLA classes, along with comprehensive test coverage for the validation rules and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/models/rlsapi/test_requests.py (2)
576-600: Duplicatemake_requestfixture — extract to module level.
TestGetInputSourceEdgeCases.make_request_fixture(lines 576–600) is an exact copy ofTestGetInputSource.make_request_fixture(lines 320–344), including the inner_RequestBuilderclass. Hoist it to a module-level fixture so both test classes share a single definition.♻️ Extract to module-level fixture and remove both class-level copies
+@pytest.fixture(name="make_request") +def make_request_fixture() -> Any: + """Factory fixture to build requests with specific context values.""" + + class _RequestBuilder: # pylint: disable=too-few-public-methods + """Helper to construct requests with variable context.""" + + `@staticmethod` + def build( + question: str = "q", + stdin: str = "", + attachment: str = "", + terminal: str = "", + ) -> RlsapiV1InferRequest: + """Build an RlsapiV1InferRequest with specified context values.""" + return RlsapiV1InferRequest( + question=question, + context=RlsapiV1Context( + stdin=stdin, + attachments=RlsapiV1Attachment(contents=attachment), + terminal=RlsapiV1Terminal(output=terminal), + ), + ) + + return _RequestBuilder + class TestGetInputSource: ... - `@pytest.fixture`(name="make_request") - def make_request_fixture(self) -> Any: - ... # remove class TestGetInputSourceEdgeCases: ... - `@pytest.fixture`(name="make_request") - def make_request_fixture(self) -> Any: - ... # remove🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/rlsapi/test_requests.py` around lines 576 - 600, The make_request fixture is duplicated in TestGetInputSourceEdgeCases and TestGetInputSource; extract the pytest fixture (the function make_request_fixture that returns the inner _RequestBuilder with its static build method creating RlsapiV1InferRequest / RlsapiV1Context / RlsapiV1Attachment / RlsapiV1Terminal) to module level, keep the `@pytest.fixture`(name="make_request") decorator, and remove both class-level copies so tests use the single shared module-level make_request fixture; ensure imports remain valid and tests reference make_request as before.
612-628:result.index(...)position checks are redundant after the exact equality assertion.Line 621 already asserts
result == "Q\n\nS\n\nA\n\nT", which fully encodes both content and order. The subsequentindexcomparisons (lines 623–627) add no incremental value.♻️ Remove the redundant position checks
result = request.get_input_source() assert result == "Q\n\nS\n\nA\n\nT" - # Verify order by checking positions - assert ( - result.index("Q") - < result.index("S") - < result.index("A") - < result.index("T") - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/rlsapi/test_requests.py` around lines 612 - 628, The test_priority_order test contains redundant assertions: after asserting result == "Q\n\nS\n\nA\n\nT" you should remove the subsequent position checks that compare result.index("Q") < result.index("S") < result.index("A") < result.index("T"); keep the exact equality assertion only. Locate test_priority_order and the use of make_request.build / request.get_input_source and delete the block performing the index() comparisons so the test remains concise and non-duplicative.src/models/rlsapi/requests.py (1)
15-21:_VERSION_PATTERNis identical to_MACHINE_ID_PATTERN— consider consolidating or aliasing.Both constants resolve to
r"^[a-zA-Z0-9._\-]*$". They're semantically distinct, but having two independent string literals with the same value risks them diverging silently in future edits.♻️ Alias one to the other
# Machine IDs: alphanumeric, dots, underscores, and hyphens (no spaces). _MACHINE_ID_PATTERN = r"^[a-zA-Z0-9._\-]*$" -# Version strings: alphanumeric, dots, underscores, and hyphens. -_VERSION_PATTERN = r"^[a-zA-Z0-9._\-]*$" +# Version strings share the same allowed set as machine IDs. +_VERSION_PATTERN = _MACHINE_ID_PATTERN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/rlsapi/requests.py` around lines 15 - 21, Migrate the duplicated regex by making _VERSION_PATTERN an alias of _MACHINE_ID_PATTERN (or vice-versa) so the same literal isn't repeated; locate the constants _MACHINE_ID_PATTERN and _VERSION_PATTERN in src/models/rlsapi/requests.py and replace the second literal with a reference to the first (e.g., set _VERSION_PATTERN = _MACHINE_ID_PATTERN) and add a short comment explaining they intentionally share the same pattern to prevent future divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/models/rlsapi/requests.py`:
- Around line 15-21: Migrate the duplicated regex by making _VERSION_PATTERN an
alias of _MACHINE_ID_PATTERN (or vice-versa) so the same literal isn't repeated;
locate the constants _MACHINE_ID_PATTERN and _VERSION_PATTERN in
src/models/rlsapi/requests.py and replace the second literal with a reference to
the first (e.g., set _VERSION_PATTERN = _MACHINE_ID_PATTERN) and add a short
comment explaining they intentionally share the same pattern to prevent future
divergence.
In `@tests/unit/models/rlsapi/test_requests.py`:
- Around line 576-600: The make_request fixture is duplicated in
TestGetInputSourceEdgeCases and TestGetInputSource; extract the pytest fixture
(the function make_request_fixture that returns the inner _RequestBuilder with
its static build method creating RlsapiV1InferRequest / RlsapiV1Context /
RlsapiV1Attachment / RlsapiV1Terminal) to module level, keep the
`@pytest.fixture`(name="make_request") decorator, and remove both class-level
copies so tests use the single shared module-level make_request fixture; ensure
imports remain valid and tests reference make_request as before.
- Around line 612-628: The test_priority_order test contains redundant
assertions: after asserting result == "Q\n\nS\n\nA\n\nT" you should remove the
subsequent position checks that compare result.index("Q") < result.index("S") <
result.index("A") < result.index("T"); keep the exact equality assertion only.
Locate test_priority_order and the use of make_request.build /
request.get_input_source and delete the block performing the index() comparisons
so the test remains concise and non-duplicative.
Add Pydantic pattern validators to RlsapiV1SystemInfo (os, version, arch, system_id) and RlsapiV1CLA (nevra, version) to restrict the character set on fields that flow into Splunk telemetry. Prevents injection of control characters, newlines, and HTML/script tags. Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
RSPEED-2465/add-pattern-validation-rlsapi-fields
branch
from
674d880 to
87fedd1
Compare
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
1 participant
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.
Description
The
os,version,arch, andsystem_idfields inRlsapiV1SystemInfo(andnevra,versioninRlsapiV1CLA) accept arbitrary strings with no character restrictions. These values flow directly into Splunk telemetry via_queue_splunk_event(), so a client could inject control characters, newlines, or HTML/script tags into the pipeline.This adds Pydantic
patternvalidators to all six fields, restricting each to the safe character set appropriate for its known value space. Empty strings are still accepted since the fields default to"". Invalid characters now result in a 422 rejection.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
41 new parameterized test cases across
TestSystemInfoCharacterValidationandTestCLACharacterValidation:Valid values (RHEL, 9.3, x86_64, ULIDs, UUIDs, NEVRA strings) pass. Invalid values (XSS payloads, control chars, newlines, null bytes, semicolons) are rejected with
ValidationErrormatching "String should match pattern".Summary by CodeRabbit
Bug Fixes
Tests