-
Notifications
You must be signed in to change notification settings - Fork 158
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
Request: Add ability to not interpret file substitution chars with arg instead of only escaping. #177
Comments
Or, could it make sense to just not interpret those special characters when |
Unchecked user input? Really? ;) I'm not sure |
I was considering that passing the user input data through Once the values are within valid JSON and passed to something that only accepts validated JSON, there would likely be no way for malicious user input to be interpreted in any other way than the intended raw values when that valid JSON is properly interpreted later. |
I think this kind of usage to take user input, encapsulate it in JSON, and then pass to Another scenario could be a password needing to be specified within JSON data to be passed to These are just a couple quick examples I've found, but I'm sure there are more out in the wild. |
You have convinced me with that example.
Indeed; I think we shouldn't mess with that. That would leave us with a new option to be implemented, something like @gromgit what do you think? |
I think that would be the most expedient and easily-understood solution, but I should sleep on it first, in case there are corner cases that haven't come to mind.
That's already the case:
|
Ah, very cool. I should have tested my assumptions on that! Making In fact, after considering that this issue could happen and before posted the request, I made sure that |
Yeah, I'm actually thinking that the current behavior is a bug, and bugs need fixing. 😀 |
Current implementation turns `-s a=null` into `{"a":""}` instead of `{"a":"null"}`, and interprets file inclusion metacharacters at the beginning of values, e.g. `-s a=@file` returns `{"a":"<contents of file>"}`, but users would generally expect `{"a":"@file"}` instead. Fixes jpmens#177.
While I'm fixing this string coercion bug, it would be good to address the behavior of the other coercion options. Specifically, if
@jpmens, your thoughts? |
@gromgit why would Today we have: $ jo -n a=12345
{"a":12345}
$ cat /tmp/file
12345
$ jo -n a=@/tmp/file
{"a":"12345"} |
I hesitate to bring this up because it would break existing functionality, but I'm wondering if it might make more sense for this
For file contents, it seems like it would be intuitive to the user for all the regular type casting to take place just the same as if those exact content were specified on the command line. (It does seem odd that currently But also, the This way, there would be no ambiguity or conflict when only I think it may also add some level of security in that |
In fact, if A file containing JSON contents would always be interpreted as JSON unless |
Clearly, this is actually a bit more of a complex issue that I first considered. This discussion has just got me looking at the big picture now. I don't mean to muddy the waters with the idea of replacing the metacharacters with new coercion args, and I totally understand if that's out of the scope of what you want to consider doing right now (or ever) since it would break existing functionality. |
I was thinking that only having But that was before I went to bed. Now, I'm wondering what I was thinking. 😀 That said, if file inclusion is still intended to work with
is wrong, and should work like this instead:
|
To summarize, current behavior is to include file as a string, regardless of coercion flags. And here's how I think it should work:
I can't think of any other use case that makes sense. |
Sorry, just realized I'm over-complicating things...
There's no need to preprocess or escape the raw values themselves. Use bash's quoting mechanisms to your advantage:
What is currently broken is the orthogonality between file inclusion and coercion, which prevents stuff like this from working:
because file inclusion currently short-circuits before coercions can be applied. I propose to fix it by first canonicalizing the value:
then applying any coercions on the canonical value. In my mind, that makes things consistent and minimizes unpleasant surprises. |
So if the first char is a With some very quick tests, this behavior seems to work properly in the current version:
BUT, this does reveal that
In this case, I would have expected the output to be And the fact that these special characters are only special when they are the first char makes
Having to escape special chars in values at all feels cryptic and unintuitive to me for a tool like this and somewhat detracts from the ease of being able to trust that Still, after sitting with this for a while it feels like metachars are not the most intuitive way to implement file reading features for consistent and reliable behavior in all scenarios. I think using arguments instead would fit that need much better with what kind of behavior is generally expected from a CLI tool. Said in another way, having metachars in values turn some possible literal values into kind of "commands". Having a value be interpreted literally or as a command without any way to disable/enable that behavior with an argument turns But, again, I understand if those kinds of large breaking changes are out of scope of what you want to do. Regardless, thank you for showing this solution that can get the job done properly! |
At the very least, this does bring me back to thinking that having a |
If some values are coming from user input, it is possible (however unlikely) that someone could input a literal string that start with
@
,%
, or:
. If this were to happen,jo
would try to interpret those values as files to read and would fail to create JSON if the file doesn't exist (or create unintended JSON if the file does exist).Currently, the only way to avoid this is to manually escape those characters with a
\
. But, if these values are coming from user input having to do any pre-processing and escaping on those values before passing them tojo
kind defeats some of the great value ofjo
being able to make valid JSON without needing to manually escape things like double quotes within values.To avoid this possible issue when values should be treated literally no matter what without anything needing to be manually escaped within the value, I'm thinking there could be a new coercion-style argument and/or a global arg to not interpret those special
jo
characters. I could see value in both a global option and a per value option depending on the scenario. I'm thinking something like-l
(for "literal") could be good candidates for this kind of option.Hopefully, this could be combined with existing coercion args so that something like
jo -- -ls "key=${user_input_value}"
could be done to always interpretuser_input_value
as a "literal string" without needing to escape any special characters in the string.Thanks so much for considering this as well as for the amazingly useful
jo
!The text was updated successfully, but these errors were encountered: