cluster-ui: add safesql to cluster-ui
#96286
Conversation
|
This change is |
There was a problem hiding this comment.
Hey, @THardy98! Just wanted to double-check: IIUC, this library does (and should) stop short of interpolating arguments to queries, yes? It's strictly for things like user, database, table, and column names? And everything else we'd pass along for the db driver to interpolate -- I think drawing that line limits our security exposure.
Reviewable status:
complete! 0 of 0 LGTMs obtained
THardy98
force-pushed
the
sql_api_safe_sql
branch
from
61efb06 to
f27c91a
Compare
There was a problem hiding this comment.
I believe so yes. The most common use case I've seen being adding identifiers directly to query strings:
safesql.Format(`SELECT * FROM %1.this_is_a_table_name`, safesql.Identifier(`this is my db name`))
Reviewable status:
THardy98
force-pushed
the
sql_api_safe_sql
branch
from
f27c91a to
7ad2606
Compare
Epic: none This change ports the `safesql` package from managed-service to cluster-ui. Safesql ensures that SQL literals/identifiers are properly quoted. Release note: None
THardy98
force-pushed
the
sql_api_safe_sql
branch
from
7ad2606 to
1403411
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/safesql.ts line 22 at r2 (raw file):
} // TODO(thomas): @knz says: the quoting rules for usernames are different from
are you addressing this on this PR?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/safesql.ts line 22 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
are you addressing this on this PR?
TLDR: no
This was actually an old TODO left by Peter a couple years ago (late 2020) on the corresponding safesql.go in managed-service. I decided to keep it and tag myself here as I've added this. Alternatively, I could omit the TODO altogether.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/safesql.ts line 22 at r2 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
TLDR: no
This was actually an old
TODOleft by Peter a couple years ago (late 2020) on the correspondingsafesql.goin managed-service. I decided to keep it and tag myself here as I've added this. Alternatively, I could omit theTODOaltogether.
You can keep it, np!
|
TYFR :) |
|
bors r+ |
|
Build succeeded: |
Epic: none
This change ports the
safesqlpackage from managed-service to cluster-ui. Safesql ensures that SQL literals/identifiers are properly quoted.Release note: None