Potential data loss when a column comment contains [SENSITIVE] #52699
ayushgade06
started this conversation in
General
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
I noticed a small issue in Studio's column sensitivity handling that can cause user-written comments to be silently modified.
Currently, Studio stores the sensitive data flag by prepending
[SENSITIVE]to the column's comment (ColumnEditor.utils.ts).The problem is that the encode/decode helpers don't check whether the marker is actually at the beginning of the comment.
There are two places where this causes trouble:
isSensitiveDataInCommentusescomment.includes('[SENSITIVE]'), so any comment mentioning the marker is incorrectly treated as sensitive.encodeCommentWithSensitivityFlagusescomment.replace('[SENSITIVE]', '').trim(), which removes the first occurrence wherever it appears, even when the sensitivity toggle is off.Reproduction
Create or edit a column.
Set its comment to:
Fields marked [SENSITIVE] are PIISave the column.
The comment gets persisted as:
Fields marked are PIIThe column is also incorrectly marked as sensitive in the editor.
For example:
This isn't an SQL injection issue since pg-meta parameterizes the comment using
literal(). The problem is silent data loss of user-authored content.Proposed fix
I think the immediate fix can be quite small:
startsWith()to detect the marker.The editor already prepends the marker when the sensitivity toggle is enabled, so this should preserve the existing behavior while avoiding accidental matches in the middle of comments.
I have a small patch and unit tests ready. The change is contained to one utility file.
One thing I'd like to discuss
The prefix fix solves the immediate problem, but there's still an edge case: a user who genuinely starts their comment with
[SENSITIVE]will still have that text interpreted as the flag.I see two possible approaches:
I'd lean towards the first approach for now, followed by considering the second separately.
Would appreciate some thoughts before I open a PR. Is there any reason to avoid the prefix-based fix as an interim solution?
All reactions