Skip to content

Conversation

@asage-me
Copy link
Contributor

@asage-me asage-me commented Oct 2, 2017

Fixes SQL injection.

Added SQL Mode and SQL Statement entry to the config. SQL Mode allows selection of how the SQL statement is provided to the node. msg.topic operates as the node currently operates. Normal uses the SQL statement entered on the config. Prepared uses the statement entered on the config and passes in parameters from msg.params. SQL Statement is a ACE editor for SQL Queries.

@jsf-clabot
Copy link

jsf-clabot commented Oct 2, 2017

CLA assistant check
All committers have signed the CLA.

@knolleary
Copy link
Member

Hi, thanks for raising a pull request.

If you check out our contribution guidelines you'll see we prefer feature request/suggestions are first discussed on the mailing list or slack team. Note we also reserve the right to close PRs that have failed to follow our contribution guidelines.

This PR does not include any information about what it is adding. The one line title is not enough for us to understand what it is doing.

Please describe in detail what functionality this adds, what problems it solves etc.

Thanks!

@asage-me
Copy link
Contributor Author

asage-me commented Dec 1, 2017

Any updates on this PR?

@hardillb
Copy link
Member

hardillb commented Dec 2, 2017

@atsage the edited description is still not really enough, you need to explain what problems it is solving. Also as noted these changes should have been discussed on the mailing list or slack first.

Also before any PR can be accepted you MUST sign the CLA.

Copy link
Member

@dceejay dceejay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also - looks like this would break all existing versions of this node unless defaulted correctly.

if (this.sqltype == "msg.topic"){

else {
msg.payload = row;
node.send(msg);
if (this.sqltype == "msg.topic"){
Copy link
Member

@dceejay dceejay Dec 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will break all previous version of the node unless defaulted correctly.

@asage-me
Copy link
Contributor Author

asage-me commented Mar 1, 2018

Hey guys, finally came back around to finish this up. I defaulted to msg.topic if nothing is set to keep backward compatibility. Not sure what else to add to the description of what this PR does, the first post in the thread covers everything. It's an enhancement to prevent SQL injection by providing prepared statements.

@dceejay dceejay merged commit babff3f into node-red:master Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants