RSPEED-2652: validate type and format of rh-identity fields#1353
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds centralized string validation and two new helpers on RHIdentityData to enforce well-formed user and system identity fields (user_id, username, system.cn, account_number) and optional org_id checks; rejects non-strings, empty/whitespace, control characters, and enforces a 256-char max. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@major LGTM, but please rebase first before merge |
Add _validate_string_field() helper to RHIdentityData that checks extracted identity fields for type (must be str), non-emptiness, length bounds (max 256), and control characters. Wire validation into _validate_structure() for all consumed fields: user_id and username (User type), cn and account_number (System type), and org_id (conditional, both types). RSPEED-2652 Signed-off-by: Major Hayden <major@redhat.com>
Reduce cyclomatic complexity of _validate_structure from C (14) to B (8) by extracting _validate_user_fields and _validate_system_fields helper methods. No behavior change. RSPEED-2652 Signed-off-by: Major Hayden <major@redhat.com>
e274f9f to
3f58309
Compare
|
@tisnik Done. Thank you! |
Description
Add type and format validation to all string fields extracted from the x-rh-identity header in
RHIdentityData. Fields likeuser_id,username,cn,account_number, andorg_idwere only checked for presence but never validated for type, emptiness, length, or control characters. These values flow intorequest.state, logs, database queries, and telemetry as trusted strings, enabling potential log injection via control characters and data integrity issues from non-string types.Changes:
_validate_string_field()helper that checks: type (must bestr), non-emptiness (including whitespace-only), length bounds (max 256), and control characters (ASCII 0-31, DEL)_validate_structure()for User fields (user_id,username), System fields (cn,account_number), andorg_id(conditional: validated only when present and non-empty)_validate_user_fields()and_validate_system_fields()to reduce cyclomatic complexity of_validate_structurefrom C (14) to B (8)Note: PR #1352 (RSPEED-2651) also modifies
rh_identity.pybut touchesRHIdentityAuthDependency.__call__(), a completely different class. Zero source code overlap; only trivial test EOF append merge conflict possible.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run pytest tests/unit/authentication/test_rh_identity.py -v --tb=shortpasses 87/87 tests (50 existing + 37 new)uv run make verifypasses all linters (black, pylint 10/10, pyright 0 errors, ruff, pydocstyle, mypy 0 issues)uv run radon cc src/authentication/rh_identity.py -sshows all methods at A or B complexitySummary by CodeRabbit
Bug Fixes
Tests