FANNG1 commented on issue #11805:
URL: https://github.com/apache/gravitino/issues/11805#issuecomment-5628306800
Thanks @sachinnn99 for picking this up! Below is just an implementation
suggestion from my side — @mchades, I'd like to hear your thoughts as well.
**1. Validation shape**
Neither a pure blocklist nor a strict single-token pattern seems sufficient:
- Blocking only `;` and comment markers misses single-statement injection.
Both MySQL `ALTER TABLE ... MODIFY COLUMN` and PostgreSQL `ALTER COLUMN ... SET
DATA TYPE` accept comma-separated actions, so a type string like `int, DROP
COLUMN secret` would still get through.
- A strict identifier/parentheses/comma pattern would reject values we
already produce, e.g. MySQL `enum('a','b')` (quotes, now round-tripped by
`MysqlTableOperations`), `int unsigned` / `timestamp with time zone` (spaces),
and Doris `struct<a:int>` (colon).
What I have in mind is a lightweight shared check in `JdbcTypeConverter`:
1. Strip single-quoted literals first, so values like `enum('a;b')` are
allowed.
2. Require the remainder to match a small character set such as `[\w
.,:()<>\[\]]`. This rules out `;`, `--`, `/*`, `#`, `=`, newlines, and unclosed
quotes.
3. Require balanced `()` and `<>` (tracked separately and never going
negative), and reject commas outside of brackets.
The goal is to keep the string confined to the column's type position, not
to verify it is a legal type — the database will reject invalid types anyway.
**2. Rejection point**
I'd validate in `fromGravitino`. What counts as a valid type is
dialect-specific, so the API layer can't judge it generically, and
`fromGravitino` is the choke point before any DDL is built. Throwing an
`IllegalArgumentException` that includes the offending type string should be
enough.
**Tests and scope**
- Please add round-trip tests for the legitimate types above, plus negative
cases: statement separators, comments, top-level commas, and prematurely closed
brackets.
- The contrib converters for ClickHouse, OceanBase, and Hologres return
`catalogString()` the same way. They could reuse the helper in this PR or in a
follow-up.
@mchades WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
