fix: add request timeouts, restore TLS verify, and stop logging secrets#639
Open
tiantt wants to merge 1 commit into
Open
fix: add request timeouts, restore TLS verify, and stop logging secrets#639tiantt wants to merge 1 commit into
tiantt wants to merge 1 commit into
Conversation
Tighten defensive defaults on outbound calls and remove secret leakage surfaced by a reliability/security/observability review. Reliability: - volcengine_sign: add an overridable `timeout` (default 10s connect / 60s read) to both signed-request helpers, which previously issued requests.request(...) with no timeout. A hung Volcengine endpoint could block the caller forever; this bounds the ~82 OpenAPI call sites that route through these two helpers. Slow control-plane calls can override. - web_scraper / vesearch: add a 30s timeout to their direct requests.post. Security / log hygiene: - web_scraper: drop `verify=False` (restore TLS certificate verification) on a request that carries X-VE-API-Key. - llm_shield: stop logging the full API key at debug level; log only whether it is configured. - ve_credential_service: fix a docstring example that printed a bearer token.
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
2 participants
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.
What
Defensive-default hardening surfaced by a reliability/security/observability review. Five small, self-contained changes across two themes.
Reliability — bound outbound calls
veadk/utils/volcengine_sign.py: both signed-request helpers (volcengine_signed_requestandrequest) previously calledrequests.request(...)with no timeout, so a hung Volcengine endpoint could block the caller indefinitely. Added an overridabletimeoutparameter defaulting to(10, 60)(10s connect / 60s read). This single change bounds the ~82 OpenAPI call sites that route through these two helpers; slow control-plane calls (deploy/upload) can pass a larger value.veadk/tools/builtin_tools/web_scraper.pyandvesearch.py: added a 30s timeout to their directrequests.postcalls (these are on the agent's tool path).Security / log hygiene
web_scraper.py: droppedverify=False(restored TLS certificate verification) on a request that carriesX-VE-API-Key.llm_shield.py: stopped logging the full API key atdebuglevel — now logs only whether it is configured. (Default log level is DEBUG, so the key was being emitted in normal runs.)ve_credential_service.py: fixed a docstring example that printed a bearer token.Reviewer note⚠️
The
verify=Falseremoval inweb_scraperis a behavior change: ifTOOL_WEB_SCRAPER_ENDPOINTserves a self-signed / private-CA certificate, requests will now fail TLS verification. If that endpoint is not publicly trusted, the correct fix isverify=<ca_bundle_path>rather than re-disabling verification. Please confirm the endpoint uses a publicly-trusted cert before merging.Scope
These are the low-risk, no-design-needed items from the review. Not included here (need design/product decisions): unverified-JWT auth path, A2A hub authz,
trace_contentdefault, and the pre-existing type issues inweb_scraper(unboundresponseinexcept, return-type mismatch). CI will validate; changes byte-compile clean.