-
Notifications
You must be signed in to change notification settings - Fork 167
Enforce schema character set on vital.name in Feature Operation APIs #2864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,30 +157,62 @@ internal class RUMFeatureOperationManager { | |
| /// Validates the `name` and `operationKey` of the command | ||
| private func validateCommand(_ command: RUMOperationStepVitalCommand) -> Bool { | ||
| // Validate name (required) | ||
| guard validateString(command.name, stepType: command.stepType) else { | ||
| guard validateName(command.name, stepType: command.stepType) else { | ||
| return false | ||
| } | ||
|
|
||
| // Validate operationKey if present (optional) | ||
| if let operationKey = command.operationKey, | ||
| !validateString(operationKey, stepType: command.stepType) { | ||
| !validateOperationKey(operationKey, stepType: command.stepType) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| /// Validates the string is not empty | ||
| // or contains only whitespace/line breaks | ||
| private func validateString(_ value: String, stepType: RUMVitalOperationStepEvent.Vital.StepType) -> Bool { | ||
| /// ASCII character set accepted by the backend for `vital.name`, matching | ||
| /// the server-side regex `[\w.@$-]` (letters, digits, `_`, `.`, `@`, `$`, | ||
| /// `-`). Built explicitly from ASCII so that Unicode-aware categories | ||
| /// (which `CharacterSet.alphanumerics` would pull in) do not mask | ||
| /// non-conforming names — e.g. `ログイン` is all Unicode "Letter, other" | ||
| /// and must still trigger the warning. | ||
| private static let validOperationNameCharacters = CharacterSet( | ||
| charactersIn: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_.@$-" | ||
| ) | ||
|
|
||
| /// Validates the operation name. | ||
| /// | ||
| /// Blank / empty names are rejected (the backend rejects them with its | ||
| /// own non-empty precondition before evaluating the character-set regex). | ||
| /// Names that fail the backend's `[\w.@$-]*` character-set regex trigger | ||
| /// a developer warning but the event is still emitted — the backend is | ||
| /// the source of truth on character-set policy, so client-side drop | ||
| /// would force an SDK bump if that policy is ever relaxed. | ||
| private func validateName(_ value: String, stepType: RUMVitalOperationStepEvent.Vital.StepType) -> Bool { | ||
| let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| guard !trimmed.isEmpty else { | ||
| DD.logger.error("Operation `name` and `operationKey` cannot be empty or contain only whitespace/line breaks. \(stepType) command will be ignored.") | ||
| DD.logger.error("Operation `name` cannot be empty or contain only whitespace/line breaks. \(stepType) command will be ignored.") | ||
| return false | ||
| } | ||
|
|
||
| if !value.unicodeScalars.allSatisfy(Self.validOperationNameCharacters.contains) { | ||
| DD.logger.warn("Operation `name` '\(value)' does not match the backend-accepted pattern [\\w.@$-]* (letters, digits, _ . @ $ -). The \(stepType) command will still be emitted and may be rejected by the backend.") | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| /// Validates the operation key: non-blank. The schema does not restrict | ||
| /// the character set for `operation_key`. | ||
| private func validateOperationKey(_ value: String, stepType: RUMVitalOperationStepEvent.Vital.StepType) -> Bool { | ||
| let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| guard !trimmed.isEmpty else { | ||
| DD.logger.error("Operation `operationKey`, when provided, cannot be empty or contain only whitespace/line breaks. \(stepType) command will be ignored.") | ||
| return false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the validation of the |
||
| } | ||
|
|
||
| // Backend takes care of sanitizing user inputs | ||
| return true | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,8 @@ class RUMFeatureOperationManagerTests: XCTestCase { | |
|
|
||
| // MARK: - Edge Cases Tests | ||
|
|
||
| // Blank / empty names are rejected: the backend rejects them with its | ||
| // own non-empty precondition, so the SDK drops client-side to match. | ||
| private let invalidNames = ["", " ", "\n", "\t", " \n\t "] | ||
| func testProcess_OperationWithInvalidName_DoesNotCreateVitalEvent() { | ||
| // Given | ||
|
|
@@ -148,6 +150,81 @@ class RUMFeatureOperationManagerTests: XCTestCase { | |
| XCTAssertEqual(vitalEvents.count, 0) | ||
| } | ||
|
|
||
| // Names containing characters outside the schema facet-path set | ||
| // (letters / digits / - _ . @ $). All must be rejected at the API boundary. | ||
| private let invalidCharacterSetNames = [ | ||
| "user login", // space | ||
| "api/v1", // slash | ||
| "checkout:step", // colon | ||
| "a,b", // comma | ||
| "a+b", // plus | ||
| "login!", // bang | ||
| "login\ttwo", // tab | ||
| "ログイン", // Unicode | ||
| "login🔐", // emoji | ||
| ] | ||
|
|
||
| func testProcess_OperationWithNameOutsideSchemaCharacterSet_StillCreatesVitalEvent() { | ||
| // Names outside the schema facet-path set are warned about but still | ||
| // emitted — the backend is the source of truth on character-set policy, | ||
| // so client-side drop would force an SDK bump if the rule were relaxed. | ||
| for invalidName in invalidCharacterSetNames { | ||
| let command = RUMOperationStepVitalCommand.mockWith(name: invalidName) | ||
|
|
||
| // When | ||
| manager.process( | ||
| command, | ||
| context: mockContext, | ||
| writer: mockWriter, | ||
| activeView: .mockAny() | ||
| ) | ||
| } | ||
|
|
||
| // Then — every event is emitted; the name is preserved verbatim. | ||
| let vitalEvents = mockWriter.events(ofType: RUMVitalOperationStepEvent.self) | ||
| XCTAssertEqual(vitalEvents.count, invalidCharacterSetNames.count) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you validate the warning messages being logged with |
||
| for (emitted, expected) in zip(vitalEvents, invalidCharacterSetNames) { | ||
| XCTAssertEqual(emitted.vital.name, expected) | ||
| } | ||
| } | ||
|
|
||
| func testProcess_OperationWithNameInSchemaCharacterSet_CreatesVitalEvent() { | ||
| // Given — exercises every allowed character class | ||
| let validNames = ["login", "step42", "login-v2", "user_login", "login.v2", "login@prod", "login$1", "LoginV2"] | ||
| for validName in validNames { | ||
| let command = RUMOperationStepVitalCommand.mockWith(name: validName) | ||
|
|
||
| // When | ||
| manager.process( | ||
| command, | ||
| context: mockContext, | ||
| writer: mockWriter, | ||
| activeView: .mockAny() | ||
| ) | ||
| } | ||
|
|
||
| // Then | ||
| let vitalEvents = mockWriter.events(ofType: RUMVitalOperationStepEvent.self) | ||
| XCTAssertEqual(vitalEvents.count, validNames.count) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here no warnings in the |
||
| } | ||
|
|
||
| func testProcess_OperationKeyOutsideNameCharacterSet_CreatesVitalEvent() { | ||
| // operation_key has no character-set restriction in the schema. | ||
| let command = RUMOperationStepVitalCommand.mockWith(name: "login", operationKey: "user foo / bar") | ||
|
|
||
| // When | ||
| manager.process( | ||
| command, | ||
| context: mockContext, | ||
| writer: mockWriter, | ||
| activeView: .mockAny() | ||
| ) | ||
|
|
||
| // Then | ||
| let vitalEvents = mockWriter.events(ofType: RUMVitalOperationStepEvent.self) | ||
| XCTAssertEqual(vitalEvents.count, 1) | ||
| } | ||
|
|
||
| func testProcess_OperationWithInvalidOperationKey_DoesNotCreateVitalEvent() { | ||
| // Given | ||
| for invalidOpKey in invalidNames { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.