I was looking at the code of bouncer and found it weird that the used middleware does not match the defined function [bouncer.blocked => bouncer.block]

``` bouncer.blocked = function (req, res, next, remaining) { res.send(429, "Too many requests have been made. Please wait " + remaining/1000 + " seconds."); }; // route to protect app.post("/login", bouncer.block, function(req, res) { if (LoginFailed){ } else { bouncer.reset( req ); } }); ```

So I went to check the library to understand why. The library hasn't been updated since 5 years!

https://www.npmjs.com/package/express-bouncer

I'm not sure if these security measures are up to date.

Maybe you're right in your conclusion, but for the wrong reasons. `bouncer.block` and `bouncer.blocked` are two different things. The first is the middleware, which tracks whether the user should be blocked. The second is an optionally defined custom error function, informing the user that they have indeed been blocked.

Perhaps the library hasn't been updated simply because it's complete?

This advice is not great! From the top:

Flat promise chains aren't fully general (you'll need a lot of trivial helper functions to do anything other than sequence of single operations). This is fine because you shouldn't be using nontrivial .then()s outside of utility code.

Alternate advice: always use async/await for asynchronous control flow.

Request limits, toobusy, and several other pieces of advice assume you're throwing a single node process on the internet directly. Do not do this, you probably want at least an nginx or other battle-ready front end proxy, and probably want a CDN and/or load-balancer too. Configure these correctly to not send overlarge, weird, slowloris, etc., requests through to the node processes.

The thing about express letting API attackers create exotic objects is genuinely scary, and if true makes me want to not use express anymore or at least install a middleware to drop requests that try it.

Describing anti-XSS measures as 'escaping' is a little questionable, you should be using proper tools for generating machine-readable information, not string concatenation or (shudder) interpolation. Build a DOM, convert it to text, send it out unmodified.

IP level rate limiting simply doesn't work, at least not in the trivial "just use bouncer" way described.

It's unexplained what you could possibly use object property descriptors for to obtain security benefits. They are not, in general, a security perimeter. If attacker is running code in your context, attacker has won.

uncaughtException is difficult to use correctly, error prone, and has little to offer in general. The default behavior does the right thing. uncaughtExceptionMonitor lets you do a little extra logging before crashing if you like and is less of a footgun.

Much of the rest of the advice is generic web application security recommendations.

Auto parsing query into objects of various types is scary indeed. This could break for all kinds of legitimate or malicious user input if you were not aware of it. There is an option called “query parser” which can be set to false to disable it globally.

Having a format for such data was probably a good idea but when it’s not defined by http and thus inconsistent across frameworks (php does similar parsing, but differently), and doesn’t have an easy way to access the raw string, I’d just stick to json for complex data.

Maybe the assumption is that one should use route parameters instead.

If you’re not crazy about this advice, is there an alternative resource you would recommend for securing an app stack with a nodejs backend?

Why is up level rate limiting bad?

Attackers have a practically unlimited number of IPs to use if they want to do a brute force password attack.

So how do you stop it?

In cryptography, a brute force (BF) attack should be the last resort for the attacker. Unfortunately, many designs involve poor crypto thus creating a context in which a BF attack is cheaper than other attacks.

Unless BF attacks cost you money directly (e.g. consumption based or pay-as-you-go billing) you should not aim at preventing them but rather aim at making them impractical for the attacker. In other words: computing one test should be as expensive as possible (e.g., computational cost, a waiting time, etc.) and/or the number of possibilities must be so gigantic that the attacker can't even dream of trying all options within reasonable time. Think about well-implemented access tokens: there are so many possible values that computing or guessing a valid token would likely cost a lot of energy/time.

Best course of action may vary depending on the asset you are trying to protect. For password-protected accounts, increasing the cost typically translates into inducing artificial wait times (e.g., authorising max. N tries per minute on an account) and increasing the "possibilities" would typically require ensuring users choose robust passwords (very unlikely) or use 2-factor authentication (which essentially brings you the "gigantic number of possible values").

