-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tweaks for MTC enhancements #200
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #200 +/- ##
===========================================
+ Coverage 57.5% 57.57% +0.06%
- Complexity 800 808 +8
===========================================
Files 144 145 +1
Lines 7314 7314
Branches 850 851 +1
===========================================
+ Hits 4206 4211 +5
+ Misses 2769 2763 -6
- Partials 339 340 +1
Continue to review full report at Codecov.
|
@@ -562,24 +600,12 @@ private void setFieldToNull(boolean postgresText, String[] transformedStrings, i | |||
* | |||
* TODO add a test including SQL injection text (quote and semicolon) | |||
*/ | |||
public String sanitize (String string) throws SQLException { | |||
public static String sanitize (String string, SQLErrorStorage errorStorage) { | |||
String clean = string.replaceAll("[^\\p{Alnum}_]", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this regex. Can you make a comment for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abyrd, can you provide a description of this? (I can add your comments to code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the undocumented regex, obviously these things should always have a comment. This removes all characters that are not alphanumeric or an underscore. The reasoning being that all GTFS field names are composed of only alphanumeric characters and underscores, and limiting all supplied strings to those characters should completely preclude SQL injection.
Since this is already merged, I think it would be OK to make a commit directly to dev to add this information in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a few more comments, and I think it'd be good to move the mentioned methods out of the JdbcGtfsLoader table to either the Table or Field class.
@evansiroky, thanks for the suggestions. I've addressed the comment gaps and moved methods to Table/Field. I don't know exactly what the regex does, but I've tagged @abyrd who wrote that method originally. If you'd like we can wait for his comments before merge. |
🎉 This PR is included in version 4.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR modifies and makes static/public a few internal methods for
Table
andJdbcTableLoader
that are used in ibi-group/datatools-server#186.