Models actions from v1 to v2 protocol #76

NRHelmi · 2022-09-19T13:20:00Z

Use v2 protocol for model actions

src/api.jl

NHDaly · 2022-09-22T18:43:29Z

src/api.jl

-    tx = Transaction(ctx.region, database, engine, "OPEN"; readonly = false)
-    actions = [_make_delete_models_action([model])]
-    return _post(ctx, PATH_TRANSACTION; query = query(tx), body = body(tx, actions...), kw...)
+    return exec(ctx, database, engine, """def delete:rel:catalog:model["$(model)"] =rel:catalog:model["$(model)"]"""; readonly=false, kw...)


This is actually a security vulnerability, since the name model may include injection attacks.

It's tricky to get this right. It used to be that this was sufficient: $(repr(model)) instead of "$(model)", but now there will still be issues if the name includes %` since this an interpolation character in Rel.

I think the right answer is that the SDK needs to provide an escape_string_for_rel(str) function.

I filed an issue for this here: https://github.com/RelationalAI/rai-sdk-issues/issues/68

Can you maybe add that as part of this PR? Does the problem make sense?

nathan can you elaborate on how this in itself is a security vuln? i believe the vuln is if an app developer takes input directly from some UI and inserts it here without any sanitization? i agree that from a correctness pov, we should escape, but not sure about the vuln. also, didnt we agree that query args was the better way to handle this?

yeah, i think my comment predates the slack discussion where we agreed on query args as the better way to handle this. I believe i had just forgotten about that when i proposed the above.

i believe the vuln is if an app developer takes input directly from some UI and inserts it here without any sanitization?

yes, that's the vulnerability I had in mind. You're right that it's probably on the developer to sanitize the inputs if they come from users, but it seems like something we could do for them to close the potential attack vector. That's all i meant.

src/api.jl

NHDaly · 2022-09-22T18:46:59Z

src/api.jl

-    tx = Transaction(ctx.region, database, engine, "OPEN"; readonly = false)
-    actions = [_make_load_model_action(name, model) for (name, model) in models]
-    return _post(ctx, PATH_TRANSACTION; query = query(tx), body = body(tx, actions...), kw...)
+    queries = ["""def insert:rel:catalog:model["$(name)"] = \"\"\"$(models[name])\"\"\"""" for name in keys(models)]


Same here, it's very important to use $(repr(models[name])) instead of """$(models[name])""" to avoid (even accidental) injection attacks.

Otherwise, if the users' model file includes """ anywhere, this will be a syntax error. Does that make sense?

(But, like above, you'll want to use https://github.com/RelationalAI/rai-sdk-issues/issues/68 to also address % in the user's model.)

yep totally makes sense, thank you for pointing this out 🙏

test/integration.jl

NHDaly

Cool, thanks @NRHelmi! Yeah this looks great. I mainly left comments about making sure we properly escape the strings to prevent (accidental or purposeful) string injection issues.

Otherwise looks great. Thanks!

src/api.jl

NHDaly · 2022-10-03T20:56:40Z

src/api.jl

+        """ def insert:rel:catalog:model[$(_escape_string_for_rel(name))] = $(_escape_string_for_rel(models[name])) """
+        for name in keys(models)
+    ]
+    println(queries)


Is this on purpose, or just for debugging?
If we want to log this, it probably needs to be more structured?

just a left from debugging 😅

src/api.jl

test/integration.jl

NHDaly

Thanks, LGTM, minus my request for round-trip tests.

NHDaly · 2022-10-03T21:02:49Z

Also, after the discussion on slack, i'm not sure if this approach or the inputs is better, to stay consistent with the other repos. But i think since you've got something working, it's probably fine to stay with this.

Co-authored-by: Nathan Daly <nathan.daly@relational.ai>

NHDaly · 2022-10-05T19:52:57Z

src/RAI.jl


 export
-    delete_model,
+    delete_models,


If you are making a breaking change like this, we'll have to bump the version number in the Project.toml to be a breaking change.

It would probably be better to leave the old names, and mark them as deprecated, and print a deprecation warning, like we did in #61, for create_database:

rai-sdk-julia/src/api.jl

Lines 398 to 402 in d2da71d

if !isempty(engine)

@warn "DEPRECATED: Passing an `engine` is no longer required for creating a" *

" database. This will be removed in a future release. Please update your call" *

" `create_database(ctx, name)`."

end

In this case, it'd be even easier, you just keep the old method, print the deprecation warning, and forward to the new method.

