Enforce schema character set on vital.name in Feature Operation APIs#2864
Enforce schema character set on vital.name in Feature Operation APIs#2864Valpertui wants to merge 2 commits into
Conversation
The backend enforces [\w.@$-]* on vital.name via the vital-common-schema facet-path rule; until now the iOS SDK only checked for blank names. This change validates the character set in RUMFeatureOperationManager: blank names are dropped with an error, names that fail the regex warn but are still emitted (backend is source of truth on the policy). operation_key has no schema character-set rule and is left untouched. mockRandom() on RUMOperationStepVitalCommand now generates ASCII alphanumeric names so existing forgery-based tests stay within the allowed set.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7f226b0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| 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 |
There was a problem hiding this comment.
Shouldn't the validation of the operationKey be relaxed? It is an optional parameter, so it could also be ignored if empty. This way the operation step would not be discarded completely.
|
|
||
| // Then — every event is emitted; the name is preserved verbatim. | ||
| let vitalEvents = mockWriter.events(ofType: RUMVitalOperationStepEvent.self) | ||
| XCTAssertEqual(vitalEvents.count, invalidCharacterSetNames.count) |
There was a problem hiding this comment.
Can you validate the warning messages being logged with DD.mockWith(logger: CoreLoggerMock())?
|
|
||
| // Then | ||
| let vitalEvents = mockWriter.events(ofType: RUMVitalOperationStepEvent.self) | ||
| XCTAssertEqual(vitalEvents.count, validNames.count) |
There was a problem hiding this comment.
And here no warnings in the dd.logger.warnLog
2 participants
What and why?
The authoritative
_vital-common-schema.jsonrestrictsvital.nameto the character set[\w.@$-]*(letters, digits,_,.,@,$,-) and the backend enforces this as a server-side regex on the facet path. Until now the iOS SDK only rejected blank names; non-conforming values (e.g."user login","api/v1","ログイン") were silently accepted and shipped to intake.This PR validates the character set at the API boundary so developers catch the problem at dev time instead of discovering it only after events are dropped at intake.
How?
Split
RUMFeatureOperationManager's singlevalidateStringhelper into:validateName— rejects blank names withDD.logger.error(drops the event), then checks each unicode scalar against an explicit ASCIICharacterSet(charactersIn: "A-Za-z0-9_.@$-"). Using.alphanumericswould have been wrong — it is Unicode-aware and would accept Japanese/Cyrillic/etc.validateOperationKey— blank-check only (schema does not restrictoperation_key).When
namecontains characters outside the allowed set,DD.logger.warnis emitted quoting the exact backend pattern[\w.@$-]*, but the event is still emitted. The rationale: the backend is the source of truth on the policy, so a client-side drop would force customers to bump the SDK if the server rule is ever relaxed.Updated
RUMFeatureMocks.mockRandom()to use.alphanumericsso existing forgery-based tests stay within the allowed set.Tests cover:
"ログイン"and emoji"login🔐"→ warn + emitted (pins ASCII-only semantics against future.alphanumericsregressions)operationKeywith space/slash/etc. → silent emit (no char-set rule)Review checklist
make api-surfacewhen adding new APIs — not applicable, no new public API