search

opendcs / rest_api

Rest API that serves OpenDCS database objects as JSON
Apache License 2.0
1 stars 3 forks source link

DbInterface.java uses sequence name dynamically in queries. #108

Closed wjonassen closed 9 months ago

wjonassen commented 9 months ago

Describe the bug For simplification, getKey and resetKey pass the table/sequence name as a parameter and builds the sequence query based on that variable. We might want to verify that the sequence actually exists to prevent a possible sql injection. NOTE - Currently, the only use of this is to not ever pass user input into this function. This might be a better solution, as getting all of the sequence names is inefficient.

To Reproduce Steps to reproduce the behavior: On DbInterface.java, you can see in the resetKey and getKey functions, it is building a dynamic sql query, passing in the sequence name. String q = isOracle ? ("SELECT " + seqname + ".nextval from dual")

Expected behavior Solutions: 1) Check that the sequence name exists before running the query.

2) Make sure user input is never passed into this function. (preferred method)

3) Create new functions for each sequence.

MikeNeilson commented 9 months ago

We really should start using auto sequence and the available insert ... <table> values(...) returning <key column> that should be able in all databases these data.

But that would require getting the main code to do the same thing at the same time so it'll probably have to wait.

wjonassen commented 9 months ago

For now, would you recommend we should run the check to verify each sequence exists, or just rely on the code only passing non -user input? Both have their drawbacks, but I think querying for all sequences is a little inefficient.

As a side note - I can create a new issue to track your comment above.

MikeNeilson commented 9 months ago

Yeah, I think that it's a reasonable sanity check. especially in case another developer violates the rule of non-user input.

wjonassen commented 9 months ago

After speaking with Peter and Adam, we thought it might be a good idea to use a similar approach, but create an enum (of valid sequences) that forces the developer to adhere to the rule of non-user input.

MikeNeilson commented 9 months ago

Hmm, that sounds better; certainly less possible performance issues waiting for a database roundtrip.