Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the RecordsAPI to enable ingest, upsert, and delete operations for records within CDF streams. A critical finding is a path traversal vulnerability in the ingest, upsert, and delete methods. This occurs because the stream_id is used to construct the API endpoint URL without proper encoding, potentially allowing an attacker to manipulate the request path. Despite this, the implementation is well-structured, includes necessary client API layers, new resource classes (RecordIdentifier, RecordSource, RecordRequest, RecordResponse), and dedicated unit tests, adhering well to the repository's style guide. Other aspects, such as data models and client initialization, appear secure.
|
|
||
| def ingest(self, stream_id: str, items: Sequence[RecordRequest]) -> None: | ||
| """Ingest records into a stream. | ||
|
|
There was a problem hiding this comment.
The stream_id parameter is directly concatenated into the URL path using an f-string. This can lead to path traversal vulnerabilities if the stream_id contains characters like ../. An attacker could potentially manipulate the URL to hit unintended API endpoints. It is recommended to URL-encode the stream_id using urllib.parse.quote(stream_id, safe='') to ensure it is treated as a single path segment. Note: You will need to import quote from urllib.parse.
There was a problem hiding this comment.
I'd lean towards this not really being a issue due to the worst case scenario being a malformed / invalid API request that would be rejected anyways
| endpoint = f"/streams/{stream_id}/records" | ||
| self._request_no_response(items, "create", endpoint=endpoint) | ||
|
|
||
| def upsert(self, stream_id: str, items: Sequence[RecordRequest]) -> None: |
There was a problem hiding this comment.
I try to use create,retrieve, delete, update as far as possible even for upsert endpoints.
| def upsert(self, stream_id: str, items: Sequence[RecordRequest]) -> None: | |
| def update(self, stream_id: str, items: Sequence[RecordRequest]) -> None: |
| endpoint = f"/streams/{stream_id}/records" | ||
| self._request_no_response(items, "create", endpoint=endpoint) | ||
|
|
||
| def upsert(self, stream_id: str, items: Sequence[RecordRequest]) -> None: |
There was a problem hiding this comment.
Lets move stream_id innto the RecordRequest, see how it is solved for RawAPI
| def upsert(self, stream_id: str, items: Sequence[RecordRequest]) -> None: | ||
| """Upsert records into a stream (create or update). | ||
|
|
||
| Args: | ||
| stream_id: The external ID of the stream to upsert records into. | ||
| items: Sequence of RecordRequest items to upsert. | ||
| """ | ||
| endpoint = f"/streams/{stream_id}/records/upsert" | ||
| self._request_no_response(items, "upsert", endpoint=endpoint) | ||
|
|
||
| def delete(self, stream_id: str, items: Sequence[RecordIdentifier]) -> None: |
There was a problem hiding this comment.
Same here, move stream_id into RecordIdentifier.
| ) -> PagedResponse[RecordResponse]: | ||
| return PagedResponse[RecordResponse].model_validate_json(response.body) | ||
|
|
||
| def ingest(self, stream_id: str, items: Sequence[RecordRequest]) -> None: |
There was a problem hiding this comment.
Try to standardize
| def ingest(self, stream_id: str, items: Sequence[RecordRequest]) -> None: | |
| def create(self, stream_id: str, items: Sequence[RecordRequest]) -> None: |
doctrino
left a comment
doctrino
left a comment
There was a problem hiding this comment.
You also need to create smoke tests for records.
One questions. The difference between ingest and upsert is that one allows pre-existing records?
2 participants
Description
Add client API layer for CDF Records, supporting ingest, upsert, and delete operations scoped to streams. This is the first step toward records upload support in the toolkit. Download support to follow later.
Bump