MonkeyCanCode commented on PR #4772: URL: https://github.com/apache/polaris/pull/4772#issuecomment-4755365065
@venkateshwaracholan thanks for fixing those. One more concern I have is the PR description claimed "Redact sensitive headers such as Authorization and realm-related headers" and "Realm header redaction" but looking at the latest code, that is not the case: ``` ➜ polaris git:(main) ✗ ./polaris --profile dev --realm POLARIS catalogs list --debug ┌─[DEPRECATED]────────────────────────────────┐ │ This script is deprecated. Use pip instead: │ │ │ │ pip install apache-polaris │ └─────────────────────────────────────────────┘ Request: GET http://localhost:8181/api/management/v1/catalogs Headers: {'Accept': 'application/json', 'Polaris-Realm': 'POLARIS', 'User-Agent': 'OpenAPI-Generator/1.0.0/python', 'Authorization': '***REDACTED***'} Response: 200 Response Headers: {'Content-Type': 'application/json;charset=UTF-8', 'content-length': '15', 'X-Request-ID': '79a797dd-d537-49e3-9b6e-89edfe0e2fcb_0000000000000000005'} Response Body: {"catalogs": []} ``` Personally, I am not sure realm header need to be redacted. If we do think that is needed, I think current PR has mis-leading description. Otherwise, please update the PR description. Also, as the sensitive fields are well known, I am not sure if we really needed `_normalize_key`, `_is_sensitive_body_key`, and `_should_redact_header`. Those felt a bit over-engineering when the payload is well-known. Beside this, rest looks good to me. Again, thanks for the contribution. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