Hope it helps.

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.

> 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?

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.

There are also:

https://nodejs.org/api/process.html#process_event_unhandledr...

And for the browser:

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.

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.

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.

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

I find the "Do not block the event loop" example terrible:

  const fs = require('fs');
  fs.readFile('/file.txt', (err, data) => {
    // perform actions on file content
    fs.unlink('/file.txt', (err) => {
      if (err) throw err;
    });
  });

This comes right after a section about "use flat promise chains" and is also vulnerable to leaving file.txt behind if the "perform actions" part throws. Much better example imho:

  const fs = require('fs/promises');
  const data = await fs.readFile('/file.txt');
  try {
    // Do something with the data
  } finally {
    await fs.unlink('/file.txt');
  }

I wouldn’t recommend NPM audit. You’ll spend more time dealing with false positives, than fixing anything of significance.

When everything is urgent, nothing is.

Filtering out a few false positives is easier than auditing every dependency from scratch by yourself, and probably less effort than explaining to your users why your site didn't include a fix for a known vulnerability.

I take your point about "npm audit" being an annoying tool to use, though, and would recommend the package better-npm-audit in its place, which allows developers to ignore specific vulnerabilities by adding entries to a config file that can be committed to the repo.

https://www.npmjs.com/package/better-npm-audit

There's some ongoing work to improve this, by reducing noise & false positives: https://github.com/npm/rfcs/pull/422

This work is very exciting. Thanks for sending me the link, greatly appreciate it!

They also suggest "Account lockout".

> The most common protection against these attacks is to implement account lockout, which prevents any more login attempts for a period after a certain number of failed logins.

Imho account lockout has a huge downside. It reveals the username to attackers, because only on correct usernames the account lockout hooks in. I just would keep it silent and throttle the requests down on any false attemp.

Lock out fake users too? Seems simple enough.

Hmm, sounds easy! :-D But isn't, you need to keep track of fake users somewhere, which is another attack vector for DoS.

Posted when? Recommending Promise chains is a security (and bug/defect) nightmare. I’m currently working in several codebases that do this and you can’t get a call stack to anything that matters. Good luck finding a vulnerability or bug with no trace of what actually happened.

Edit: “posted when” because I don’t know if this is outdated advice, but async/await introduced after Promise standardization addresses the problem.

The main points are 2 years old, with newer edits: https://github.com/OWASP/CheatSheetSeries/blame/master/cheat...

Catch-log-new error-rethrow or catch-annotate-rethrow is (unfortunately) an important part of using Promises that isn't widely taught. I've also seen some Promise replacements with better debugging (often call site carry through, so more like a stack trace) - though I like to keep it vanilla.

> Catch-log-new error-rethrow or catch-annotate-rethrow is (unfortunately) an important part of using Promises that isn't widely taught

And horribly unwieldy to have to do everywhere, causes code bloat, is easy to forget.

> I've also seen some Promise replacements with better debugging (often call site carry through, so more like a stack trace) - though I like to keep it vanilla.

I remembered Bluebird has call stack support so I looked into that yesterday. Unfortunately it doesn’t play well with sourcemaps so it’s barely an improvement.

I really think this is all a lot of trouble for very little benefit when async/await is supported basically everywhere and doesn’t have these problems. It’s not just a huge ergonomic improvement, though it’s also that.

Callbacks are still widely used in JavaScript. The "callback pyramid of doom" was horrible and we're all glad that it's sorted out through Promises and Async/Await, but functions as first-class language members remains one of the best features of JavaScript, and if a dev is not passing around functions somewhat frequently I would really question their credentials as a "modern" JS developer. Callbacks remain some of the best and most elegant solutions to a range of problems that are not at all uncommon.

In practical terms Promises just moved callbacks into .then() instead of a function parameter and encouraged the use of pre-written functions instead of anonymous inline functions. It solves a lot of legibility problems.

It was entirely possible to write nice-looking Javascript before Promises were a thing; people just didn't.