privilege: remove any string concat #22523
Conversation
| @@ -357,12 +373,7 @@ func (p *MySQLPrivilege) LoadRoleGraph(ctx sessionctx.Context) error { | |||
|
|
|||
| // LoadUserTable loads the mysql.user table from database. | |||
| func (p *MySQLPrivilege) LoadUserTable(ctx sessionctx.Context) error { | |||
| userPrivCols := make([]string, 0, len(mysql.Priv2UserCol)) | |||
There was a problem hiding this comment.
I would like to keep this dynamic array. Won't we add more privUserCol in the future?
There was a problem hiding this comment.
In theory actually it should become rare. The new privileges added to MySQL are usually added with something called DYNAMIC privileges. We have a spec for adding it here: #22486
We have already extended the privileges table prior to add ad-hoc DYNAMIC privileges. I don't think we need to migrate these yet, but it does actually break MySQL compatibility since a mysqldump becomes non-restorable.
There was a problem hiding this comment.
It makes sense to me, though. I wonder that there should be a comment at the mysql.Priv2UserCol to remind the update of tidb.
It is also rare that internal column names will cause problems, even if they are using string concat. The new API could handle the problem, too.
There was a problem hiding this comment.
Lets see what @tiancaiamao thinks. I am happy to take either approach, but prefer the comment if he is okay with that.
|
LGTM |
|
/merge |
|
@bb7133 Oops! This PR requires at least 2 LGTMs to merge. The current number of |
|
/run-all-tests |
|
Please resolve conflicts, rest LGTM |
|
I've fixed this to work against the changes in master. |
|
/run-check_dev |
|
/merge |
ti-srebot
added
the
status/can-merge
|
/run-all-tests |
|
(The PR needs approval before merge because I pushed new changes.) |
|
/merge |
|
/run-all-tests |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-3.0 in PR #22688 |
|
cherry pick to release-3.1 failed |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-4.0 in PR #22689 |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-5.0-rc in PR #22690 |
What problem does this PR solve?
Part of https://github.com/pingcap/tidb-test/issues/1152
What is changed and how it works?
What's Changed:
This removes any string concatenation from the
privilegepackage, and changes it to useExecuteInternal. Because the privilege package loads whole tables and does not need any paramaterized SQL, it should work before/after #22499 is merged.There was no risk of injection from the previous usage, since the concat did not include any user-supplied data. But it's possible a PR in the future may misunderstand this and introduce a security issue. The new approach is to define the sqls as
const. Thus, temptations are reduced.Related changes
Check List
Tests
Convered by existing tests.
Side effects
Release note