fix(point): validate newlines on write_line_protocol - #49
Conversation
6100a9a to
63f4539
Compare
|
@karel-rehor please take a look to this |
|
For consideration - AI (5.6-Sol & Opus 5) converged on these potential issues:
|
63f4539 to
105d09f
Compare
|
thanks for the replies; rebased to latest on main, added tests & adjusted changelog @alespour point 1 & 2 highlighted by the LLM is not necessarily relevant in the scope of the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 82.74% 83.44% +0.69%
==========================================
Files 10 10
Lines 1971 1999 +28
==========================================
+ Hits 1631 1668 +37
+ Misses 340 331 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @an-jello Thanks for catching this, the underlying bug is real and worse than the description suggests. On That said, I'd like to go with escaping rather than rejecting, to match the rest of the client family:
Rust is currently the only client doing nothing at all, so we're the outlier, but erroring would also make us the outlier in the other direction, and it's import that users have a similar experience across influxdb clients. Concretely, I'd suggest adding Plus two more the validation loop can't reach:
Because Two smaller notes if you'd like to keep iterating on this:
|
|
Let’s align all clients on the same solution: escaping characters, as already implemented in the Python, Java, C#, and JS clients. @an-jello, would you like to proceed with the same approach in Rust? If it’s too much work on your side, we can take care of it. |
|
PR for GO v3 je ready - InfluxCommunity/influxdb3-go#287 |
|
I'd probably yield this PR for your team to take care of it; It's a little bit too much for my plate right now but I'm happy that y'all finally get back to this 🙏 There are quite a few thought that occurred and manifested in me since writing the initial PR. If you don't mind some potential noise / my personal 2-cents: DetailsThis bug might be CVE-minor worthy after all; I can imagine an SQL injection-like attack but for data appends or data point alteration. (If filing for CVE / changelog, I would be happy if it could mention me or this PR 😛 /halfjoking). As an outsider to the InfluxDB internals and someone who's not familiar with InfluxDB's ecosystem, my uneducated judgement that's solely based on the Line Protocol reference doc would be that I would be against escaping under the basis of correctness; I do understand that the behavior suggested is based on familiarity and reproducibility with other clients, but Familiarity is not necessarily Correct: I view escaping You've also mentioned concerns on the TAB character ( Respectfully, y'all might have a bigger issue with correctness within the ecosystem :( On the usage of |
Proposed Changes
In line with InfluxDB 3's Line Protocol reference, this PR adds an extra validation on
Point::write_line_protocol()where fields containing newlines (\n) will throw out an error.Checklist