MapManager.get() Race Condition Duplicates API Requests

Affected Versions

The Vulnerability Explained

The MapManager.get(mapUid, cache) method implemented a classic check-then-act race condition. Here's the vulnerable pattern:

async get(mapUid, cache = this.client.options.cache.enabled){
    if (cache && this._cache.has(mapUid)) {
        return this._cache.get(mapUid);
    } else {
        return await this._fetch(mapUid, cache);
    }
}

The problem manifests in the gap between the else branch's entry and the await completing. Consider two concurrent calls to get('stadium01', true) when the cache is empty:

1. Thread A: this._cache.has('stadium01') returns false
2. Thread B: this._cache.has('stadium01') returns false (still empty)
3. Thread A: begins _fetch('stadium01', true)
4. Thread B: begins _fetch('stadium01', true) — duplicate request

Both threads now hit the external API for identical data. Worse, depending on _fetch's implementation and the cache's concurrency model, the two responses could race to write into this._cache, potentially causing:
- Overwrite of fresher data with stale data
- Corrupted cache entries from interleaved writes
- Memory pressure from storing duplicate objects

This is particularly damaging for a map management service where mapUid values like 'stadium01' represent large, expensive-to-fetch terrain data.

The Fix

The patch introduces promise deduplication through a _pending Map:

// Added to constructor
this._pending = new Map();

// Modified get() method
async get(mapUid, cache = this.client.options.cache.enabled){
    if (cache && this._cache.has(mapUid)) {
        return this._cache.get(mapUid);
    } else if (this._pending.has(mapUid)) {
        return await this._pending.get(mapUid);
    } else {
        const promise = this._fetch(mapUid, cache)
            .finally(() => this._pending.delete(mapUid));
        this._pending.set(mapUid, promise);
        return await promise;
    }
}

The key insight: promises are reference-equal and awaitable by multiple consumers. By storing the in-flight promise in _pending before awaiting it, subsequent callers receive the identical promise object. The .finally() cleanup ensures that once any caller's await completes (success or failure), the entry is removed, allowing retry of failed fetches.

This transforms the failure mode from "unbounded duplicate requests" to "exactly one request per concurrent batch, cleanly bounded."

Key Takeaways

• Promise-returning methods need deduplication for cache misses: Any async getter with a cache should consider whether concurrent callers for the same uncached key will spawn redundant work. The _pending Map pattern is a robust, language-native solution in JavaScript.

• Check-then-act over async boundaries is always racy: The if (!cached) { await fetch() } pattern is fundamentally unsound when multiple executions may interleave. Atomic test-and-set operations or promise deduplication are required.

• Cleanup belongs in finally, not after await: The fix uses .finally(() => this._pending.delete(mapUid)) rather than deleting after the await returns. This prevents leaked entries on fetch failures and ensures the map stays bounded even under error conditions.

• Defence-in-depth has value even for "unexploitable" patterns: The PR description notes this was "defence-in-depth at src/managers/MapManager.js:30 rather than a vulnerability I can show is exploitable." Race conditions in cache layers often manifest as subtle production issues (cost spikes, latency outliers) rather than security breaches, making proactive fixes worthwhile.

How Orbis AppSec Detected This

• Source: The mapUid parameter passed to MapManager.get()
• Sink: The _fetch(mapUid, cache) call invoked after a non-atomic cache miss check
• Missing control: No synchronization mechanism prevented multiple concurrent executions from passing the !this._cache.has(mapUid) check simultaneously
• CWE: CWE-362 (Concurrent Execution using Shared Resource with Improper Synchronization)
• Fix: Introduced a _pending Map to store in-flight promises, ensuring all concurrent requests for the same mapUid await a single shared promise

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The MapManager.get() race condition exemplifies how async JavaScript code inherits classical concurrency problems. The vulnerability wasn't exotic—just two lines of sequential logic that became interleaving hazards under load. The fix demonstrates that promise-based languages have elegant solutions: by treating in-flight requests as first-class values in a Map, the code achieves exactly-once semantics without locks or complex state machines. For developers building cached async APIs, the _pending pattern belongs in your standard toolkit.