test(middleware): test invalid charsets and trailing newlines in X-Request-ID (#501)

[SuparvaCode]

Description

Validates client-supplied X-Request-ID headers against length (1-128 chars) and permitted correlation ID character set (^[A-Za-z0-9._:-]{1,128}$). If missing, empty, overly long, or containing invalid characters, a freshly generated 16-character hex UUID is used instead.

Related Issue

Closes #501

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to change)
• 📚 Documentation update
• 🔧 Maintenance (refactoring, dependencies, CI, etc.)
• 🧪 Test improvement

Changes Made

• Defined _VALID_ID = re.compile(r"^[A-Za-z0-9._:\-]{1,128}$") in server/core/middleware.py.
• In RequestIDMiddleware.dispatch, validated supplied = request.headers.get(_HEADER, "") against _VALID_ID.fullmatch(supplied), falling back to uuid.uuid4().hex[:16].
• Added unit tests in tests/test_middleware.py covering:
  • Generation of 16-char hex ID when header is absent.
  • Acceptance and propagation of valid correlation IDs.
  • Replacement of overly long IDs (> 128 characters).
  • Replacement of IDs with invalid characters (spaces, newlines, injection attempts, empty string).
• Added sibling integration regression test in tests/integration/test_edge_cases.py verifying overly long IDs are replaced.

Testing

• Unit tests pass locally
• Integration tests pass locally (requires local PostgreSQL)
• Manual testing completed
• New tests added for new functionality

Test Commands Run

pytest tests/test_middleware.py -v
pytest tests/test_config.py tests/test_tracing.py tests/test_runtime_imports.py -v
pytest tests/test_route_limits_invariant.py tests/test_no_raw_tokenization.py -v
ruff check server/ tests/
ruff format --check server/core/middleware.py tests/test_middleware.py tests/integration/test_edge_cases.py

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my code
• I have commented my code where necessary
• I have updated documentation as needed
• My changes generate no new warnings or errors
• I have added tests that prove my fix/feature works
• All existing tests pass
• I have checked for breaking changes

[smaramwbc]

Apologies for the silence on this one, that is on me: I wrote this up when I reviewed it and never actually posted it, so you have had a conflicted PR and no explanation for three days.

What happened: #509 arrived with the same fix and I merged that one, which is why this now conflicts. Same character class, same 1-128 bound, same decision to replace a bad id rather than reject the request. You two got there independently, which was the best evidence the fix was right.

I did not want to just close yours, because it covers input classes #509 does not, and there is one case neither of you pinned.

Neither PR tests a trailing newline. Yours puts it mid-string, which every variant of the regex rejects. Trailing is the position that matters: fullmatch handles it correctly today, but nothing would catch someone later changing it to .match(), and at that point X-Request-ID: abc\n gets echoed into the response header and the log context, which is the whole point of the fix.

So if you are still up for it, could you rebase onto main as a tests-only change:

• keep the invalid-charset loop, including the empty-string case, which is yours alone
• drop the three that duplicate tests already in that file (generated-when-absent, custom-valid-accepted, overly-long)
• add a trailing \n and \r\n case

That makes this the PR that closes the injection vector in the suite. Happy to push the rebase myself if you would rather not redo it, just say.

[SuparvaCode]

Rebased onto main and converted this to a tests-only change as suggested: kept the invalid charset loop + empty string case, dropped the duplicate tests, and added checks for trailing \n and \r\n. All middleware tests pass locally.

[smaramwbc]

Exactly it, thanks for turning it around.

Checked the trailing case does the job: swapping fullmatch for match now fails your test, which is the refactor that would have started echoing a newline into the response header. Neither of the original PRs caught that.

Merging.