Skip to content

Client#168

Merged
blakebyrnes merged 4 commits intomainfrom
client
Jan 27, 2023
Merged

Client#168
blakebyrnes merged 4 commits intomainfrom
client

Conversation

@calebjclark
Copy link
Contributor

No description provided.

@@ -0,0 +1,2 @@
export type IInputFilter = Record<string, any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this "any" something we should add typing to later? Or is it meant to just be literally anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we will add typing to this later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This typing is confusing me. Does it actually narrow types that are input or output? Seems to just say, this extends basically anything, and defaults to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a placeholder. We need to download and support schemas from remote datastores.

});
}
throw ValidationError.fromZodValidation(
`The API parameters for ${command} have some issues`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the "have" because it's "The api parameterS"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? Definitely if it said, "The ${command}'s API parameters have...", but I'm not sure about the "API parameters for ${command}". The wording felt weird when I first saw the error.

const boundValues: string[] = [];

for (const field of Object.keys(request.input || {})) {
const value = request.input[field];
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a SQL injection attack vector here without checking these fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we chat about this?

@blakebyrnes blakebyrnes merged commit 0190c51 into main Jan 27, 2023
@blakebyrnes blakebyrnes deleted the client branch January 27, 2023 18:55
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.

2 participants

Comments