5 ms·

The article: "And this is what I am complaining about — that the API looks like it is safe for an untrusted user input, while it is not" It is a bit sad to rea

by rvnx 5y ago

The article: "And this is what I am complaining about — that the API looks like it is safe for an untrusted user input, while it is not" It is a bit sad to read that. Spawning applications from your application is dangerous, so you need to be careful, and at least read the documentation. This shows that a lot of developers nowadays just copy-paste code from StackOverflow, don't read the documentation and don't know the real low-level calls in a soup of unorganised abstractions and libraries. This behaviour is documented, is actually intended, and is all explained: https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback https://nodejs.org/api/child_process.html#child_process_chil... There is even a warning: "Never pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution." few lines later: "do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution." Please read the manual before you want to operate a dangerous machine.

lifthrasiir 5y ago
If people read the manual before operating a dangerous machine we wouldn't need terms like "misuse resistance". Documentation is not a substitute for safe API design.
- tialaramex 5y ago
  The eventual point of the article's shaggy dog story is even "misuse resistance resistance" in VSCode. There's an API provided which looks exactly like the API you'd provide that resists misuse but in fact it's designed in a way that enables misuse instead. Why? In this particular case they know you might need to pass a bunch of completely unrelated strings, and they know their underlying primitive can't really do that, and so they... provide code that just concatenates all your strings, as if you're a programmer who doesn't know how to concatenate strings and needs help with that. The best excuse I can imagine is somebody at VSCode assigned a junior the task of writing the trivial "escape everything properly and concatenate it" function. Except, that function isn't trivial, and the junior lacked confidence to come back with the answer "This isn't trivial after all, I need a grown-up" so they hacked up a solution that doesn't solve anything, checked it in and breathed a sigh of relief when it passed review.
matklad 5y ago
To avoid confusion for future readers: the second part of the post, the part which contains the quote, talks about VS Code `ShellExedution` API. This API is unrelated to node’s `child_process` module. The absence of clear docs on `ShellExecution` is one drawback of the API which is called out. Separately from the factual response, I want to explicitly say that I don’t endorse such style of communication. Please do not blindly assume, and then pick on, on the lack of knowledge of other people (be it author of the post or abstract lot of developers). Even when you actually find the lack of knowledge (rather than assuming it), do not scold people for it. It’s OK to not know things, we all learned them for the first time once.
- rvnx 5y ago
  You are doing well I think. My comment was more about the situation where a newbie knows that he is handling dangerous operations but still copy-paste from StackOverflow without even thinking or reading. You went one-step ahead and wrote a summary so it wasn't about your situation. The only point where I fundamentally disagree is about ShellExecution. When you send a string to the shell for execution, it seems reasonable to expect >, semi-colons, pipes, and other shell operators to be interpreted by the shell unless you escape them.
  - matklad 5y ago
    > When you send a string to the shell for execution, it seems reasonable to expect >, semi-colons, pipes, and other shell operators to be interpreted by the shell unless you escape them. Fully agree here. But the problem is the second overload, which can take an array of strings (so looks like a safe API), but concatenates them without escaping.
    - rvnx 5y ago
      Ah, ah. I wanted to say no, but after reading more and more the method they offer is actually much more evil than it seems. In the ShellExecution function, if you actually comes from C programming you can get confused: Could the parameter "string | ShellQuotedString" mean: "A string which has the binary flag ShellQuotedString on" = "A shell quoted/escaped string" +/- "a string of sub-type ShellQuotedString" which would imply that the developer has to take this responsibility. or it means "string || ShellQuotedString", as in param is a "string OR ShellQuotedString" which in this case the behaviour is unknown. (well, now I know it's the second one :| ) but it's fun to read

by rvnx 5y ago

lifthrasiir 5y ago

If people read the manual before operating a dangerous machine we wouldn't need terms like "misuse resistance". Documentation is not a substitute for safe API design.

tialaramex 5y ago
The eventual point of the article's shaggy dog story is even "misuse resistance resistance" in VSCode. There's an API provided which looks exactly like the API you'd provide that resists misuse but in fact it's designed in a way that enables misuse instead. Why? In this particular case they know you might need to pass a bunch of completely unrelated strings, and they know their underlying primitive can't really do that, and so they... provide code that just concatenates all your strings, as if you're a programmer who doesn't know how to concatenate strings and needs help with that. The best excuse I can imagine is somebody at VSCode assigned a junior the task of writing the trivial "escape everything properly and concatenate it" function. Except, that function isn't trivial, and the junior lacked confidence to come back with the answer "This isn't trivial after all, I need a grown-up" so they hacked up a solution that doesn't solve anything, checked it in and breathed a sigh of relief when it passed review.

matklad 5y ago

To avoid confusion for future readers: the second part of the post, the part which contains the quote, talks about VS Code `ShellExedution` API. This API is unrelated to node’s `child_process` module. The absence of clear docs on `ShellExecution` is one drawback of the API which is called out. Separately from the factual response, I want to explicitly say that I don’t endorse such style of communication. Please do not blindly assume, and then pick on, on the lack of knowledge of other people (be it author of the post or abstract lot of developers). Even when you actually find the lack of knowledge (rather than assuming it), do not scold people for it. It’s OK to not know things, we all learned them for the first time once.

rvnx 5y ago
You are doing well I think. My comment was more about the situation where a newbie knows that he is handling dangerous operations but still copy-paste from StackOverflow without even thinking or reading. You went one-step ahead and wrote a summary so it wasn't about your situation. The only point where I fundamentally disagree is about ShellExecution. When you send a string to the shell for execution, it seems reasonable to expect >, semi-colons, pipes, and other shell operators to be interpreted by the shell unless you escape them.
- matklad 5y ago
  > When you send a string to the shell for execution, it seems reasonable to expect >, semi-colons, pipes, and other shell operators to be interpreted by the shell unless you escape them. Fully agree here. But the problem is the second overload, which can take an array of strings (so looks like a safe API), but concatenates them without escaping.
  - rvnx 5y ago
    Ah, ah. I wanted to say no, but after reading more and more the method they offer is actually much more evil than it seems. In the ShellExecution function, if you actually comes from C programming you can get confused: Could the parameter "string | ShellQuotedString" mean: "A string which has the binary flag ShellQuotedString on" = "A shell quoted/escaped string" +/- "a string of sub-type ShellQuotedString" which would imply that the developer has to take this responsibility. or it means "string || ShellQuotedString", as in param is a "string OR ShellQuotedString" which in this case the behaviour is unknown. (well, now I know it's the second one :| ) but it's fun to read