You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/03/08 17:21:11 UTC
[GitHub] [fineract] vorburger opened a new pull request #725: Fix audit
trails filter (FINERACT-808)
vorburger opened a new pull request #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725
As per the great analysis by Manthan Surkar (@thesmallstar) in the original PR #723, the cause of
the [FINERACT-808](https://issues.apache.org/jira/browse/FINERACT-808) bug was that "the backend would treat "UPDATE" and similar strings as SQL injection".
The root cause of that was that (IMHO..) how Fineract does SQL injection is more of a workaround (blacklisting some keywords and some heuristic checks) then how this really should be done (by using JDBC Prepared statements with arguments for all external data, instead inlined SQL).
This also lays the foundation for more like this in [FINERACT-854](https://issues.apache.org/jira/browse/FINERACT-854).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596498126
@vorburger @awasum
The API is not working with any action name filter, Status code: 500
message: "criteria column name must match [a-z]: aud.action_name ="
Note: it is working fine without any filter:
I am trying to find the bug.
Edit: The problem is with regex, trying to fix it.
thanks- Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596498126
@vorburger @awasum
The API is not working with any action name filter, Status code: 500
message: "criteria column name must match [a-z]: aud.action_name ="
Note: it is working fine without any filter:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between quote :P
check this
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between
` ` ` `
:P
check this
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between ` ` :P
check this:
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger closed pull request #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
vorburger closed pull request #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] awasum commented on issue #725: Fix audit trails filter
(FINERACT-808)
Posted by GitBox <gi...@apache.org>.
awasum commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596232540
@thesmallstar Have u been able to test this with the front end? So you pull this PR to a branch and run this with the front end to see if the issue is solved functionally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596498126
@vorburger @awasum
The API is not working with any action name filter, Status code: 500
message: "criteria column name must match [a-z]: aud.action_name ="
[image](https://user-images.githubusercontent.com/42006277/76213027-c701f280-622f-11ea-8771-acc93afce24b.png)
Note: it is working fine without any filter:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596230270
@thesmallstar I'm curious if (and how) you like this idea / approach... it's obviously very much inspired by your work in #723! If you do do like this, and are in favour of this approach, then you can "LGTM" this PR, and perhaps would be willing to close your #723 ?
If you are further interested in the general topic of security and SQL injections, you would be very very welcome to continue working in this area! Some ideas: 1. Implement the `TODO` about the `toString()` in `SQLBuilder`[`Test`] (which I didn't have time for); 2. similarly change `CenterReadPlatformServiceImpl`'s and `GroupReadPlatformServiceImpl`'s remaining use of `ApiParameterHelper.sqlEncodeString` to this approach (and then delete `sqlEncodeString`), and then more work as described in [FINERACT-854](https://issues.apache.org/jira/browse/FINERACT-854)...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596231452
I am very satisfied with this approach(I would have tested it and update).
I am willing to work on the files and issue you mentioned 😄
Thanks - Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596442936
@awasum sorry for the late response, I am bedridden due to high fever.
Will test and tell in an hour.
thanks- Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between
> ` `
:P
check this
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596498126
@vorburger @awasum
The API is not working with any action name filter, Status code: 500
message: "criteria column name must match [a-z]: aud.action_name ="
Note: it is working fine without any filter:
I am trying to find the bug.
thanks- Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596501440
Oh I see the problem, that's really easy to fix: That ATOZ RegExp in my new class SqlBuilder is much too strict.. it should allow (match) not only a-z but also . and - and _ and.. maybe a few more special characters that typically appear in column names. Do you want to try adding that to fix? Once you have it working, you could push a new PR, with my original commit, and then a 2nd Fix commit attributing your work. PS: Get well first, don't work with fever, nobody is dying ;-) because of this bug, this can wait until you're feeling better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596506753
@vorburger, yes I will fix it :)
thanks - Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596231452
I am very satisfied with this approach(I would have tested it and update).
I am willing to work on the files you mentioned 😄
Thanks - Manthan
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between quote :P
check this
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: [a-zA-Z_][a-zA-Z0-9_]*
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between ```` :P
check this:
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #725: Fix audit
trails filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-596526103
@vorburger
It turns out everything is a valid column name as long as we use it between quote
:P
check this
For our convenience though, we can limit it to this form of regex [validtablename.]validcolumnname
[ ] -> means optional, and valid table name and valid column name comes from: [here](https://stackoverflow.com/questions/4977898/check-for-valid-sql-column-name)
Regex: ^[a-zA-Z_][a-zA-Z0-9_]*$
any suggestions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #725: Fix audit trails
filter (FINERACT-808)
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #725: Fix audit trails filter (FINERACT-808)
URL: https://github.com/apache/fineract/pull/725#issuecomment-597017031
Closing as this was included in #729 (this plus a required fix).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services