Add S3 storage option to diet link with presigned URLs #111
Conversation
Enable dialog bodies to be stored in S3 instead of being removed or posted to an HTTP endpoint. The S3 option generates presigned URLs for secure access with configurable expiration (optional, defaults to 1 hour). New configuration options: - s3_bucket: S3 bucket name for storing dialog bodies - s3_path: Optional path prefix within the bucket - aws_access_key_id/aws_secret_access_key: AWS credentials - aws_region: AWS region (default: us-east-1) - presigned_url_expiration: URL expiration in seconds (optional) S3 takes precedence over post_media_to_url when both are configured. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: AWS secret key logged in plain text
The new S3 configuration options include aws_secret_access_key, which is now logged in plain text by the existing loop that iterates through all options. This exposes sensitive AWS credentials in log files, potentially allowing unauthorized access to the S3 bucket if logs are compromised or accessible to unauthorized personnel.
server/links/diet/__init__.py#L113-L115
vcon-server/server/links/diet/__init__.py
Lines 113 to 115 in e42b3b6
Comment @cursor review or bugbot run to trigger another review on this PR
There was a problem hiding this comment.
Pull request overview
This PR adds S3 bucket storage capability to the diet link, enabling dialog bodies to be uploaded to S3 with presigned URL generation for secure access. S3 storage takes precedence over HTTP endpoints when both are configured.
Key Changes:
- S3 upload functionality with presigned URL generation
- Configurable URL expiration (defaults to 1 hour)
- Comprehensive test coverage for S3 operations including success, failure, and precedence scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| server/links/diet/init.py | Adds S3 client creation, upload function with presigned URL generation, and integrates S3 storage into the main run function with precedence over HTTP endpoints |
| server/links/diet/test_diet.py | Adds comprehensive test suite for S3 functionality including upload tests, expiration handling, failure scenarios, and integration tests |
| server/links/diet/README.md | Documents the new S3 storage options, configuration parameters, usage examples, and clarifies precedence over HTTP endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s3.put_object( | ||
| Bucket=bucket, | ||
| Key=key, | ||
| Body=content.encode("utf-8") if isinstance(content, str) else content, | ||
| ContentType="text/plain", |
There was a problem hiding this comment.
The content type is hardcoded as "text/plain" for all S3 uploads. Dialog bodies may contain various content types (JSON, XML, etc.). Consider making the content type configurable or inferring it from the dialog body content to ensure proper handling by consumers of the presigned URLs.
| logger.error(f"S3 client error uploading dialog body: {e}") | ||
| return None | ||
| except Exception as e: | ||
| logger.error(f"Exception uploading dialog body to S3: {e}") |
There was a problem hiding this comment.
The broad Exception catch at line 103 may hide important errors. While catching ClientError is appropriate for S3-specific errors, consider being more specific about what other exceptions are expected here. At minimum, log the exception type to aid in debugging unexpected failures.
| logger.error(f"Exception uploading dialog body to S3: {e}") | |
| logger.exception(f"Unexpected exception uploading dialog body to S3: {type(e).__name__}: {e}") |
| dialog["body_type"] = "url" | ||
| else: | ||
| logger.error("Failed to upload to S3, removing body") | ||
| dialog["body"] = "" |
There was a problem hiding this comment.
When S3 upload fails and the body is cleared, the body_type field is not updated or removed. This could leave dialogs with body_type="url" but an empty body, which is an inconsistent state. Consider either removing the body_type field or setting it to an appropriate value (like an empty string) when the upload fails.
| dialog["body"] = "" | |
| dialog["body"] = "" | |
| dialog["body_type"] = "" |
| def _get_s3_client(options: Dict[str, Any]): | ||
| """Create and return an S3 client with the provided credentials.""" | ||
| return boto3.client( | ||
| "s3", | ||
| aws_access_key_id=options["aws_access_key_id"], | ||
| aws_secret_access_key=options["aws_secret_access_key"], | ||
| region_name=options.get("aws_region", "us-east-1"), | ||
| ) |
There was a problem hiding this comment.
Missing input validation for required S3 configuration parameters. When s3_bucket is configured, the code should validate that required credentials (aws_access_key_id and aws_secret_access_key) are also provided before attempting to create the S3 client. Currently, the code will fail with a less helpful error if these are missing or empty.
| expiration = options.get("presigned_url_expiration") | ||
| if expiration is None: | ||
| # Default to 1 hour (3600 seconds) if not specified | ||
| expiration = 3600 | ||
|
|
||
| presigned_url = s3.generate_presigned_url( | ||
| "get_object", | ||
| Params={"Bucket": bucket, "Key": key}, | ||
| ExpiresIn=expiration, | ||
| ) |
There was a problem hiding this comment.
The presigned_url_expiration value is not validated. According to AWS documentation, ExpiresIn must be a positive integer and has limits (typically up to 7 days for standard presigned URLs). Consider validating that the expiration value is within acceptable bounds to fail fast with a clear error message rather than letting boto3 raise an exception.
| "aws_access_key_id": "", # AWS access key ID | ||
| "aws_secret_access_key": "", # AWS secret access key | ||
| "aws_region": "us-east-1", # AWS region (default: us-east-1) | ||
| "presigned_url_expiration": None, # Presigned URL expiration in seconds (None = no expiration/default 1 hour) |
There was a problem hiding this comment.
The comment says "None = no expiration/default 1 hour" which is misleading. According to the implementation (lines 86-89), None doesn't mean "no expiration" - it means the default of 3600 seconds (1 hour) will be used. The comment should be clarified to say "None = default 1 hour" or "optional, defaults to 3600 seconds (1 hour)".
| "presigned_url_expiration": None, # Presigned URL expiration in seconds (None = no expiration/default 1 hour) | |
| "presigned_url_expiration": None, # Presigned URL expiration in seconds (optional, defaults to 3600 seconds [1 hour] if None) |
| from botocore.exceptions import ClientError | ||
|
|
||
| from server.links.diet import run, default_options, remove_system_prompts_recursive | ||
| from server.links.diet import run, default_options, remove_system_prompts_recursive, _upload_to_s3_and_get_presigned_url |
There was a problem hiding this comment.
Import of 'default_options' is not used.
| from server.links.diet import run, default_options, remove_system_prompts_recursive, _upload_to_s3_and_get_presigned_url | |
| from server.links.diet import run, remove_system_prompts_recursive, _upload_to_s3_and_get_presigned_url |
Co-authored-by: thomas.howe <[email protected]>
|
Cursor Agent can help with this pull request. Just |
Summary
New Configuration Options
s3_buckets3_pathaws_access_key_idaws_secret_access_keyaws_regionpresigned_url_expirationTest plan
🤖 Generated with Claude Code
Note
Add S3-based dialog body storage with presigned URLs, update diet link flow and options, and add comprehensive tests and docs.
boto3; S3 takes precedence over HTTPpost_media_to_url._get_s3_client,_upload_to_s3_and_get_presigned_urlfor upload and URL generation with configurable expiration and key structure{s3_path}/{vcon_uuid}/{dialog_id}_{uuid}.txt.run(...)to route dialog body handling: upload to S3 -> replace with URL and setbody_type="url"; fallback to HTTP post; else remove body on failure.default_optionswiths3_bucket,s3_path,aws_access_key_id,aws_secret_access_key,aws_region,presigned_url_expiration.run, and S3-over-HTTP precedence.Written by Cursor Bugbot for commit e42b3b6. Configure here.