fix: properly escape table name when querying for failed events #2663
Conversation
atzoum
force-pushed
the
fix.failedEvents
branch
from
d1efe2d
to
eaceb65
Compare
atzoum
force-pushed
the
fix.failedEvents
branch
from
eaceb65
to
c0c4c43
Compare
Codecov ReportBase: 43.73% // Head: 45.34% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2663 +/- ##
==========================================
+ Coverage 43.73% 45.34% +1.60%
==========================================
Files 191 287 +96
Lines 40483 47780 +7297
==========================================
+ Hits 17707 21667 +3960
- Misses 21671 24745 +3074
- Partials 1105 1368 +263
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -55,7 +56,7 @@ func (*FailedEventsManagerT) SaveFailedRecordIDs(taskRunIDFailedEventsMap map[st | |||
| } | |||
|
|
|||
| for taskRunID, failedEvents := range taskRunIDFailedEventsMap { | |||
| table := fmt.Sprintf(`%s_%s`, failedKeysTablePrefix, taskRunID) | |||
| table := `"` + strings.ReplaceAll(fmt.Sprintf(`%s_%s`, failedKeysTablePrefix, taskRunID), `"`, `""`) + `"` | |||
There was a problem hiding this comment.
I know this is probably a temporary measure. It would make more sense to perform this in failedEventsHandler() where both SaveFailedRecordIDs() and FetchFailedRecordIDs() are called.
There was a problem hiding this comment.
@chrikar The goal of this pr is to evade a security risk with respect to /v1/failed-events endpoint. SaveFailedRecordIDs function is not exposed outside. So, no risk.
There was a problem hiding this comment.
My point @chandumlg is that this conversion should happen at the handler endpoint, not the internal function.
There was a problem hiding this comment.
the HTTP handler should not know how/where failed events are stored, let alone that jobRunId needs to be escaped as if it was a postgresql identifier in the first place :)
There was a problem hiding this comment.
Oh! Apologies. I misread the changes in this pr. I thought the escaping is done on exposed functions FetchFailedRecordIDs & DropFailedRecordIDs and felt it is not necessary to modify SaveFailedRecordIDs.
Description
Using double quotes and escaping the table name when querying for failed events, until we drop support for the old endpoint.
Reference
Security