Add combinator to include label in comments with generated SQL.

[thomasvnoort]

This fixes #123.

I'm not sure about the implementation of defuseComments, maybe you have some wishes on how to do that? The label should be cleaned up so that it cannot be used for SQL injections.

Also, I looked at this with @hesselink and he suggested using the CallStack implicit parameter from GHC 7.10 to automatically insert locations into labels for queries. We have implemented this at Silk and it works out pretty well! We'll actually use this once we upgrade our stack to GHC 7.10. Maybe Opaleye can use this approach too to include labels automatically?

[tomjaguarpaw]

Hi, thanks for this. Sorry for not replying sooner. I didn't see the notification. In general, if I take a long time to respond to a PR or bug report please send a message to get my attention.

Looks like it needs to be rebased, but other than that it looks like a good idea.

By the way, this

ST.replace (ST.pack "--") (ST.pack "- -")

will not work. It will replace ---- with - -- -. It might be best just to strip out everything that isn't alphanumeric.

[thomasvnoort]

No problem, I figured you were busy and didn't want to bother you with my impatience.

Regarding defuseComments, only allowing alphanumeric characters would solve the problem. But, this would also make our intended application using the CallStack implicit parameter less useful since it would obfuscate source, line, and column information. How about we replace -- (and the other comments) with an extra whitespace before and after like - - ?

[tomjaguarpaw]

I'll leave it up to you how to deal with the comments but I think there should be some tests to ensure it can't be used for SQL injections.

Will a version using CallStack compile on GHC < 7.10?

Don't worry about bothering me. If you think I've missed something please let me know. Even if I'm too busy I intend to reply to say so explicitly.

[thomasvnoort]

How should I set up these tests? I can annotate some queries with bad labels and check that they still run, but I'm not sure if that tests the right thing.

[tomjaguarpaw]

I would only test the quoting function, but how you do that is a good question ...

[tomjaguarpaw]

So in summary, yes this looks really good and will improve Opaleye. If you can reassure me about quoting the comments I'll merge it. Why is -- quoted, for example? Surely only */ can terminate a /*, or am I wrong about that?

[thomasvnoort]

You're right, only '*/' will terminate the comment that Opaleye inserts before the label. But, SQL injections might use -- to ignore anything after the label to for instance ignore a where condition that occurs after the label. This is unlikely to cause any problems other than syntax errors in the current Opaleye implementation since queries are printed on multiple lines. However, once #103 is merged this will become more likely since injecting -- would cancel the rest of the query since it's on a single line.

Therefore, defusing any comment character seems like a good idea to me. Inserting whitespace defuses the comment and by padding it with whitespace it's impossible to create new comment characters from the original, like in your earlier example.

[tomjaguarpaw]

Could you specify precisely how an attacker would do that? When you have /* blah -- whatever */ isn't the -- ignored?

Anyway, I don't mind defusing -- too.

[thomasvnoort]

You're right, it will be ignored. I can't come up with an evil example using --, as long as we defuse */. I'd rather be safe than sorry so I'd suggest defusing -- :)

Note that we do need to defuse /* since Postgres allows nested comments and having that in a label would break the query.

[tomjaguarpaw]

This is a useful patch, very well written to keep it in the overall style of the library. Very nice!

[thomasvnoort]

Thanks, happy to contribute!