-
Notifications
You must be signed in to change notification settings - Fork 128
Add option to rate limit the publish endpoint #1420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ricardo-devis-agullo
wants to merge
5
commits into
master
Choose a base branch
from
rate-limit-publish
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
57f3f3b
rate limit publish
ricardo-devis-agullo a654dcd
remove resetKey method
ricardo-devis-agullo 3caf5d3
fix tests
ricardo-devis-agullo 7b109ed
Update router.ts
ricardo-devis-agullo e666430
export ratelimitstore type
ricardo-devis-agullo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
export { default as Client } from 'oc-client'; | ||
export { default as cli } from './cli/programmatic-api'; | ||
export { default as Registry, RegistryOptions } from './registry'; | ||
export { RateLimitStore } from './types'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import { LRUCache } from 'lru-cache'; | ||
import type { RateLimitStore } from '../../types'; | ||
|
||
interface RateLimitEntry { | ||
hits: number; | ||
resetTime: number; | ||
} | ||
|
||
export default class MemoryRateLimitStore implements RateLimitStore { | ||
private store: LRUCache<string, RateLimitEntry>; | ||
|
||
constructor(maxSize: number = 1000) { | ||
this.store = new LRUCache({ | ||
max: maxSize, | ||
// TTL is handled manually in our increment logic | ||
ttl: 0, | ||
// Don't allow stale items | ||
allowStale: false, | ||
// Update age on access to maintain LRU behavior | ||
updateAgeOnGet: true, | ||
// Clean up expired entries when they're accessed | ||
dispose: (_value, _key) => { | ||
// Optional: log when entries are disposed | ||
} | ||
}); | ||
} | ||
|
||
async increment( | ||
key: string, | ||
windowMs: number | ||
): Promise<{ | ||
totalHits: number; | ||
resetTime: Date; | ||
}> { | ||
const now = Date.now(); | ||
const resetTime = new Date(now + windowMs); | ||
|
||
const existing = this.store.get(key); | ||
|
||
if (!existing || existing.resetTime < now) { | ||
// New entry or expired entry | ||
const entry: RateLimitEntry = { | ||
hits: 1, | ||
resetTime: now + windowMs | ||
}; | ||
this.store.set(key, entry); | ||
return { | ||
totalHits: 1, | ||
resetTime | ||
}; | ||
} | ||
|
||
// Increment existing entry | ||
existing.hits++; | ||
return { | ||
totalHits: existing.hits, | ||
resetTime: new Date(existing.resetTime) | ||
}; | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import type { NextFunction, Request, Response } from 'express'; | ||
import type { Config, RateLimitStore } from '../../types'; | ||
import MemoryRateLimitStore from '../domain/memory-rate-limit-store'; | ||
|
||
const DEFAULT_WINDOW_MS = 15 * 60 * 1000; // 15 minutes | ||
const DEFAULT_MAX_HITS = 100; | ||
const DEFAULT_MAX_CACHE_SIZE = 1000; // Maximum number of rate limit entries to keep in memory | ||
|
||
function defaultKeyGenerator(req: Request): string { | ||
return `${req.ip}:${req.user ?? 'anon'}`; | ||
} | ||
|
||
export default function createPublishRateLimiter(conf: Config) { | ||
const rateLimitConfig = conf.publishRateLimit ?? {}; | ||
const windowMs = rateLimitConfig.windowMs ?? DEFAULT_WINDOW_MS; | ||
const maxHits = rateLimitConfig.max ?? DEFAULT_MAX_HITS; | ||
const keyGenerator = rateLimitConfig.keyGenerator ?? defaultKeyGenerator; | ||
const skip = rateLimitConfig.skip; | ||
const maxCacheSize = rateLimitConfig.maxCacheSize ?? DEFAULT_MAX_CACHE_SIZE; | ||
|
||
// Use provided store or create memory store with configurable size | ||
const store: RateLimitStore = | ||
rateLimitConfig.store ?? new MemoryRateLimitStore(maxCacheSize); | ||
|
||
// Initialize store if it has an init method | ||
if (store.init) { | ||
store.init(); | ||
} | ||
|
||
return async function publishRateLimiter( | ||
req: Request, | ||
res: Response, | ||
next: NextFunction | ||
): Promise<void> { | ||
try { | ||
// Skip rate limiting if skip function returns true | ||
if (skip?.(req)) { | ||
return next(); | ||
} | ||
|
||
const key = keyGenerator(req); | ||
const { totalHits, resetTime } = await store.increment(key, windowMs); | ||
|
||
if (totalHits > maxHits) { | ||
// Calculate seconds until reset | ||
const retryAfter = Math.ceil((resetTime.getTime() - Date.now()) / 1000); | ||
|
||
res.set('Retry-After', retryAfter.toString()); | ||
|
||
res.status(429).json({ | ||
error: 'rate_limit_exceeded', | ||
message: 'Too many publish requests', | ||
resetTime: resetTime.toISOString(), | ||
retryAfter | ||
}); | ||
|
||
// Set Retry-After header | ||
return; | ||
} | ||
|
||
next(); | ||
} catch (error) { | ||
// If rate limiting fails, log error but allow request to proceed | ||
console.error('Rate limiting error:', error); | ||
next(); | ||
} | ||
}; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than gathering things like windowMs, maxHits, maxCacheSize, etc. on global level wouldn't it be better to leave it purely to the rate limit store?
So you could provide in config a factory function that would provide those values to the store directly. For default store you would provide those values on line 22, but not configurable.