fix: escape sessionId in VertexAiClient request paths - #1072
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
9611a18 to
2e99100
Compare
|
Hi @Tulgaaaaaaaa, thank you for your contribution! We appreciate you taking the time to submit this pull request. To proceed with the review, could you please address the following,
|
Session ids flow into VertexAiClient from user-supplied Session objects and were concatenated straight into the Vertex AI REST path. A session id containing "/" or ".." could retarget the request at a different resource, and one containing "?" could append arbitrary query parameters. Escape the session id with UrlEscapers.urlPathSegmentEscaper() in listEvents, getSession, deleteSession and appendEvent. The reasoning engine id is already constrained to digits by VertexAiSessionService.parseReasoningEngineId, and the userId filter in listSessions is already quoted as an AIP-160 literal and form-escaped. Adds VertexAiClientTest covering path traversal, query-string injection and the unescaped happy path for each affected method.
3313ecf to
ed68aee
Compare
|
Hi, I've addressed the requested changes:
Could you please review the updated PR when you have time? Thank you! |
|
@Tulgaaaaaaaa, thank you for addressing the feedback and making those updates. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you |
Problem
VertexAiClientconcatenates the session id straight into the Vertex AI REST path:Session ids reach the client from user-supplied
Sessionobjects, so a session idcontaining
/or..can retarget the request at a different resource, and onecontaining
?can append arbitrary query parameters to the request.Fix
Escape the session id as a single URL path segment with
UrlEscapers.urlPathSegmentEscaper()inlistEvents,getSession,deleteSessionand
appendEvent.Two values in this class were already safe and are left alone:
reasoningEngineIdis constrained to digits byVertexAiSessionService.parseReasoningEngineId.userIdinlistSessionsis already wrapped as a quoted AIP-160 literal andform-escaped on
main, so the original version of this PR (which replaced thatwith a bare
URLEncoder.encode) would have been a regression. Rebased away.UrlEscapersis used rather thanURLEncoder.encode, which is HTML form encoding —it turns a space into
+, which is a literal+in a path segment.Tests
New
VertexAiClientTest— 12 tests using MockitoArgumentCaptorto assert on theexact path handed to
HttpApiClient.request:../../secret→..%2F..%2Fsecret456?view=FULL→456%3Fview=FULLreasoningEngines/123/sessions/456:appendEventcustom verb and thelistEventsfilterparameter still survive escapinglistSessionsregression cover for the existing AIP-160 quotingNotes for reviewers
Rebased onto current
mainas a single commit — merge conflicts resolved, and thelistEvents(reasoningEngineId, sessionId, filter)signature that the earlier versionof this branch had dropped is preserved, which is what was breaking the Maven build.
Verified locally:
./mvnw test -Dtest=VertexAiClientTest(12/12), the existingVertexAiSessionServiceTest(30/30), and a full-projecttest-compile.