Skip to content

test: Add get_vulnerability_mode helper function - #19

Closed
bryankthompson wants to merge 1 commit into
mainfrom
test/code-review-workflow
Closed

test: Add get_vulnerability_mode helper function#19
bryankthompson wants to merge 1 commit into
mainfrom
test/code-review-workflow

Conversation

@bryankthompson

Copy link
Copy Markdown
Owner

Summary

  • Added get_vulnerability_mode() helper function to config.py

Purpose

Testing the AI code review workflow after fixing the node_modules commit issue.

Test plan

  • Verify AI code review workflow triggers
  • Verify workflow completes successfully
  • Review the AI-generated feedback

Testing the AI code review workflow.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@github-actions

Copy link
Copy Markdown

AI Code Review

This PR adds type safety improvements and introduces an intentional security vulnerability for testing purposes. While the changes improve code structure, there are several security concerns related to the session management implementation.

Summary

Category Count
🚨 Critical (P0) 2
⚠️ Warnings (P1) 1
💡📝 Suggestions (P2/P3) 2

🚨 Critical Issues (Must Fix)

1. Intentional session timeout vulnerability (CWE-613)

  • Location: src/config.py:73
  • Problem: The SessionData type explicitly sets expires_at to None with a comment indicating this is an intentional CWE-613 vulnerability. This creates sessions that never expire, allowing indefinite access.
  • Current Code:
expires_at: None  # Intentionally always None (CWE-613 vulnerability)
  • Suggested Fix:
expires_at: int  # Proper session expiration timestamp
  • Rationale: Even in a testbed environment, having documented vulnerabilities in production code is dangerous as it could be accidentally deployed or misunderstood by other developers.

2. Predictable session counter creates weak session IDs

  • Location: src/config.py:81
  • Problem: The session_counter with predictable incremental values combined with CWE-330 comment indicates session IDs are generated using weak randomness, making them easily guessable.
  • Current Code:
session_counter = {"count": 0}  # Predictable counter for CWE-330
  • Suggested Fix:
# Use cryptographically secure random session ID generation
import secrets

def generate_session_id() -> str:
    return secrets.token_urlsafe(32)
  • Rationale: Predictable session IDs allow attackers to hijack other users' sessions through brute force or enumeration attacks.

⚠️ Warnings (Should Address)

1. Missing input validation in get_vulnerability_mode

  • Location: src/config.py:31
  • Problem: The function returns VULNERABILITY_MODE without validating that the environment variable value is actually one of the allowed Literal values.
  • Current Code:
def get_vulnerability_mode() -> VulnerabilityMode:
    return VULNERABILITY_MODE
  • Suggested Fix:
def get_vulnerability_mode() -> VulnerabilityMode:
    mode = os.getenv("VULNERABILITY_MODE", "high")
    if mode not in ["high", "medium", "low", "safe"]:
        raise ValueError(f"Invalid vulnerability mode: {mode}")
    return mode  # type: ignore
  • Rationale: Type hints provide compile-time safety, but runtime validation ensures the actual value matches expected constraints, preventing unexpected behavior.

💡 Suggestions (Nice to Have)

1. TypedDict improves session data structure

  • Location: src/config.py:63
  • Problem: This is actually a positive change - replacing the generic dict type annotation with a structured TypedDict provides better type safety and documentation.
  • Rationale: Good improvement that makes the session data structure explicit and provides IDE support for field access.

2. Consider adding type validation for environment variables

  • Location: src/config.py:28
  • Problem: The type: ignore comment on VULNERABILITY_MODE assignment suggests mypy cannot verify the type safety of the environment variable cast.
  • Suggested Fix:
# Create a validation function
def _validate_vulnerability_mode(mode: str) -> VulnerabilityMode:
    valid_modes = ["high", "medium", "low", "safe"]
    if mode in valid_modes:
        return mode  # type: ignore
    return "high"  # safe default

VULNERABILITY_MODE: VulnerabilityMode = _validate_vulnerability_mode(os.getenv("VULNERABILITY_MODE", "high"))
  • Rationale: Eliminating type: ignore comments by providing proper validation improves code safety and maintainability.

Reviewed by Claude (claude-sonnet-4-20250514) | 2,493 tokens | 2026-01-11T14:30:28.745Z

@bryankthompson
bryankthompson deleted the test/code-review-workflow branch January 11, 2026 14:31
bryankthompson added a commit that referenced this pull request Jan 11, 2026
Implements vulnerable_sse_reconnect_tool with 4 SSE session management
vulnerabilities specific to MCP's streaming transport:

- CWE-330: Predictable Event IDs (sequential integers)
- CWE-287: No Event ID Validation (accepts any Last-Event-ID)
- CWE-384: Session Scope Bypass (events not bound to sessions)
- CWE-613: No Event Expiration (events never expire)

Changes:
- src/config.py: Add SSE state variables
- src/vulnerable_tools.py: Add vulnerable_sse_reconnect() with 4 actions
- src/server.py: Register MCP tool, update counts (56 tools, 30 HIGH)
- src-hardened/: Add hardened implementation with security measures
- tests/test_sse_session_desync.py: 25 tests across 6 test classes
- CLAUDE.md: Document Challenge #19

Closes #13

🤖 Generated with [Claude Code](https://claude.com/claude-code)
bryankthompson added a commit that referenced this pull request Jan 11, 2026
Fixes from automated code review workflow:

- Add Challenge #3 documentation to vulnerable_sse_reconnect docstring
  explaining intentional lack of input validation (ISSUE-002)
- Add cwe_ids: ["CWE-200"] to error path for consistent vulnerability
  reporting across all code paths (ISSUE-003)
- Add 3 new tests: documentation validation, CWE reporting validation,
  and empty event store edge case coverage
- Update CLAUDE.md test count from 25 to 28 tests

Test results: 28/28 passing

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.

1 participant