chore: dbconn - add requestId info as a comment in the database logs #3110
Conversation
it is pending to add requestId to prepared statements
|
You can find the image built from this PR at Built from 1939ba4 |
Ivansete-status
requested review from
gabrielmer,
richard-ramos and
NagyZoltanPeter
| @@ -224,7 +224,8 @@ proc dbConnQuery*( | |||
|
|
|||
| var queryStartTime = getTime().toUnixFloat() | |||
|
|
|||
| (await dbConnWrapper.sendQuery(query, args)).isOkOr: | |||
| let reqIdAndQuery = fmt"/* requestId={requestId} */ " & $query | |||
There was a problem hiding this comment.
Are we 100% sure that no-one can pass harmful - sqj inject - string with requestId string?
Concatenating it does not seem very secure unless we take special care.
Also wonder if fmt cannot throw exception?!
There was a problem hiding this comment.
Are we 100% sure that no-one can pass harmful - sqj inject - string with requestId string? Concatenating it does not seem very secure unless we take special care.
Also wonder if fmt cannot throw exception?!
Good points! Thanks! I tackled them in c065116
| @@ -207,13 +207,42 @@ proc waitQueryToFinish( | |||
|
|
|||
| pqclear(pqResult) | |||
|
|
|||
| proc containsRiskyPatterns(input: string): bool = | |||
There was a problem hiding this comment.
Really awesome! I think next time we need to generalize this to apply wherever we think needed. WDYT?
Just an idea, usually what SQL inject attacker use is to trick the comments inside an SQL string, so that must be banned first (sometimes it is just enough to filter out threats).
Ivansete-status
force-pushed
the
request-id-in-queries
branch
from
bcd3099 to
c065116
Compare
Addition of more debug information to the database logs.
It is pending to add requestId to prepared statements and that would require deeper refactor.
Let's see if is enough with that to fine-tune the arbitrary queries