Update gold_image_request.py #205
Conversation
📝 WalkthroughWalkthroughImageMetadata fields Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Proxy
participant API_Server
participant Auth_Middleware
participant Token_Service
Client->>Proxy: send request (may include X-Forwarded-Client-Cert / Authorization)
Proxy->>API_Server: forward request + headers
API_Server->>Auth_Middleware: perform auth checks
alt X-Forwarded-Client-Cert present
Auth_Middleware->>Auth_Middleware: extract principal (URI or spiffe://)
Auth_Middleware->>Auth_Middleware: validate_mtls_auth(principal)
alt valid
Auth_Middleware->>API_Server: allow request
else invalid
Auth_Middleware->>API_Server: return 403
end
else no mTLS header
Auth_Middleware->>Token_Service: validate token / fallback HMAC
Token_Service->>Auth_Middleware: validation result
Auth_Middleware->>API_Server: allow or deny based on token/HMAC
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wavefront/server/modules/gold_module/gold_module/models/gold_image_request.py (2)
86-90: UseField(default_factory=ImageMetadata)instead ofImageMetadata()to prevent shared mutable state.The current code reuses a single
ImageMetadatainstance across allImageAnalysisRequestinstances. SinceImageMetadatacontains mutabledictfields (metadata_1throughmetadata_5), modifications to these fields in one request would affect all other instances.Proposed diff
-from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, Field @@ class ImageAnalysisRequest(BaseModel): image: str # data URL (base64 with MIME) or direct URL - metadata: ImageMetadata = ( - ImageMetadata() - ) # Ensure metadata is always an ImageMetadata instance + metadata: ImageMetadata = Field(default_factory=ImageMetadata)
56-84:to_string_dict()incorrectly stringifiesItemmodels (and doesn't stringify dict keys) — breaks expected output format.When processing
items: List[Item], each Item object falls through tostr(val)(line 80), producing"Item(item_id='...' ...)"instead of a dict with stringified leaf values. Additionally, dict keys remain non-string despite the docstring claiming "all nested values are strings."Also replace
__pydantic_extra__(internal API) withmodel_extrafor Pydantic v2 compatibility.Proposed fix
def get_extra_fields(self) -> dict: """Return a dict of extra fields not defined in the model.""" - return ( - dict(self.__pydantic_extra__) - if hasattr(self, '__pydantic_extra__') and self.__pydantic_extra__ - else {} - ) + return dict(self.model_extra or {}) def get_defined_fields(self) -> dict: """Return a dict of only the defined fields (excluding extras).""" - return self.model_dump(exclude=self.get_extra_fields().keys()) + return self.model_dump(exclude=set(self.get_extra_fields())) def to_string_dict(self) -> dict: """Return a dict with all fields (excluding extras) as strings. None remains None. All nested values are strings.""" def to_str_recursive(val): if val is None: return None + if isinstance(val, BaseModel): + return to_str_recursive(val.model_dump()) if isinstance(val, list): return [to_str_recursive(v) for v in val] if isinstance(val, dict): - return {k: to_str_recursive(v) for k, v in val.items()} + return {str(k): to_str_recursive(v) for k, v in val.items()} return str(val)
🧹 Nitpick comments (1)
wavefront/server/modules/gold_module/gold_module/models/gold_image_request.py (1)
44-48: Tightenmetadata_1..metadata_5typing (avoid baredict).
Baredictweakens validation/schema and makes downstream expectations unclear; preferdict[str, Any] | None(orMapping[str, Any] | None).Proposed diff
-from typing import List +from typing import Any, List class ImageMetadata(BaseModel): @@ - metadata_1: dict = None - metadata_2: dict = None - metadata_3: dict = None - metadata_4: dict = None - metadata_5: dict = None + metadata_1: dict[str, Any] | None = None + metadata_2: dict[str, Any] | None = None + metadata_3: dict[str, Any] | None = None + metadata_4: dict[str, Any] | None = None + metadata_5: dict[str, Any] | None = None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wavefront/server/modules/gold_module/gold_module/models/gold_image_request.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-artifact
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py:
- Around line 421-435: The code currently logs the full X-Forwarded-Client-Cert
header (mtls_header) in the mTLS branch; remove or replace that sensitive value
with a non-sensitive presence message and rely on validate_mtls_auth to log the
extracted principal. Update the logger.info call in the block that checks
request.headers.get('X-Forwarded-Client-Cert') to something like "mTLS client
certificate presented" (or otherwise redact mtls_header) and do not log
mtls_header itself; keep the rest of the flow (await validate_mtls_auth,
call_next, JSONResponse using response_formatter.buildErrorResponse) unchanged
so authentication behavior is preserved.
🧹 Nitpick comments (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py (1)
272-274: Consider refactoring for improved readability.The double-negative logic (
and not ... and not ...) is correct but could be clearer. Consider using a positive check or restructuring the condition.♻️ Proposed refactor for clarity
- if not principal.startswith( - 'spiffe://cluster.local/ns/client-applications' - ) and not principal.startswith('spiffe://cluster.local/ns/gpu-processing'): + allowed_prefixes = ( + 'spiffe://cluster.local/ns/client-applications', + 'spiffe://cluster.local/ns/gpu-processing', + ) + if not principal.startswith(allowed_prefixes): logger.error(f'Invalid mTLS authentication. Principal: {principal}') return False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-artifact
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py:
- Around line 425-439: The code logs the full X-Forwarded-Client-Cert header
(mtls_header) in the middleware; remove or redact that sensitive logging. Locate
the block handling mtls_header in require_auth.py (look for the mtls_header =
request.headers.get('X-Forwarded-Client-Cert') line and the subsequent
logger.info call) and either delete the logger.info(f'mTLS authentication by
{mtls_header}') statement or replace it with a non-sensitive message like
logger.info('mTLS authentication attempt') while keeping the existing call to
validate_mtls_auth and the existing success/failure logging inside
validate_mtls_auth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py
🧰 Additional context used
🧬 Code graph analysis (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py (1)
wavefront/server/modules/api_services_module/api_services_module/models/service.py (1)
error(131-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-artifact
🔇 Additional comments (3)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py (3)
267-274: LGTM! Good handling of multiple XFCC formats.The dual extraction strategy correctly handles both standard URI-encoded SPIFFE IDs and direct SPIFFE ID formats. The fallback logic is well-structured.
275-280: LGTM! Correct namespace authorization logic.The addition of the
gpu-processingnamespace correctly implements an OR condition - either namespace is allowed for mTLS authentication.
297-297: LGTM! More accurate logging.The updated log message correctly references the
principalvariable, improving log clarity and consistency.
| mtls_header = request.headers.get('X-Forwarded-Client-Cert') | ||
| if mtls_header and not token: | ||
| logger.info(f'mTLS authentication by {mtls_header}') | ||
| if await validate_mtls_auth(request): | ||
| return await call_next(request) | ||
| else: | ||
| logger.error( | ||
| f'Invalid mTLS authentication for {request.url.path}' | ||
| ) | ||
| return JSONResponse( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| content=response_formatter.buildErrorResponse( | ||
| error='Invalid mTLS authentication' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Security concern: Avoid logging sensitive certificate data.
Line 427 logs the entire X-Forwarded-Client-Cert header, which may contain sensitive certificate information such as serial numbers, subject distinguished names, and other metadata. This data should not be logged in full, as logs may be accessed by multiple systems and personnel.
The validate_mtls_auth function already logs authentication success/failure with the principal on lines 290-292 and 296-298, making this additional logging redundant.
🔒 Proposed fix to remove or redact sensitive header logging
Option 1 (Recommended): Remove the redundant log statement
Since validate_mtls_auth already provides detailed logging, simply remove this line:
mtls_header = request.headers.get('X-Forwarded-Client-Cert')
if mtls_header and not token:
- logger.info(f'mTLS authentication by {mtls_header}')
if await validate_mtls_auth(request):Option 2: Log only that mTLS is being attempted
If you need logging at the dispatch level, log without exposing the header value:
mtls_header = request.headers.get('X-Forwarded-Client-Cert')
if mtls_header and not token:
- logger.info(f'mTLS authentication by {mtls_header}')
+ logger.info('Attempting mTLS authentication')
if await validate_mtls_auth(request):🤖 Prompt for AI Agents
In
@wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py
around lines 425 - 439, The code logs the full X-Forwarded-Client-Cert header
(mtls_header) in the middleware; remove or redact that sensitive logging. Locate
the block handling mtls_header in require_auth.py (look for the mtls_header =
request.headers.get('X-Forwarded-Client-Cert') line and the subsequent
logger.info call) and either delete the logger.info(f'mTLS authentication by
{mtls_header}') statement or replace it with a non-sensitive message like
logger.info('mTLS authentication attempt') while keeping the existing call to
validate_mtls_auth and the existing success/failure logging inside
validate_mtls_auth.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py`:
- Around line 295-298: The code logs the raw mTLS header variable xfcc in
logger.warning; remove or redact that sensitive value: locate the logger.warning
call near where request_id is computed (get_current_request_id()) and change the
message to omit xfcc (e.g., log "mTLS header present but no valid principal
found" with the request_id) or replace xfcc with a non-sensitive
token/hashing/redacted literal before logging; keep request_id in the log for
traceability and do not persist or print the raw certificate header anywhere.
♻️ Duplicate comments (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py (1)
425-439: Duplicate: Sensitive certificate logging already flagged.The logging of full
mtls_headeron line 427 was addressed in a previous review comment. The new explicit 403 response for invalid mTLS authentication (lines 430-439) is a good addition that properly rejects failed mTLS attempts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py
🔇 Additional comments (3)
wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py (3)
267-280: LGTM on SPIFFE principal extraction enhancement.The logic correctly handles both URI-embedded SPIFFE IDs and direct SPIFFE headers. The disjunction allowing
client-applicationsorgpu-processingnamespaces is clear.
338-340: LGTM on HMAC gating with Authorization header check.The condition
not authorizationensures HMAC validation only applies when noAuthorizationheader is present, preventing auth method ambiguity.
371-372: LGTM on dispatch flow restructuring.The explicit
elseblock cleanly separates auth paths (HMAC, service auth, normal token flow), improving readability and maintainability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| request_id = getattr(request.state, 'request_id', get_current_request_id()) | ||
| logger.warning( | ||
| f'mTLS header present but no valid URI found: {xfcc} [Request ID: {request_id}]' | ||
| f'mTLS header present but no valid principal found: {xfcc} [Request ID: {request_id}]' | ||
| ) |
There was a problem hiding this comment.
Security concern: Avoid logging raw certificate header.
Line 297 logs the full xfcc header content, which may contain sensitive certificate metadata (serial numbers, subject DNs, issuer info). Consider logging only that no valid principal was found, without exposing the raw header value.
🔒 Proposed fix to redact sensitive header data
request_id = getattr(request.state, 'request_id', get_current_request_id())
logger.warning(
- f'mTLS header present but no valid principal found: {xfcc} [Request ID: {request_id}]'
+ f'mTLS header present but no valid SPIFFE principal found [Request ID: {request_id}]'
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request_id = getattr(request.state, 'request_id', get_current_request_id()) | |
| logger.warning( | |
| f'mTLS header present but no valid URI found: {xfcc} [Request ID: {request_id}]' | |
| f'mTLS header present but no valid principal found: {xfcc} [Request ID: {request_id}]' | |
| ) | |
| request_id = getattr(request.state, 'request_id', get_current_request_id()) | |
| logger.warning( | |
| f'mTLS header present but no valid SPIFFE principal found [Request ID: {request_id}]' | |
| ) |
🤖 Prompt for AI Agents
In
`@wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py`
around lines 295 - 298, The code logs the raw mTLS header variable xfcc in
logger.warning; remove or redact that sensitive value: locate the logger.warning
call near where request_id is computed (get_current_request_id()) and change the
message to omit xfcc (e.g., log "mTLS header present but no valid principal
found" with the request_id) or replace xfcc with a non-sensitive
token/hashing/redacted literal before logging; keep request_id in the log for
traceability and do not persist or print the raw certificate header anywhere.
Summary by CodeRabbit
Enhancements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.