fix(api): stop echoing the api_key value in the external knowledge 403 error - #39897
Closed
pacocartones wants to merge 1 commit into
Closed
fix(api): stop echoing the api_key value in the external knowledge 403 error#39897pacocartones wants to merge 1 commit into
pacocartones wants to merge 1 commit into
Conversation
…3 error The validation probe in check_endpoint_and_api_key interpolated the raw api_key into the ValueError message on a 403 response. The console API's handle_value_error then logs the full message and returns it verbatim in the 400 response body, writing the credential to application logs and echoing it back to the caller. Keep the auth-failure signal without echoing the value, consistent with the other api_key validations in the same file. Adds a regression test asserting the key never appears in the raised message. Fixes langgenius#39888
pacocartones
requested review from
JohnJyong,
QuantumGhost and
laipz8200
as code owners
August 2, 2026 11:38
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Fixes #<issue number>.Summary
The external knowledge API validation probe at
check_endpoint_and_api_keyinterpolates the rawapi_keyvalue into theValueErrormessage on a 403 response (api/services/external_knowledge_service.py:117). Because the exception is not wrapped by the caller, it reaches the console API'shandle_value_error(api/libs/external_api.py:92-97), which (a) logs the full message viacurrent_app.logger.exception, and (b) returns it verbatim to the caller in a 400 body ({"code": "invalid_param", "message": str(e), "status": 400}). The credential therefore lands in application logs and in the API response itself. (Sentry is unaffected: this project explicitly ignoresValueErrorinextensions/ext_sentry.py.)Stop interpolating the key, keeping the auth-failure signal intact. This matches the other validations in the same file (
api_key is requiredat lines 57, 85, and 270), which never echo the value.Fixes #39888
Screenshots
Backend error-message change with no UI surface; the behaviour change is covered by the regression test.
400 {"code": "invalid_param", "message": "Forbidden: Authorization failed with api_key: sk-live-key-value"}— key returned to the caller and written to application logs400 {"code": "invalid_param", "message": "Forbidden: Authorization failed with the provided api_key"}— key is never echoedChecklist
make lint && make type-check(backend) andcd web && pnpm exec vp staged(frontend) to appease the lint godsOn the absence of a private advisory: I did not use the private advisory channel on purpose: there is nothing to disclose beyond log/observability hygiene. The tenant who submits the key already knows its value; the issue is limited to the credential being written to application logs and echoed in the 400 response to that same tenant. Sentry explicitly ignores
ValueErrorin this project, so it never reaches the error-tracking pipeline. I have classified it accordingly — happy to move this to a private advisory if you read it differently.Test evidence
New regression test
test_check_endpoint_403_forbidden_does_not_leak_api_keyinapi/tests/unit_tests/services/test_external_dataset_service.py(same class and mocking pattern as the existingtest_check_endpoint_403_forbidden).Without the fix (current
main):With the fix applied:
(The full test file passes, 110/110, including the pre-existing
test_check_endpoint_403_forbidden, whoseForbidden.*Authorization failedmatch still holds.)Assisted-by: AI tooling (implementation and verification reviewed line by line; I understand and stand by the change).