test: add security tests for worker authentication edge cases

[iamkrishpathak]

Description

Adds comprehensive security-focused tests for worker authentication logic, covering API key validation, request context extraction, and edge cases such as expired, revoked, and disabled workers.

Target Branch

• This PR targets dev
• This PR targets main

Related Issues

Addresses missing security test coverage for worker authentication.

Type of Change

• Bug fix
• New feature
• Breaking change
• Tests
• Documentation update

Changes Made

• Added security-focused unit tests for verify_worker_key
• Added edge case tests for _get_request_context
• Added deterministic and safety tests for hash_api_key
• Isolated tests to avoid database dependency

Testing

• Ran pytest security_tests/
• Tests pass locally
• Manual testing (not applicable)

Checklist

• Code follows project style
• No production code changes
• Tests do not require real database
• CI-friendly

Additional Notes

These tests intentionally isolate security logic and avoid PostgreSQL/Redis dependencies.

[iamkrishpathak]

All CI checks are now passing!
Added comprehensive security-focused tests for worker authentication edge cases, including hashing, request context handling, and key validation scenarios.

Happy to address any feedback or requested changes.
Thanks for the review!

[filthyrake]

Hey @iamkrishpathak! Thanks for working on security test coverage - this is a valuable area to strengthen! 🔐

I've done a thorough review and have some feedback to help get this ready for merge.

Main Issues to Address

1. Dead Code: tests/_disable_db.py

This file is created but never imported or used anywhere in the PR. The sys.modules mocking is duplicated directly in security_tests/test_worker_auth_security.py instead. Either delete this file or actually use it via import.

2. Unsafe sys.modules Manipulation

The module-level sys.modules mocking (lines 1-8) can cause test pollution:

sys.modules["api.database"] = MagicMock()

This modifies sys.modules at import time and persists across all tests in the pytest session. If other tests run alongside these, they might get the mocked module instead of the real one.

Recommendation: Use pytest fixtures with monkeypatch like the rest of the project does in tests/conftest.py:

@pytest.fixture
def mock_database(monkeypatch):
    mock_db = MagicMock()
    mock_db.fetch_one = AsyncMock()
    monkeypatch.setattr("api.worker_auth.database", mock_db)
    yield mock_db

3. Separate security_tests/ Directory

Creating a separate directory breaks project conventions:

• pytest.ini has testpaths = ["tests"], so security_tests/ won't be discovered by default
• The project convention is to put all tests in tests/

Recommendation: Move to tests/test_worker_auth_unit.py or similar.

4. Unrelated .env.example Changes

The audit log rotation settings (VLOG_AUDIT_LOG_BACKUP_COUNT, VLOG_AUDIT_LOG_MAX_BYTES) aren't related to security tests. These should be in a separate PR - especially since the functionality to read these settings isn't implemented in config.py or api/audit.py yet.

5. Missing Test: Worker Not Found

The verify_worker_key function has a code path for when the worker record doesn't exist (lines 176-187 in worker_auth.py), but there's no test for this scenario.

Minor Suggestions

1. Import at line 100 - from datetime import datetime, timedelta, timezone appears mid-file after test functions. Move to top.

2. Strengthen test_hash_api_key_not_reversible - Currently just checks key not in hashed. Could also verify hash is 64 hex characters.

3. Add docstrings - Brief descriptions of what security scenario each test covers would be helpful.

A Note on Existing Coverage

Just a heads up: tests/test_worker_api.py already has integration tests covering many of these scenarios (missing key, expired key, revoked key, disabled worker, hash consistency) using actual database fixtures. Your unit tests with mocks are still valuable for fast feedback, but you might want to check that file to avoid duplication and ensure you're adding new coverage. If you're going "but Damen, the issue called those out!" that is my bad - I need to go back through and update some issues :D

---

Looking forward to the updates! Once these are addressed, this will be a nice addition to our test suite. 🙌

[filthyrake]

Hey @iamkrishpathak,

I owe you a sincere apology. 😔

After doing a more thorough audit of our codebase, I discovered that the tests you wrote already exist in tests/test_worker_api.py. We failed to keep issue #398 updated when these tests were added, and I'm really sorry you spent time on work that was already done.

Here's what's already covered in tests/test_worker_api.py (lines 24-189):

Your Test	Existing Test
test_verify_worker_key_missing_key	test_missing_api_key (line 27)
test_verify_worker_key_invalid_key	test_invalid_api_key_format (line 33)
test_verify_worker_key_expired	test_expired_api_key (line 43)
test_verify_worker_key_disabled_worker	test_disabled_worker (line 80)
test_verify_worker_key_hash_mismatch	test_wrong_api_key_with_matching_prefix (line 100)
test_hash_api_key_deterministic	test_hash_api_key_produces_consistent_hash (line 163)
test_hash_api_key_uniqueness	test_hash_api_key_different_inputs_different_outputs (line 171)

The existing tests use proper database fixtures rather than sys.modules mocking, which is why they weren't flagged as duplicates initially.

However, your _get_request_context() tests are genuinely new and valuable! The X-Forwarded-For handling, trusted proxy logic, and IPv6 tests don't exist in our current test suite.

If you'd like to continue with this PR, I'd suggest:

1. Remove the duplicate tests (the verify_worker_key and hash_api_key tests)
2. Keep only the _get_request_context() tests (trusted proxy, forwarded-for, IPv6)
3. Move them to tests/test_worker_auth.py using proper pytest fixtures

Alternatively, if you'd prefer to close this PR given the confusion, I completely understand - and again, I'm really sorry for the wasted effort. This is entirely our fault for not maintaining the issue properly.

Thank you for your patience and for wanting to contribute to the project. 🙏

[filthyrake]

Just a quick follow-up - I've updated issue #398 to reflect the current state. It now clearly shows what's already covered and focuses specifically on the _get_request_context() tests that would still be valuable.

If you'd like to continue with this PR, the updated issue should give you a clear scope for what to keep. No pressure either way! 🙂

[filthyrake]

Hi @iamkrishpathak! 👋

I've applied the code review feedback to get this PR ready for merge:

Changes made:

• ✅ Rebased onto current dev branch
• ✅ Removed duplicate tests that already exist in tests/test_worker_api.py (verify_worker_key, hash_api_key tests)
• ✅ Kept only the valuable _get_request_context() tests (trusted proxy handling, X-Forwarded-For, IPv6)
• ✅ Moved tests to tests/test_worker_auth.py following project conventions
• ✅ Replaced sys.modules manipulation with proper pytest monkeypatch fixtures
• ✅ Removed unused tests/_disable_db.py and security_tests/ directory
• ✅ Dropped unrelated .env.example changes (skipped during rebase)

All 9 tests pass locally and CI security scans are green. Ready for merge! 🎉

Thanks for contributing these tests - the _get_request_context() coverage is a valuable addition!