Then we can delete these when we bump the major version number in a future release. Does that make sense?

Or if you'd rather not do this now, you can just bump the major version number now, and make this a breaking change, but I would check with @torkins and @bradlo about whether we have the appetite for a breaking release here, or if it'd be better to do this via deprecation warning + new function. I'd personally rather go the non-breaking approach.

technically, we are only required to bump the version when we release a version. as for leaving deprecated entries, its really a judgement call .. given the number of breaking changes we are and will be making, there is real overhead to doing this, otoh if its a widely used entry point, then it may be worth it. and if its obscure, it may not. the only hard requirement is that all feature adds and breaking changes are noted in the CHANGEME.md file.

NHDaly

👍 awesome, thanks. My last comment is about the breaking change. sorry i didn't notice that earlier. The rest of the changes look good, thanks for addressing the comments

NHDaly · 2022-10-06T18:28:14Z

src/RAI.jl

    get_model,
    list_models,
-    load_model
+    load_models


Also, related to the above, can you add a line to the CHANGELOG?

bradlo · 2022-10-11T15:54:16Z

src/api.jl

+function load_models(ctx::Context, database::AbstractString, engine::AbstractString, models::Dict; kw...)
+    queries = []
+    queries_inputs = Dict()
+    rand_uint = rand(UInt32)


why 32 bits and not 64 (natural word length)

bradlo · 2022-10-11T15:57:58Z

src/api.jl

+
+function get_model(ctx::Context, database::AbstractString, engine::AbstractString, name::AbstractString; kw...)
+    out_name = "model$(rand(UInt32))"
+    query = """ def output:$out_name = rel:catalog:model[$(_escape_string_for_rel(name))] """


why the spaces at beginning and end of string?

NRHelmi added 2 commits September 19, 2022 14:18

moving models actions from v1 to v2

238fbd7

models integration tests

f51a545

NRHelmi requested review from NHDaly, bradlo and larf311 September 20, 2022 04:06

NHDaly reviewed Sep 22, 2022

View reviewed changes

src/api.jl Outdated Show resolved Hide resolved

NHDaly reviewed Sep 22, 2022

View reviewed changes

src/api.jl Outdated Show resolved Hide resolved

NHDaly reviewed Sep 22, 2022

View reviewed changes

test/integration.jl Show resolved Hide resolved

NHDaly requested changes Sep 22, 2022

View reviewed changes

refactoring && cleanup

e2a3e9f

NRHelmi requested a review from NHDaly September 26, 2022 13:39

remove unused _list_models

fec0161

NHDaly reviewed Oct 3, 2022

View reviewed changes

src/api.jl Outdated Show resolved Hide resolved

NHDaly reviewed Oct 3, 2022

View reviewed changes

src/api.jl Outdated Show resolved Hide resolved

NHDaly reviewed Oct 3, 2022

View reviewed changes

test/integration.jl Outdated Show resolved Hide resolved

NHDaly reviewed Oct 3, 2022

View reviewed changes

test/integration.jl Show resolved Hide resolved

NHDaly approved these changes Oct 3, 2022

View reviewed changes

NRHelmi and others added 3 commits October 4, 2022 19:37

Update test/integration.jl

01e5c6d

Co-authored-by: Nathan Daly <nathan.daly@relational.ai>

updates

0aecb71

fix

02bc1ae

NHDaly reviewed Oct 5, 2022

View reviewed changes

bradlo added type:feature component:rai-sdk-julia labels Oct 5, 2022

NHDaly reviewed Oct 6, 2022

View reviewed changes

changelog updates

8185f0d

sync with main & resolve conflicts

fab2c31

bradlo approved these changes Oct 11, 2022

View reviewed changes

addressing PR comments

00e397a

NRHelmi merged commit 7d54b71 into main Oct 12, 2022

NRHelmi deleted the hnr-models-v2 branch October 12, 2022 13:28

	if !isempty(engine)
	@warn "DEPRECATED: Passing an `engine` is no longer required for creating a" *
	" database. This will be removed in a future release. Please update your call" *
	" `create_database(ctx, name)`."
	end

Models actions from v1 to v2 protocol #76

Models actions from v1 to v2 protocol #76

Uh oh!

Conversation

NRHelmi commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NHDaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NHDaly left a comment

Choose a reason for hiding this comment

Uh oh!

NHDaly commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NHDaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NRHelmi commented Sep 19, 2022 •

edited

Loading