[REFACTOR] Consolidate duplicate retry decorator logic into generic _retry_operation

[anushkapunekar]

Fixes #283

What changed

• Extracted common retry logic into a private _retry_operation function
• All 4 public decorators now delegate to it, preserving full backward compatibility
• retry_storage_operation correctly uses wait_exponential; others use wait_random_exponential
• No changes to public API — existing code using these decorators is unaffected

Tests

• 627 tests passing
• ruff check bindu/utils/retry.py — no errors

[raahulrahl]

PR #311 Review: Security & Authentication Improvements

Summary

This PR introduces payment security features and authentication guards, but is incorrectly labeled as a refactor. It contains significant behavioral changes and new features that require careful consideration before merging.

Classification: ❌ NOT A REFACTOR

Expected (Refactor): Code reorganization without behavior changes
Actual: New features, breaking changes, and behavioral modifications

---

Critical Issues

1. Breaking Change: Authentication Middleware Logic (🔴 HIGH SEVERITY)

File: bindu/server/applications.py:559

# BEFORE
if auth_enabled and app_settings.auth.enabled:

# AFTER  
if auth_enabled or app_settings.auth.enabled:

Impact:

• Changes from AND to OR logic
• Existing deployments with auth_enabled=False but app_settings.auth.enabled=True will suddenly enforce authentication
• No migration path provided
• Could break production systems without warning

Recommendation:

• Add deprecation warning for old behavior
• Provide migration guide
• Consider feature flag for gradual rollout
• Document the change in CHANGELOG

---

2. Hardcoded Security Secret (🔴 HIGH SEVERITY)

File: bindu/settings.py:299

payment_security_secret: str = "bindu-default-payment-key-please-override-in-production"

Security Issues:

• Hardcoded default secret is a critical security anti-pattern
• Developers may forget to override in production
• Creates false sense of security
• Violates principle of secure-by-default

Recommendation:

# Better approach
payment_security_secret: str | None = None

# In PaymentSecurity.__init__
if not secret_key:
    if os.getenv("BINDU_ENV") == "production":
        raise ValueError("PAYMENT_SECURITY_SECRET must be set in production")
    logger.critical("Using insecure default payment key - DO NOT USE IN PRODUCTION")

---

3. Unrelated File Included (⚠️ MEDIUM SEVERITY)

File: run_mortgage_agent.py (NEW)

Issues:

• Appears to be example/demo code
• No relation to security improvements
• Clutters the PR scope
• Makes review difficult

Recommendation:

• Remove from this PR
• Create separate PR for examples/demos
• Keep PRs focused on single concern

---

4. Missing Documentation (⚠️ MEDIUM SEVERITY)

New Features Added:

• ✅ PaymentSecurity class with HMAC signing
• ✅ Authentication guards on A2A endpoint
• ✅ Authentication guards on negotiation endpoint
• ✅ Permission-based authorization system
• ✅ New settings: payment_security_secret

Documentation Provided:

• ❌ No migration guide
• ❌ No security documentation
• ❌ No configuration examples
• ❌ No upgrade instructions
• ❌ No CHANGELOG entry

Recommendation:
Create documentation covering:

1. How to configure payment security
2. How to generate secure secret keys
3. Migration path from current version
4. Security best practices
5. Permission system usage

---

5. Test Changes Indicate Breaking Behavior (⚠️ MEDIUM SEVERITY)

File: tests/unit/test_applications.py:33

@pytest.fixture(autouse=True)
def _reset_auth_config():
    """Reset authentication config for each application test."""
    app_settings.auth.enabled = False  # ← Tests need to disable auth

Analysis:

• Tests now require auth to be explicitly disabled
• Indicates default behavior changed
• Existing code will break
• Not backward compatible

Similar patterns in:

• test_health.py
• test_negotiation_endpoint.py
• test_a2a_endpoint.py

---

Positive Aspects ✅

Despite the issues, there are well-implemented features:

1. Payment Security Implementation

# Good: Constant-time comparison prevents timing attacks
if not hmac.compare_digest(signature, expected_signature):
    return False, "Invalid or tampered signature"

2. Replay Attack Prevention

# Good: Timestamp validation
if age > max_age_seconds:
    return False, f"Timestamp too old: {age} seconds"

3. Comprehensive Test Coverage

• New test files for security features
• Authentication test scenarios
• Permission enforcement tests

4. Clean Code Structure

• Well-organized PaymentSecurity class
• Clear separation of concerns
• Good error messages

---

Recommendations

Option 1: Split into Multiple PRs (RECOMMENDED)

PR #311a - Refactor: Auth Middleware Cleanup

• Only pure refactoring changes
• No behavior modifications
• Documentation updates

PR #311b - Feature: Payment Security HMAC

• payment_security.py module
• HMAC signing/verification
• Related tests
• Security documentation

PR #311c - Feature: Endpoint Authentication Guards

• A2A endpoint auth
• Negotiation endpoint auth
• Related tests
• Migration guide

PR #311d - Feature: Permission-Based Authorization

• Permission checking system
• Scope validation
• Related tests
• Configuration guide

Option 2: Comprehensive Rewrite

If keeping as single PR:

1. Rename PR:

   • From: "fixing weakness"
   • To: "feat: Add payment security and endpoint authentication"

2. Add Documentation:

   • docs/SECURITY.md - Security features overview
   • docs/MIGRATION_AUTH.md - Migration guide
   • CHANGELOG.md entry
   • Configuration examples

3. Fix Security Issues:

   • Remove hardcoded secret default
   • Require explicit configuration
   • Add environment validation

4. Add Feature Flags:

   class AuthSettings:
       enforce_on_endpoints: bool = False  # Gradual rollout
       strict_mode: bool = False  # Fail if misconfigured

5. Remove Unrelated Files:

   • run_mortgage_agent.py → separate PR

6. Add Migration Path:

   # Deprecation warning
   if auth_enabled and not app_settings.auth.enabled:
       logger.warning(
           "DEPRECATED: auth_enabled parameter will be removed in v3.0. "
           "Use app_settings.auth.enabled instead."
       )

---

Security Considerations

Good Practices Implemented ✅

• HMAC-SHA256 for payload signing
• Constant-time comparison
• Timestamp-based replay protection
• Proper error messages (don't leak info)

Areas for Improvement ⚠️

• Secret key management (use secrets manager)
• Key rotation strategy
• Audit logging for auth failures
• Rate limiting on auth endpoints
• Consider JWT instead of custom HMAC

---

Testing Recommendations

Required Tests (Missing)

1. Integration tests for auth flow
2. Load tests for payment verification performance
3. Security tests for timing attacks
4. Backward compatibility tests

Existing Tests (Good)

• Unit tests for PaymentSecurity
• Authentication guard tests
• Permission enforcement tests

---

Verdict: REQUEST CHANGES

Cannot approve because:

1. ❌ Not a refactor - contains new features
2. ❌ Breaking changes without migration
3. ❌ Hardcoded security secrets
4. ❌ Missing documentation
5. ❌ Unclear scope (unrelated files)

This is important security work that deserves proper planning, documentation, and gradual rollout. Please address the concerns above before merging.

---

Suggested Next Steps

1. Immediate:

   • Remove run_mortgage_agent.py
   • Fix hardcoded secret
   • Rename PR to reflect actual changes

2. Short-term:

   • Add comprehensive documentation
   • Create migration guide
   • Add feature flags

3. Long-term:

   • Consider splitting into focused PRs
   • Add integration tests
   • Plan gradual rollout strategy

---

Reviewed by: Cascade AI
Date: March 5, 2026
Severity: High - Contains breaking changes and security concerns