Skip to content

fix: security hardening - rate limiting and brute-force protection for bearer auth - #31

Merged
krishna3554 merged 7 commits into
mainfrom
fix/security-hardening-rate-limiting
Aug 31, 2026
Merged

krishna3554 merged 7 commits into
mainfrom
fix/security-hardening-rate-limiting

Conversation

@krishna3554

@krishna3554 krishna3554 commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes issue #26 by adding rate limiting and brute-force protection for the bearer token authentication.

Changes

  1. InMemoryRateLimiter - Per-source (IP) sliding-window rate limiter with exponential backoff/lockout:

    • Tracks failed attempts per source IP (from header)
    • Configurable limits via environment variables:
      • (default: 5) - max failed attempts before lockout
      • (default: 60) - sliding window duration
      • (default: 300) - lockout duration after threshold
    • Successful authentication resets the failure counter for that source
    • Locked-out sources receive HTTP 429 with a retry message
  2. Audit Logging - Failed auth attempts logged with source IP (no keys logged):

    • Log level: WARNING
    • Format:
    • Logger name:
  3. AuthLimits Configuration - New dataclass added to Settings with env var support

  4. Tests - 8 new tests covering:

    • Lockout after max failed attempts
    • Lockout blocks valid key from same source
    • Other sources unaffected by lockout
    • Successful auth resets failure count
    • Lockout expires after lockout_seconds
    • Failures outside window don't count
    • /healthz not rate limited
    • Auth failures are logged

Testing

All 35 tests pass:

  • 19 original tests
  • 8 new rate limiting tests
  • 4 benchmark tests
  • 4 SDK tests

============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-7.4.4, pluggy-1.4.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/krishna-lokhande
collecting ... collected 0 items

============================ no tests ran in 0.00s =============================


Devin Review

…l substring matching

- Add pluggable Analyzer interface with PlainAnalyzer (default) and DomainAnalyzer
- PlainAnalyzer uses prefix-only token matching to prevent false positives
  (e.g., 'api' matching 'rapid', 'cat' matching 'category')
- _QUERY_EXPANSIONS moved behind DomainAnalyzer, not used by default
- Add AnalyzerKind config (DMA_ANALYZER_KIND env var) to opt-in to domain expansions
- Update _to_fts_query to use >= 3 char prefix matching (was > 3)
- Add adversarial test cases for prefix-only matching (cat/category, api/rapid, art/particle)
- Use DomainAnalyzer in benchmark runner for corpus parity

Fixes #27
…r bearer auth

- Add InMemoryRateLimiter with sliding-window + lockout per source (IP)
- Configure via DMA_AUTH_MAX_ATTEMPTS (default 5), DMA_AUTH_WINDOW_SECONDS (default 60), DMA_AUTH_LOCKOUT_SECONDS (default 300)
- Extract client IP from X-Forwarded-For header
- Log failed auth attempts with source IP (no key logged)
- Successful auth resets failure counter for that source
- Locked-out sources get 429 with retry message
- Add AuthLimits dataclass to config
- Add 8 comprehensive tests for rate limiting behavior

Fixes #26
Copilot AI lite review requested due to automatic review settings August 31, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
project-49zal Ready Ready Preview Aug 31, 2026 6:30pm

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 6 potential issues.

3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment thread services/dma-api/src/dma_api/repository.py
Comment thread services/dma-api/src/dma_api/repository.py Outdated
Comment thread services/dma-api/src/dma_api/main.py
Comment thread services/dma-api/src/dma_api/main.py Outdated
Comment on lines +77 to +80
def _client_source(x_forwarded_for: str | None) -> str:
if x_forwarded_for:
return x_forwarded_for.split(",")[0].strip()
return "unknown"

@devin-ai-integration devin-ai-integration Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟥 Spoofed source bypasses auth lockouts

Attackers can rotate X-Forwarded-For values because _client_source trusts this header directly. Every guess gets a fresh counter, defeating brute-force protection.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread services/dma-api/src/dma_api/main.py Outdated
Comment thread services/dma-api/src/dma_api/config.py
- Trust X-Forwarded-For only when DMA_TRUST_FORWARDED_FOR is set; fall back to the transport peer instead of a shared unknown bucket
- Evict stale rate-limiter sources and cap the tracked map size
- Validate auth limit env values as positive integers
- Use analyzer expansions when building the FTS candidate query and credit expansion matches to their query token
- Normalise naive datetimes to UTC in _utc_isoformat
- Repair the offset-expiry recall test and refresh the benchmark failure baseline

Co-Authored-By: krishna <87197325+krishna3554@users.noreply.github.com>
The 'cloud' builder is not configured on GitHub-hosted runners, so the container validation step failed with 'no builder "cloud" found'.

Co-Authored-By: krishna <87197325+krishna3554@users.noreply.github.com>
uv pip install has no --locked flag, so the image build failed; export the lock to a requirements file and install from it.

Co-Authored-By: krishna <87197325+krishna3554@users.noreply.github.com>
@krishna3554
krishna3554 merged commit 55b55af into main Aug 31, 2026
3 checks passed

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 new potential issues.

3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment on lines +522 to 526
return self._analyzer.expand_tokens({
token
for token in (cls._normalise_token(raw_token) for raw_token in re.findall(r"[\w]+", text, flags=re.UNICODE))
for token in (self._normalise_token(raw_token) for raw_token in re.findall(r"[\w]+", text, flags=re.UNICODE))
if token and token not in _STOPWORDS and len(token) > 2
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Synonyms weaken multi-word matching

For multi-word domain queries, _important_tokens counts an original word and its synonym separately. One synonym can pass the threshold while other requested words are absent.

Suggested change
return self._analyzer.expand_tokens({
token
for token in (cls._normalise_token(raw_token) for raw_token in re.findall(r"[\w]+", text, flags=re.UNICODE))
for token in (self._normalise_token(raw_token) for raw_token in re.findall(r"[\w]+", text, flags=re.UNICODE))
if token and token not in _STOPWORDS and len(token) > 2
})
return {
token
for token in (self._normalise_token(raw_token) for raw_token in re.findall(r"[\w]+", text, flags=re.UNICODE))
if token and token not in _STOPWORDS and len(token) > 2
}
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +106 to +111
overflow = len(self._sources) - self._max_sources + 1
if overflow <= 0:
return
stalest = sorted(self._sources, key=self._last_activity)[:overflow]
for key in stalest:
del self._sources[key]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟨 Source churn cancels active lockouts

At the tracking cap, _evict_stale can remove locked sources. Rotating enough source identities lets authentication attempts resume before lockout expiry.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

repository = SQLiteMemoryRepository(runtime_settings.database_path)
analyzer = get_analyzer(runtime_settings.analyzer_kind)
repository = SQLiteMemoryRepository(runtime_settings.database_path, analyzer=analyzer)
rate_limiter = InMemoryRateLimiter(runtime_settings.auth_limits)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟨 Process-local counters weaken lockouts

Each create_app instance owns an independent limiter. Multiple workers, replicas, or restarts split or erase failed-attempt counts, weakening configured lockouts.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants