Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure values from parser are escaped #9

Open
Raynos opened this issue Feb 15, 2020 · 7 comments
Open

Ensure values from parser are escaped #9

Raynos opened this issue Feb 15, 2020 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@Raynos
Copy link
Contributor

Raynos commented Feb 15, 2020

We have an example like

const expression = 'SET $(elevation) = 46.6'
const { err, data } = await table.update('oregon', 'salem', expression)

Our users are going to do something like

const expression = 'SET $(elevation) = ' + newValue
const { err, data } = await table.update('oregon', 'salem', expression)

^ This is a DynamoDB injection waiting to happen.

We should have something like

const expression = table.expr`SET $(elevation) = ${newValue}`
const { err, data } = await table.update('oregon', 'salem', expression)

With a function that does escaping for whatever

@heapwolf
Copy link
Member

heapwolf commented Feb 15, 2020

It would actually be like this which prevents injection. The example you give with concat and template literal would throw a parse error.

const expression = `SET $(elevation) = S(${newValue})`
const { err, data } = await table.update('oregon', 'salem', expression)

all values must be typed.

@Raynos
Copy link
Contributor Author

Raynos commented Feb 15, 2020

hmm after reading the docs & source code more I think this just needs docs & tests.

I don't know if table.expr is going to be necessary but it would be more like table.parseQuery.

If we did it so that the update method took the { expression, attributeNames, attributeValues }as a parameter and the users has to doparseQuery`...`` to create such an object.

I don't know if a templat eliteral is necessary for the parseQuery() fn or if a string is enough.

@heapwolf
Copy link
Member

in theory you could do some fancy stuff with template-tags, but I think having explicit types is probably less error prone. you can still use template tags to emplace values.

@heapwolf
Copy link
Member

... N(100) ... is 1. contained 2. isolates the value from the query, 3. requires a type.

@Raynos
Copy link
Contributor Author

Raynos commented Feb 15, 2020

👍 I think that syntax is good.

We just need to add more tests to make sure theres no injection attack on our syntax.

@Raynos
Copy link
Contributor Author

Raynos commented Feb 15, 2020

For example

const newValue = '100) AND SET $(rekt) = S(100'
const expression = `SET $(elevation) = S(${newValue})`

@Raynos
Copy link
Contributor Author

Raynos commented Feb 15, 2020

basically // TODO escaping

@heapwolf heapwolf changed the title Impl & document helper function for building update expressions Ensure values from parser are escaped Feb 15, 2020
@jwerle jwerle added the enhancement New feature or request label Nov 2, 2021
@jwerle jwerle self-assigned this Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants