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

Feature Request: Command verifier callback instead of static list of commands #415

Closed
shantur opened this issue Nov 26, 2019 · 4 comments · Fixed by #585
Closed

Feature Request: Command verifier callback instead of static list of commands #415

shantur opened this issue Nov 26, 2019 · 4 comments · Fixed by #585
Assignees
Labels
enhancement New feature or request

Comments

@shantur
Copy link

shantur commented Nov 26, 2019

Hi,

As of now console needs the list of commands on creation and the commands are case-sensitive.
Although my requirement will be enough just to have case insensitive commands but i think a command verifier / provider callback will make it even more powerful.

This could enable case insensitive and option to have commands which are indexed
like instead of having all commands Command1, command2, command3 provided I can just use Command as supported command.

@linuswillner
Copy link
Owner

For one, the case-insensitive detection of commands should definitively be an option, I'll look into implementing that as soon as possible. I'll also look into some way of implementing your other suggestion of the verifier callback, although I'm significantly less sure of how I'll implement that one.

@linuswillner linuswillner added the enhancement New feature or request label Nov 27, 2019
@linuswillner linuswillner self-assigned this Nov 27, 2019
@linuswillner
Copy link
Owner

linuswillner commented May 15, 2020

It's taken me way too long to get to this one 😆 anyhow, I've finally implemented case-insensitive command name matching on the v4 branch. I'm presently looking into your other suggestion as well, namely the possibility of supplying a custom command verifier callback instead of a static list.

@shantur
Copy link
Author

shantur commented May 15, 2020

Awesome 👍

@linuswillner
Copy link
Owner

linuswillner commented May 26, 2020

Hi, after spending quite a lot of time researching the possibility of implementing your suggestion about a command validator, I have to unfortunately conclude this to be something I don't see as possible to implement at this stage.

The first reason for this is that the internal structure of the application is intricate and complicated, but it is that way for a reason - it makes the application function properly and securely. Allowing a custom command validator into the mix is almost certainly going to make the application brittle and encourage bad/lazy solutions to circumvent the (few) reasonable restrictions the application places.

The case insensitive command validation that I implemented earlier this month is one prime target for this. Due to security reasons, when using case insensitive command matching, all characters that are not letters, numbers or dashes/underscores are disallowed from command names. This is to prevent regular expression denial of service attacks, which in turn is because I have to use regexes to match command names case-insensitively (There's really no other smart way to implement case insensitivity in command names BTW; I weighed in all options I could think of).

In this event, there is almost certainly going to be a subset of people who are going to say "I don't care about opening my application up to denial of service attacks, I just want to use whatever characters I like". This is of course a completely ludicrous sentiment, but this audience would likely be one prime candidate to circumvent in-place validation procedures by using a validator callback, and I don't think I should give them a venue to use my library for doing dumb things.

Secondly, the only way to slot in this kind of behaviour (without completely gutting the entire application) would be to allow the submission of a custom validator function that hooks into the command processor and returns an object of the same shape as the one required by the application natively. This would for all intents and purposes end up just moving the location where the command object is defined from the commands prop to that validator function, which is just silly. And even then, I'd still need to implement some kind of validation on the object it returns to make sure the application won't crash and burn due to the custom validator submitting improper return values, meaning the gains from removing the extra validation procedures would be as near as makes no difference nonexistent.

Now, I mentioned that this is the only way to do this without completely gutting the entire application, implying that there is a way to do so if you gut the entire application. However, that is no exaggeration - the application would need to be entirely converted to use a completely different architecture on how commands are passed, validated and run. Essentially, I'd have to rewrite the whole library, which is not something I'm intent on doing, since I think it has a well-rounded and working structure in its current state.

In my view, bearing in mind the current application architecture, this feature would not solve enough problems to be worth the hassle of implementing it, and the implications it would give birth to would be far from positive, in stark contrast to the benefits it would bring. As such, I'm unfortunately unable to fully satisfy your feature request.

A workaround for this that I can think of is to separately construct the commands in the parent application, and then update the value of the commands prop accordingly when changes are made. This way you could still have somewhat dynamic commands - not that I'm entirely sure why you'd want to introduce these kinds of side effects in the first place, but that might just be me 😅

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

Successfully merging a pull request may close this issue.

2 participants