4 ms·

As always very good content from owasp. However, I'm a bit confused by the first one, "use flat Promise chains": it doesn't feel like a question about security

As always very good content from owasp. However, I'm a bit confused by the first one, "use flat Promise chains": it doesn't feel like a question about security, but more about code readability. While a good practice to use promises (and maybe async/await instead of then), it feels much like saying use switch instead of if/then/else because it's less prone to errors.

lucideer 5y ago
The implication here seems to be that comprehensive error handling & logging improves security. Silently swallowing errors is a common issue in async code and that can be a major risk if those errors are indicative of security holes (or even active attack attempts). The author seems to believe flat chains make it easier / more likely that errors will be caught and handled. That's probably true, it does seem easier to insert error handling code with flat promises, but it's certainly not done automatically. Would be good if it was clearer on that point and to see more hard examples of actual error handling.
- dgb23 5y ago
  There are also: https://nodejs.org/api/process.html#process_event_unhandledrejection https://nodejs.org/api/process.html#process_event_unhandledr... And for the browser: https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event https://developer.mozilla.org/en-US/docs/Web/API/Window/unha... Not entirely sure of their utility though. But can be elegant or robust if applied correctly.
capableweb 5y ago
Although I think it's a weak argument, the argument they seem to make is that it's easier to screw up doing callbacks in a nested way when it comes to correct flow + error handling, as compared to a flat flow of promises to execute. At least that's what I understood from "Promises provide a higher assurance of capturing and handling errors" which seems to be the only argument in that snippet.
joshxyz 5y ago
i think custom error types (extended from Error) wrapped in try catch is a good alternative kn that.
chrismorgan 5y ago
> As always very good content from owasp. I have a severely jaded view of OWASP content: I feel it’s very hit and miss; never excellent, and often mediocre. They do have some decent stuff, but historically I’ve found it regularly quite low-quality; my favourite example is that their XSS stuff is quite badly written, utterly incomplete, severely misleading in places, and recommends doing some things that are completely unnecessary, with no explanation of why except in one place where it provides an incomplete citation to a spec from at least 15 years ago (section number but no mention of even what spec), on a point where that spec was actually at fault and didn’t reflect what anyone ever did (I think that was about the ' character reference in XML). Until the end of last year it said to escape / as / with no explanation, so that got cargo-culted, and they resisted removing it largely because there’s got to be some reason why it’s been there for at least a decade, right?
kemitchell 5y ago
This was my first thought, as well. They never draw any straight, solid line from flat promise chains to security. It's at best a "my preferred style encourages secure coding practices" form of argument, which is a weak form of argument, especially to put at the top of the page. Their callback-based async code examples also stand like straw men. Use some utility packages. async. run-{parallel,parallel-limit,series,auto}. When I hear complaints about error handling and callbacks, it's usually the opposite of what I read here. You end up with error-first parameters to every callback for every async operation. Linters like Standard will yell at you if you fail to handle an argument called "error". There's no chaining a bunch of promises with one `catch`, or wrapping a bunch of await calls in one `try`. The utility packages themselves take error-first callbacks.

lucideer 5y ago

The implication here seems to be that comprehensive error handling & logging improves security. Silently swallowing errors is a common issue in async code and that can be a major risk if those errors are indicative of security holes (or even active attack attempts). The author seems to believe flat chains make it easier / more likely that errors will be caught and handled. That's probably true, it does seem easier to insert error handling code with flat promises, but it's certainly not done automatically. Would be good if it was clearer on that point and to see more hard examples of actual error handling.

dgb23 5y ago
There are also: https://nodejs.org/api/process.html#process_event_unhandledrejection https://nodejs.org/api/process.html#process_event_unhandledr... And for the browser: https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event https://developer.mozilla.org/en-US/docs/Web/API/Window/unha... Not entirely sure of their utility though. But can be elegant or robust if applied correctly.

capableweb 5y ago

Although I think it's a weak argument, the argument they seem to make is that it's easier to screw up doing callbacks in a nested way when it comes to correct flow + error handling, as compared to a flat flow of promises to execute. At least that's what I understood from "Promises provide a higher assurance of capturing and handling errors" which seems to be the only argument in that snippet.

joshxyz 5y ago

i think custom error types (extended from Error) wrapped in try catch is a good alternative kn that.

chrismorgan 5y ago

> As always very good content from owasp. I have a severely jaded view of OWASP content: I feel it’s very hit and miss; never excellent, and often mediocre. They do have some decent stuff, but historically I’ve found it regularly quite low-quality; my favourite example is that their XSS stuff is quite badly written, utterly incomplete, severely misleading in places, and recommends doing some things that are completely unnecessary, with no explanation of why except in one place where it provides an incomplete citation to a spec from at least 15 years ago (section number but no mention of even what spec), on a point where that spec was actually at fault and didn’t reflect what anyone ever did (I think that was about the ' character reference in XML). Until the end of last year it said to escape / as / with no explanation, so that got cargo-culted, and they resisted removing it largely because there’s got to be some reason why it’s been there for at least a decade, right?

kemitchell 5y ago

This was my first thought, as well. They never draw any straight, solid line from flat promise chains to security. It's at best a "my preferred style encourages secure coding practices" form of argument, which is a weak form of argument, especially to put at the top of the page. Their callback-based async code examples also stand like straw men. Use some utility packages. async. run-{parallel,parallel-limit,series,auto}. When I hear complaints about error handling and callbacks, it's usually the opposite of what I read here. You end up with error-first parameters to every callback for every async operation. Linters like Standard will yell at you if you fail to handle an argument called "error". There's no chaining a bunch of promises with one `catch`, or wrapping a bunch of await calls in one `try`. The utility packages themselves take error-first callbacks.