[n2d4]

The default behaviour of setTimeout seems problematic. Could be used for an exploit, because code like this might not work as expected:

    const attackerControlled = ...;
    if (attackerControlled < 60_000) {
      throw new Error("Must wait at least 1min!");
    }

    setTimeout(() => {
      console.log("Surely at least 1min has passed!");
    }, attackerControlled);

The attacker could set the value to a comically large number and the callback would execute immediately. This also seems to be true for NaN. The better solution (imo) would be to throw an error, but I assume we can't due to backwards compatibility.

[arghwhat]

A scenario where an attacker can control a timeout where having the callback run sooner than one minute later would lead to security failures, but having it set to run days later is perfectly fine and so no upper bound check is required seems… quite a constructed edge case.

The problem here is having an attacker control a security sensitive timer in the first place.

[a_cardboard_box]

The exploit could be a DoS attack. I don't think it's that contrived to have a service that runs an expensive operation at a fixed rate, controlled by the user, limited to 1 operation per minute.

[arghwhat]

A minimum timing of an individual task is not a useful rate limit. I could schedule a bunch of tasks to happen far into the future but all at once for example.

Rate limits are implemented with e.g., token buckets which fill to a limit at a fixed rate. Timed tasks would then on run try to take a token, and if none is present wait for one. This would then be dutifully enforced regardless of the current state of scheduled tasks.

Only consideration for the timer itself would be to always add random jitter to avoid having peak loads coalesce.

[lemagedurage]

I don't think it's that far fetched that a developer implements a rate limiter with setTimeout, where a task can only be executed if a timeout is not already running. The behaviour in the article is definitely a footgun in this scenario.

[lucideer]

> I don't think it's that contrived to have a service that runs an expensive operation at a fixed rate, controlled by the user

Maybe not contrived but definitely insecure by definition. Allowing user control of rates is definitely useful & a power devs will need to grant but it should never be direct control.

[shawnz]

Can you elaborate on what indirect control would look like in your opinion?

No matter how many layers of abstraction you put in between, you're still eventually going to be passing a value to the setTimeout function that was computed based on something the user inputted, right?

If you're not aware of these caveats about extremely high timeout values, how do any layers of abstraction in between help you prevent this? As far as I can see, the only prevention is knowing about the caveats and specifically adding validation for them.

[lucideer]

> that was computed

Or comes from a set of known values. This stuff isn't that difficult.

This doesn't require prescient knowledge of high timeout edge cases. It's generally accepted good security practice to limit business logic execution based on user input parameters. This goes beyond input validation & bounds on user input (both also good practice but most likely to just involve a !NaN check here), but more broadly user input is data & timeout values are code. Data should be treated differently by your app than code.

To generalise the case more, another common case of a user submitting a config value that would be used in logic would be string labels for categories. You could validate against a known list of categories (good but potentially expensive) but whether you do or not it's still good hygiene to key the user submitted string against a category hashmap or enum - this cleanly avoids using user input directly in your executing business logic.

[bryanrasmussen]

>Can you elaborate on what indirect control would look like in your opinion?

although not the OP this is what I would mean by indirect control.

pseudo if userAccountType === "free" then rate = longRate

if userAccountType === "base" then rate = infrequentRate

if userAccountType === "important" then rate = frequentRate

obviously rate determination would probably be more complicated than just userAccountType

[chacham15]

I would imagine the intent behind this would be that the attacker has indirect control over the timeout. E.g. a check password input which delays you in between attempts doubling the length of time you have to wait in between each failed attempt. With this bug in place, the attacker would simply wait all the timeouts until the timeout exceeded 25 days at which point they could brute force the password check back to back.

[arghwhat]

A login back off should be capped to a number of hours rather than be allowed to grow to a month though. I also have a hard time seeing this implemented as setTimeouts for every failed login attempt instead of storing a last login attempt time and counter in a user database with a time comparison when login is called.

It’s definitely suboptimal though, even if it is documented.

[swatcoder]

That's just terrible input validation and has nothing to do with setTimeout.

If your code would misbehave outside a certain range of values and you're input might span a larger range, you should be checking your input against the range that's valid. Your sample code simply doesn't do that, and that's why there's a bug.

