-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added name check to avoid directory attack #16720
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: force_ci/object_type
Are you sure you want to change the base?
Conversation
…into tag_name_check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation for OBJECT type fields in table schemas to prevent invalid characters in device IDs, table names, and object names. The validation ensures that these identifiers do not equal '.', '..', or contain './' or '.\', which would cause issues with the file system storage backend.
Key changes:
- Adds
needCheck4Objectflag to track tables with OBJECT columns and validation methods to check for invalid patterns - Implements RPC endpoint
checkDeviceIdForObjectto validate existing device IDs when OBJECT columns are added to tables - Renames schema lock type from
VALIDATE_VS_DELETION_TABLEtoAVOID_CONCURRENT_DEVICE_ALTER_TABLEfor clarity
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| datanode.thrift | Adds TCheckDeviceIdForObjectReq struct and checkDeviceIdForObject RPC method |
| TsTable.java | Adds needCheck4Object flag and validation methods for OBJECT field restrictions |
| DataNodeTableCache.java | Sets needCheck4Object flag during initialization, pre-update, and rollback operations |
| MTreeBelowSGMemoryImpl.java | Renames storageGroupMNode to databaseMNode; adds checkTableDevice4Object traversal method |
| SchemaRegionPBTreeImpl.java | Adds stub checkTableDevice4Object method throwing UnsupportedOperationException |
| SchemaRegionMemoryImpl.java | Implements checkTableDevice4Object by delegating to MTree |
| ISchemaRegion.java | Adds checkTableDevice4Object interface method |
| TableHeaderSchemaValidator.java | Updates lock type to AVOID_CONCURRENT_DEVICE_ALTER_TABLE |
| TableDeviceSchemaValidator.java | Changes exception type from IoTDBException to IoTDBRuntimeException |
| StatementAnalyzer.java | Validates device IDs against OBJECT field restrictions during CREATE OR UPDATE DEVICE |
| ClusterConfigTaskExecutor.java | Handles OBJECT field validation errors during ALTER TABLE ADD COLUMN |
| TableConfigTaskVisitor.java | Validates table and object names when creating tables with OBJECT columns |
| SchemaLockType.java | Renames VALIDATE_VS_DELETION_TABLE to AVOID_CONCURRENT_DEVICE_ALTER_TABLE |
| DataNodeInternalRPCServiceImpl.java | Implements checkDeviceIdForObject RPC handler with lock protection |
| AddTableColumnProcedure.java | Adds checkObject method and parseStatus helper for validation when adding columns |
| AbstractAlterOrDropTableProcedure.java | Enhances TableRegionTaskExecutor with configurable exception generator |
| ClusterSchemaManager.java | Validates table and object names during column extension |
| DataNodeAsyncRequestRPCHandler.java | Registers CHECK_DEVICE_ID_FOR_OBJECT handler |
| CnToDnInternalServiceAsyncRequestManager.java | Maps CHECK_DEVICE_ID_FOR_OBJECT to checkDeviceIdForObject client call |
| CnToDnAsyncRequestType.java | Adds CHECK_DEVICE_ID_FOR_OBJECT enum value |
| IoTDBTableIT.java | Adds comprehensive integration test for OBJECT field validation |
Comments suppressed due to low confidence (1)
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:4224
- The error message parsing using string contains and replace is fragile. If the exception message format changes, this code will break silently. Consider using a specific error code in TSStatus or a more robust parsing mechanism to identify and handle OBJECT field validation errors.
ReadWriteIOUtils.write(targetName, stream);
} catch (final IOException ignored) {
// ByteArrayOutputStream won't throw IOException
}
final TSStatus tsStatus =
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String message = ""; | ||
| for (final TSStatus status : statuses) { | ||
| if (status.getCode() == TSStatusCode.SEMANTIC_ERROR.getStatusCode()) { | ||
| message = status.getMessage(); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing 'message' to empty string and then reassigning it in the loop is inefficient. Consider initializing to null and using Objects.isNull() checks, or restructure to return early when a semantic error is found, to avoid unnecessary string assignments.
...in/java/org/apache/iotdb/confignode/procedure/impl/schema/table/AddTableColumnProcedure.java
Outdated
Show resolved
Hide resolved
...rg/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java
Outdated
Show resolved
Hide resolved
...b-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/table/DataNodeTableCache.java
Outdated
Show resolved
Hide resolved
...b-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/table/DataNodeTableCache.java
Outdated
Show resolved
Hide resolved
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java
Show resolved
Hide resolved
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java
Show resolved
Hide resolved
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/schema/table/TsTable.java
Show resolved
Hide resolved
| private void checkObject( | ||
| final ConfigNodeProcedureEnv env, final String database, final String tableName) { | ||
| final Map<TConsensusGroupId, TRegionReplicaSet> relatedRegionGroup = | ||
| env.getConfigManager().getRelatedSchemaRegionGroup4TableModel(database); | ||
|
|
||
| if (!relatedRegionGroup.isEmpty()) { | ||
| new TableRegionTaskExecutor<>( | ||
| "check deviceId for object", | ||
| env, | ||
| relatedRegionGroup, | ||
| CnToDnAsyncRequestType.CHECK_DEVICE_ID_FOR_OBJECT, | ||
| ((dataNodeLocation, consensusGroupIdList) -> | ||
| new TCheckDeviceIdForObjectReq(new ArrayList<>(consensusGroupIdList), tableName)), | ||
| ((tConsensusGroupId, tDataNodeLocations, failureMap) -> { | ||
| final String message = parseStatus(failureMap.values()); | ||
| // Shall not be SUCCESS here | ||
| return Objects.nonNull(message) | ||
| ? new IoTDBException(message, TSStatusCode.SEMANTIC_ERROR.getStatusCode()) | ||
| : null; | ||
| })) | ||
| .execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check while adding a column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding a object column, we need to ensure that the directory parts does not contain illegal paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this during schema validation of insertion. No need to do so in adding columns.
If an existing device will not be inserted with object data, you do not have to fail the column addition.
Description
As the title said.
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR