fix: validate custom property name charset#27808
Conversation
github-actions
Bot
added
backend
safe to test
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
There was a problem hiding this comment.
Pull request overview
This PR tightens custom property name validation to prevent a small set of characters that break downstream parsers, by enforcing a stricter JSON Schema pattern plus server-side validation and integration tests.
Changes:
- Added a
customPropertyNamedefinition inbasic.jsonwith a restricted character set and max length. - Updated
customProperty.jsonto validatenameagainstcustomPropertyNameinstead ofentityName. - Added Java-side validation in
TypeRepositoryand integration tests inTypeResourceITfor allowed/disallowed names, leading character, and length rules.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openmetadata-spec/src/main/resources/json/schema/type/customProperty.json | Switches name validation to the new customPropertyName definition and updates the field description to document the new rules. |
| openmetadata-spec/src/main/resources/json/schema/type/basic.json | Introduces the reusable customPropertyName schema definition (pattern + length). |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TypeRepository.java | Adds defense-in-depth validation for custom property names during add/update operations. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TypeResourceIT.java | Adds integration coverage for allowed/disallowed character sets, leading character rules, and max length. |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
Jest test CoverageUI tests summary
|
🔴 Playwright Results — 1 failure(s), 17 flaky✅ 3987 passed · ❌ 1 failed · 🟡 17 flaky · ⏭️ 93 skipped
Genuine Failures (failed on all attempts)❌
|
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TypeRepository.java:228
validateCustomPropertyNameis only invoked fromvalidateProperty(CustomProperty), which is used by the/types/{id}add/update-property endpoint. Other write paths (notablyTypeUpdater.updateCustomProperties()→storeCustomProperty()during PUT/PATCH of aType) can still persist custom properties without any name validation, allowing clients to bypass this restriction via JSON Patch/PUT. Consider validating allupdated.getCustomProperties()entries (e.g., callvalidateCustomPropertyName/validatePropertyforaddeditems, or enforce it inprepare(Type, boolean)for every custom property) so the rule cannot be bypassed.
private void validateProperty(CustomProperty customProperty) {
validateCustomPropertyName(customProperty.getName());
switch (customProperty.getPropertyType().getName()) {
case "enum" -> validateEnumConfig(customProperty.getCustomPropertyConfig());
case "table-cp" -> validateTableTypeConfig(customProperty.getCustomPropertyConfig());
case "date-cp" -> validateDateFormat(
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
Tighten custom property name validation to block characters that break
downstream parsers, with verified empirical reproduction:
- `"` causes HTTP 500 on PUT /metadata/types/{id}
- `:` breaks CSV import — exporter writes `key:value;key:value`, importer
splits at first colon, treats prefix as the field name
- `^` breaks OpenSearch query when the name is in
searchSettings.searchFields — Lucene reads `^` as the boost separator
in `field^boost`
- `$` breaks CSV import via java.util.regex.Matcher.replaceAll which
interprets `$<letter>` as a backreference
Adds a `customPropertyName` definition in basic.json and switches
customProperty.json to reference it. Adds a defensive regex check in
TypeRepository.validateProperty so the API returns 400 with a clear
error message even if schema validation is bypassed.
Tests cover allowed-charset acceptance, the four blocked characters,
leading-character validation, max-length enforcement, and unbalanced
brackets.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/common/SanitizedInput/SanitizedInput.tsx:32
shouldSanitizeis now part of this component's public API, buthandleChangestill only depends ononChange. If a caller togglesshouldSanitizewithout remounting, the callback keeps using the old mode and sanitizes (or skips sanitizing) incorrectly until the component is recreated.
const handleChange = useCallback(
(e: ChangeEvent<HTMLInputElement>) => {
const sanitizedValue = shouldSanitize
? getSanitizeContent(e.target.value)
: e.target.value;
if (onChange) {
onChange({ ...e, target: { ...e.target, value: sanitizedValue } });
}
},
[onChange]
Schema-side counterpart to the UI changes in the previous two commits: basic.json#/definitions/customPropertyName now blocks &, <, > alongside the existing " : ^ $ \\. The DOMPurify pass on the UI sanitizes &, <, > into HTML entities, which produced inconsistent persisted values; rejecting them at the schema layer prevents that drift across all write paths. IT updates: - Drop &, <, > from the allowed-charset cases (and the "withMatched(pair)And<more>" composite) - Add &, <, > to the disallowed-charset cases - Drop "<" leading-character case (now covered as a disallowed character) - Drop "<" / ">" unbalanced-bracket cases
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
Bean Validation runs for the dedicated PUT /types/{id} (addOrUpdateProperty)
because the resource declares @Valid CustomProperty, and the createOrUpdate
path can't carry customProperties at all (CreateType schema doesn't include
the field). PATCH /types/{id} accepts an opaque JsonPatch, so @Valid never
reaches into the resulting customProperties[] — a JSON Patch like
[{"op":"add","path":"/customProperties/-","value":{"name":"bad:colon",...}}]
persisted bad-named properties (verified live: HTTP 200 before this fix).
Run Hibernate Validator programmatically inside TypeRepository.prepare() so
every write path enforces the schema-derived @pattern / @SiZe / @NotNull on
each CustomProperty. The rule still lives only in basic.json — picked up via
the generated @pattern annotation, executed via ValidatorUtil.validate.
Tests in TypeResourceIT:
- test_patchCannotAddCustomPropertyWithDisallowedName — seeds a valid property
to ensure /customProperties exists, then PATCHes appending a name with ':',
asserts InvalidRequestException and verifies the bad name is not persisted
- test_patchCanAddCustomPropertyWithValidName — guards against the fix
rejecting valid PATCH-driven additions
When the property name appears in extension.<propertyName>^boost entries of
searchSettings.searchFields, OpenSearch treats * as a field-path wildcard.
The literal * field never matches its own wildcard pattern, so the field
gets silently skipped from the query and Explore search returns no hit for
the value. Bisected against the running server: of 12 candidate Lucene-special
chars, only * actually breaks the mainline UI search flow. ? ~ ( ) { } [ ] /
! and space all returned hits via the searchFields path because OS looks up
the field literally and only treats * as a wildcard at that layer.
Updates the regex + description in basic.json/customProperty.json, the UI
regex in regex.constants.ts, the validation message across 19 locales, the
generated TS docstrings, the Playwright invalid-name fixtures and spec, and
the IT TypeResourceIT case (with*asterisk moves from allowed to disallowed).
…h types prepare() previously validated the entire customProperties[] on every Type write. An upgraded instance with a legacy property whose name contained a now-banned char would then reject any subsequent PUT/PATCH on that type, even when the write only adds a different valid property. Move the name validation into TypeUpdater.updateCustomProperties() and scope it to the `added` list computed by recordListChange against the original entity. New properties are still validated; pre-existing names are left alone. Replace the IT PATCH cases' shared `topic` Type with a fresh, namespaced entity-category Type per test (createEntityTypeForTest). The shared `topic` was mutated concurrently by other tests in the class — combined with PATCH's lack of per-type locking, that produced lost-update races and flaky asserts. The fresh per-test type has customProperties: [] from creation, so the patch sets the array directly without a seed property.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Type.customProperties is a lazy field — TypeRepository.setFields only populates it when the request URL includes ?fields=customProperties. The default getTypeById helper omits the param, so the read-back always saw customProperties == null. That made test_patchCanAdd... fail (the just- persisted property wasn't visible) and made test_patchCannotAdd... pass for the wrong reason (would have stayed green even if the bad name had slipped through validation). Add a fields-aware getTypeById overload and use it in both PATCH cases. Empirically verified against the live server: good name returns 200 + appears in customProperties, bad name returns 400 + does not.
Code Review ✅ Approved 3 resolved / 3 findingsTightens custom property name validation with a new schema definition and Java consistency check to prevent downstream parser failures. The PR resolves previous argument order, test naming, and regex mismatch findings. ✅ 3 resolved✅ Bug: assertEquals arguments are swapped (expected vs actual)
✅ Bug: NAME_SUFFIX contains now-disallowed chars, will break Playwright tests
✅ Bug: Frontend/backend regex mismatch: &, <, > blocked in UI but allowed by backend
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Fixes #27699
Summary
Tighten custom property name validation to block characters that empirically break downstream parsers. Adds a new
customPropertyNameschema definition, defense-in-depth Java validation inTypeRepository, a schema-vs-Java consistency test that prevents drift between the two, and integration tests covering allowed/blocked charsets, leading-character rules, length, and unbalanced brackets.Why
Custom property names previously inherited the very permissive
entityNamepattern (^((?!::).)*$), which only blocks::. A matrix run on 1.13 across 24 special chars × 17 property types × 4 operations identified five characters that break specific operations regardless of property type:"PUT /metadata/types/{id}:key:value;key:value; importer splits at first:, treats prefix as the field name (Unknown custom field: <prefix>)^searchSettings.searchFields, OpenSearch reads^as the boost separator in thefield^boostgrammar (x_content_parse_exception [bool] failed to parse field [should])$Matcher.replaceAllinterprets$<letter>as a regex backreference (Illegal group reference) when the value contains certain characters\entityReference/entityReferenceListtypes (export emits double-backslash, import un-escapes to single, key lookup fails)All other tested characters (alphanumeric,
_,-,.,/,&,%,#,@,!,,,;,=,|,',+,?,*,~,`, space,(,),<,>,[,],{,}) round-trip cleanly across CREATE / PATCH / CSV (real, non-dry-run) / Search across all 17 property types, including multi-property CSV roundtrips that exercise the;and,CSV-quoting layer.Note: subsequent live testing on the actual Explore searchFields path tightened the blocklist further.
&,<,>are blocked because DOMPurify on the UI auto-encodes them to HTML entities, producing inconsistent stored values.*is blocked because it triggers ES field-path wildcard expansion inextension.<propertyName>^boostlookups — the literal field never satisfies its own wildcard pattern, so Explore search silently drops it. See the "Final charsets" section below for the authoritative list.Final charsets
A-Z a-z 0-9 _ - . / % # @ ! , ; = | ' + ? ~space ( ) [ ] { }`" * & < > : ^ $ \The initial CSV/Create/Search matrix in the "Why" section above identified five chars that broke specific operations and recommended allowing the rest. After it was opened, additional empirical testing on a running OpenSearch instance with
extension.<propertyName>^boostconfigured insearchSettings.searchFields(the actual Explore search path) revealed that*triggers ES field-path wildcard expansion (the literal-*field never satisfies its own pattern) and silently drops the field from the query.&,<,>were tightened separately because DOMPurify on the UI auto-encodes them to HTML entities, producing inconsistent stored values. So the final blocklist is",:,^,$,\(the original five) plus*(search bypass) plus&,<,>(UI sanitization).Changes
openmetadata-spec/.../type/basic.json— addscustomPropertyNamedefinition with regex^[A-Za-z0-9][A-Za-z0-9 _\-.,;/&%#@!'(){}\[\]<>|=+?*~]*$`, max length 256.openmetadata-spec/.../type/customProperty.json—namenow$refscustomPropertyNameinstead ofentityName.openmetadata-service/.../jdbi3/TypeRepository.java— addsvalidateCustomPropertyName(String)called fromvalidateProperty(CustomProperty)so the API returns 400 with a clear, human-readable error message even if schema validation is bypassed (e.g. internal callers that don't go through the@Valid-protected resource layer).openmetadata-service/.../jdbi3/TypeRepositoryTest.java(new) — schema-vs-Java consistency tests that loadbasic.jsonat runtime and assert the pattern andmaxLengthmatch theTypeRepositoryconstants. Drift in either direction fails CI with a message naming both files.Why two layers + a sync test (instead of one)
Each layer earns its keep:
@ValidBean Validation flow.@Patternviolation (which dumps the raw regex), and runs even on code paths that buildCustomPropertyprogrammatically without crossing the HTTP boundary.Backwards compatibility
Name validation is scoped to newly added custom properties (the
addedlist computed byTypeUpdater.updateCustomProperties()viarecordListChangeagainst the entity's stored state).added, so the regex is never re-checked./types/{id}(addCustomProperty), PATCH/types/{id}(JsonPatch), or any internal call intorepository.createOrUpdate— is validated against the schema-derived@Pattern/@Size/@NotNullannotations on the generatedCustomProperty.javaand rejected with HTTP 400 if its name violates the rule.This was a deliberate scope narrowing in response to review feedback (#27808 review note): an earlier iteration validated every property in
customProperties[]on every Type write, which meant an upgraded instance with a single legacy bad-named property would have any subsequent PUT/PATCH on that Type rejected — even when the write merely updated the description of a different, valid property. Restricting validation to new additions preserves write-time backwards compatibility while still preventing any newly introduced bad name from reaching the database via any write path.Test plan
Unit tests in
openmetadata-service(TypeRepositoryTest)customPropertyNamePatternMatchesSchema— schema regex matches JavaPatterncustomPropertyNameMaxLengthMatchesSchema— schemamaxLengthmatches Java constantcustomPropertyNameDefinitionIsRequiredString— definition shape sanity checkIntegration tests in
openmetadata-integration-tests(TypeResourceIT)test_customPropertyNameAllowedCharacters_succeeds— 31 names spanning every safe character (including+,?,*,~,`)test_customPropertyNameDisallowedCharacters_fails— verifies",:,^,$,\are rejectedtest_customPropertyNameMustStartWithAlphanumeric_fails— leading char rule for_,-,.,(,<test_customPropertyNameTooLong_fails— > 256 character names rejectedtest_customPropertyNameUnbalancedBrackets_succeeds—(,),<,>,[,],{,}accepted unbalanced (no closure required)🤖 Generated with Claude Code
Summary by Gitar
getTypeByIdinTypeResourceITto ensurecustomPropertiesare populated during validation checks.This will update automatically on new commits.