That the bug happens to involve a timer is irrelevant.

[ashkankiani]

You are a bad programmer if you think silently doing the wrong thing is not a bug. The right thing to do with unexpected input as the setTimeout library author is to raise an exception.

[balls187]

The wrong thing, or the undefined thing?

Feel free to make a proposal to ECMA.

[log_e]

It's in the standard library. You're a bad programmer if you don't learn the ins and outs of the standard library, or make sweeping generalizations.

[notpushkin]

Standard library is an API just like any other library. The only thing different about it is backward compatibility (which in JS is paramount and the is reason setTimeout can't be fixed directly). It is a bad design still.

[paulddraper]

> If your code would misbehave outside a certain range of values and you're input might span a larger range, you should be checking your input against the range that's valid.

What's funny is you think that about the caller of setTimeout but not setTimeout itself :)

[tourist2d]

> That's just terrible input validation and has nothing to do with setTimeout.

Except for the fact that this behaviour is surprising.

> you should be checking your input against the range that's valid. Your sample code simply doesn't do that, and that's why there's a bug.

Indeed, so why doesn't setTimeout internally do that?

[drdaeman]

> Indeed, so why doesn't setTimeout internally do that?

Given that `setTimeout` is a part of JavaScript's ancient reptilian brain, I wouldn't be surprised it doesn't do those checks just because there's some silly compatibility requirement still lingering and no one in the committees is brave enough to make a breaking change.

(And then, what should setTimeout do if delay is NaN? Do nothing? Call immediately? Throw an exception? Personally I'd prefer it to throw, but I don't think there's any single undeniably correct answer.)

Given the trend to move away from the callbacks, I wonder why there is no `async function sleep(delay)` in the language, that would be free to sort this out nicely without having to be compatible with stuff from '90s. Or something like that.

[erinaceousjones]

I think it's more likely that it's just "undefined behaviour" and up to the implementers of the JavaScript engines. Given that modern browsers do limit and throttle how much you can do with setTimeout in some situations (try to use setTimeout on a page after you've switched to a VR context! More than like 120hz and it'll just.... Not run the timeout anymore, from experience with Chrome).

The browser devs have decided it's acceptable to change the behaviour of setTimeout in some situations.

https://developer.chrome.com/blog/timer-throttling-in-chrome...

[wging]

In nodejs you at least get a warning along with the problematic behavior:

    Welcome to Node.js v22.7.0.
    Type ".help" for more information.
    > setTimeout(() => console.log('reached'), 3.456e9)
    Timeout { <contents elided> }
    > (node:64799) TimeoutOverflowWarning: 3456000000 does not fit into a 32-bit signed integer.
    Timeout duration was set to 1.
    (Use `node --trace-warnings ...` to show where the warning was created)
    reached

I'm surprised to see that setTimeout returns an object - I assume at one point it was an integer identifying the timer, the same way it is on the web. (I think I remember it being so at one point.)

[dgoldstein0]

It's return your differs between node and in a browser. If you want to type a variable to hold the return value in typescript and share that across node (eg jest tests where you might include @types/node) and the browser you need ReturnType<typeof setTimeout>, otherwise the code won't typecheck in all cases. Similar with setInterval.

[augusto-moura]

It returns an object for a long time now, I might say it was always like this actually. Don't know about very old versions

[jandrese]

One could imagine an app that doubles the wait between each failed authentication attempt could exploit this by doggedly trying until the rate limiter breaks. Maybe not the most practical attack, but it is a way this behavior could bite you.

[userbinator]

I always try to force the timeout to 0 on those really annoying download sites that try to make me wait.

Sometimes the wait is over before I find the responsible code, and sometimes it does check server-side, but that's just part of the fun...

[sfvisser]

Don’t ever use attacker controlled data directly in your source code without validation. Don’t blame setTimeout for this, it’s impolite!

[n2d4]

The problem is the validation. You'd expect you just have to validate a lower bound, but you also have to validate an upper bound.

[leptons]

It's user input, you have to validate all the bounds, and filter out whatever else might cause problems. Not doing so is a a problem with the programmer, not setTimeout.