Skip to content

Fix SQL injection#18

Merged
raymondfeng merged 1 commit into
masterfrom
feature/fix-sql-injection
Jan 9, 2015
Merged

Fix SQL injection#18
raymondfeng merged 1 commit into
masterfrom
feature/fix-sql-injection

Conversation

@raymondfeng

Copy link
Copy Markdown
Contributor

/to @bajtos or @ritch

Comment thread lib/mssql.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Need to escape val... This is still an issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps if (isNaN(Number(val))) { val = escape(''+val); } will do the trick?

On the other hand, I don't understand why we need to specifically handle this case, why can't we use escape(val) directly?

@bajtos

bajtos commented Jan 9, 2015

Copy link
Copy Markdown
Member

Related, though out of scope of this PR: Use parameterized queries in SQL connectors strongloop/loopback#983

@raymondfeng raymondfeng force-pushed the feature/fix-sql-injection branch from e4284b5 to a8eef61 Compare January 9, 2015 08:08
@raymondfeng

Copy link
Copy Markdown
Contributor Author

I added an escape() to handle embedded single quote.

raymondfeng added a commit that referenced this pull request Jan 9, 2015
@raymondfeng raymondfeng merged commit 1440f3d into master Jan 9, 2015
@raymondfeng raymondfeng deleted the feature/fix-sql-injection branch January 9, 2015 08:16
Comment thread lib/mssql.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Until we ditch out string concatenation in favour of parameterized commands/queries: it would be nice to move this escape function to loopback-connector/lib/sql.js so that there is only one instance of this code shared by all SQL connectors.

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.

3 participants