Start on documents feature#43
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThe changes introduce a new document upload feature integrated across the backend, proxy, and deployment scripts. This includes new endpoints for document uploads, proxy support for forwarding document upload requests, and updated deployment instructions and scripts to support the new service. Supporting dependencies and configuration were also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend (Axum)
participant Tinfoil Proxy
participant Tinfoil Doc Service
Client->>Backend (Axum): POST /v1/documents/upload (multipart file)
Backend->>Tinfoil Proxy: Forward multipart request (with JWT validation)
Tinfoil Proxy->>Tinfoil Doc Service: POST /v1alpha/convert/file (multipart)
Tinfoil Doc Service-->>Tinfoil Proxy: JSON response (extracted text, etc.)
Tinfoil Proxy-->>Backend (Axum): JSON response
Backend (Axum)-->>Client: Encrypted JSON response
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added a new document upload feature across multiple components, implementing secure file processing through Tinfoil proxy service with AWS Nitro Enclaves integration.
- New
/v1/documents/uploadendpoint insrc/web/documents.rshandles file uploads with 10MB limit and billing checks - Modified
tinfoil-proxy/main.goto support document processing with new DocumentUploadResponse struct - Added vsock proxy service configuration for
doc-upload.model.tinfoil.shindocs/nitro-deploy.mdandentrypoint.sh - Empty
tinfoil-proxy/tinfoil-proxyfile requires implementation or removal src/proxy_config.rsmakes assumptions about proxy ordering that could be problematic
9 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
AnthonyRonning
force-pushed
the
document-start
branch
from
db5681f to
49f372f
Compare
There was a problem hiding this comment.
PR Summary
Further improvements to the document upload feature implementation focusing on security and infrastructure configuration.
- Implemented proper input validation and file size limits (10MB) in
src/web/documents.rs - Added guest user restrictions and security checks for file uploads
- Configured response encryption for document handling in
src/web/documents.rs - Added proper .gitignore rules in
tinfoil-proxy/.gitignoreto handle binary tracking - Added 'multipart' feature to axum in
Cargo.tomlfor handling file uploads
9 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/proxy_config.rs (1)
106-110: 🛠️ Refactor suggestionBrittle identification of Tinfoil proxy
Assuming the first entry inadditional_proxiesis always Tinfoil is fragile—consider tagging each proxy with a type or checking URL patterns to reliably identify the Tinfoil endpoint.src/web/mod.rs (1)
2-2:⚠️ Potential issueExpose
documentsmodule publicly
Thedocumentsmodule needs to be declaredpubforpub use documents::routerto compile.tinfoil-proxy/.gitignore (1)
5-5: Explicitly unignoredist/tinfoil-proxy
Add an explicit!dist/tinfoil-proxyrule to clearly allow the binary indist/.src/web/documents.rs (1)
90-97:_file_sizeis unused – will trigger a warn(-deny) buildThe variable is created but never read. Either incorporate it into the payload or remove it:
- let _file_size = file_data.len() as i64; + let file_size = file_data.len() as i64; // use later or remove entirelytinfoil-proxy/main.go (1)
373-375: Hard-coded URL ignores config – same issue previously flaggedThe request is still built with a literal hostname. Use
docUploadConfig.Enclavefor consistency and single-point config.-req, err := http.NewRequest("POST", "https://doc-upload.model.tinfoil.sh/v1alpha/convert/file", &requestBody) +req, err := http.NewRequest( + "https://"+docUploadConfig.Enclave+"/v1alpha/convert/file", + &requestBody)
🧹 Nitpick comments (6)
entrypoint.sh (2)
277-278: Avoid accumulating duplicate /etc/hosts entries across restartsEach time the script runs it blindly appends
127.0.0.21 doc-upload.model.tinfoil.sh.
If the enclave/container restarts frequently the same line will be appended many times, eventually bloating/etc/hosts.-echo "127.0.0.21 doc-upload.model.tinfoil.sh" >> /etc/hosts +grep -q "doc-upload.model.tinfoil.sh" /etc/hosts || \ + echo "127.0.0.21 doc-upload.model.tinfoil.sh" >> /etc/hosts
516-521: Connectivity test block duplicates previous pattern – consider factoring into a helperWe now have ~30 identical “timeout … </dev/tcp/X/Y” blocks. This is becoming unmaintainable.
A tiny bash helper would remove duplication:test_conn () { local host=$1; local port=$2; local name=$3 log "Testing connection to ${name}:" if timeout 5 bash -c "</dev/tcp/${host}/${port}"; then log "${name} connection successful" else log "${name} connection failed" fi } # … test_conn 127.0.0.21 443 "Tinfoil Document Upload"docs/nitro-deploy.md (2)
955-973: Missing fenced-code language spec breaks markdown-lint
markdownlintis already flagging MD040.
Simply tag the block asshto silence CI and get syntax highlighting:-``` +```sh🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
961-961: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
845-848: Allow-list entry added — remind to keep allocator in syncNice catch adding
doc-upload.model.tinfoil.sh; remember to restartnitro-enclaves-vsock-proxy.serviceafter editing/etc/nitro_enclaves/vsock-proxy.yamlor the allow-list change won’t apply.
(update instructions if you expect newcomers to follow the doc verbatim)src/web/documents.rs (1)
172-174:cargo fmtneeded – CI is red
cargo fmtreports trailing whitespace/blank-line issues around here.
Runcargo fmt --allto let CI pass.🧰 Tools
🪛 GitHub Actions: Rust CI
[error] 172-172: cargo fmt check failed due to trailing whitespace or extra newline. Run 'cargo fmt' to fix code style issues.
tinfoil-proxy/main.go (1)
386-396: Reuse initializeddocUploadClientinstead of barehttp.ClientYou already create
server.docUploadClient; leveraging its underlying client (or SDK helper when available) would give you retries, tracing and consistent TLS settings.
Consider exposingdocUploadClient.HTTPClient()or adding a thin wrapper instead of building a newhttp.Clienthere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(1 hunks)docs/nitro-deploy.md(4 hunks)entrypoint.sh(3 hunks)src/main.rs(2 hunks)src/proxy_config.rs(1 hunks)src/web/documents.rs(1 hunks)src/web/mod.rs(2 hunks)tinfoil-proxy/.gitignore(1 hunks)tinfoil-proxy/main.go(6 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/nitro-deploy.md
961-961: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 GitHub Actions: Rust CI
src/main.rs
[error] 16-16: cargo fmt check failed due to incorrect import formatting. Run 'cargo fmt' to fix code style issues.
src/web/documents.rs
[error] 172-172: cargo fmt check failed due to trailing whitespace or extra newline. Run 'cargo fmt' to fix code style issues.
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (4)
Cargo.toml (1)
7-7: Enable multipart feature for Axum
The addition of"multipart"alongside"macros"correctly supports handling multipart form data in the new document upload endpoint.src/main.rs (2)
19-19: Runcargo fmtto fix import formatting
The long import line violates Rust style rules and is causing CI failures; applycargo fmtfor proper formatting.
2418-2421: Merge document upload routes with JWT middleware
Thedocument_routesare correctly merged and protected by the JWT validation layer, consistent with theopenai_routesintegration.entrypoint.sh (1)
365-367: Port collision / hard-coding sanity-check
8024is currently unused, but new forwarders are added very frequently.
Please confirm in Terraform / systemd service definitions that no other forwarder already binds to 8024 to avoid silent failures.
No change requested – just ensure the collision check is on your deployment checklist.
AnthonyRonning
force-pushed
the
document-start
branch
from
49f372f to
3a5ec08
Compare
There was a problem hiding this comment.
PR Summary
Updated PCR measurements and system integrity configuration for the new document upload feature integration with AWS Nitro Enclaves.
- Modified PCR values in
pcrProd.jsonandpcrDev.jsonwith new hash measurements for document upload functionality validation - Updated PCR0 value to
b2203a0393607d0ae50e80d9d148b76c807fd3fc69e76ba2457372cf05c18a6c0e00ffa38518089ad7ba26dd582dc4f9for enclave measurement - Integrated document routes with JWT validation middleware in
src/main.rsunder protected routes - Added DNS entry for
doc-upload.model.tinfoil.shmapped to 127.0.0.21 inentrypoint.sh
11 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/web/documents.rs (1)
98-99:_file_sizeis still unused – remove or use it
The variable triggersunused_variablewarnings and was called out in the previous review. Either drop it or include it in the JSON you return.-let _file_size = file_data.len() as i64; +let file_size = file_data.len() as i64; // keep if you plan to propagate
🧹 Nitpick comments (2)
src/web/documents.rs (1)
100-117: Manually constructing multipart bodies is fragile – use a library
Hard-coding the boundary and hand-assembling CRLFs risks subtle bugs (boundary collisions, wrong encoding, etc.). Considerreqwest::multipartormime_multipartto build the form securely:let part = reqwest::multipart::Part::bytes(file_data) .file_name(body.filename.clone()) .mime_str("application/octet-stream")?; let form = reqwest::multipart::Form::new().part("file", part);This also removes the need to pick a boundary yourself.
docs/nitro-deploy.md (1)
955-973: Add language identifier to fenced code block
Markdown-lint (MD040) complains because the code block for the newvsock-tinfoil-doc-upload.servicelacks a language tag. Prefix it withinifor correct highlighting and style compliance.-``` +```ini [Unit] Description=Vsock Tinfoil Document Upload Service …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockpcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.jsontinfoil-proxy/dist/tinfoil-proxyis excluded by!**/dist/**
📒 Files selected for processing (9)
Cargo.toml(1 hunks)docs/nitro-deploy.md(4 hunks)entrypoint.sh(3 hunks)src/main.rs(2 hunks)src/proxy_config.rs(1 hunks)src/web/documents.rs(1 hunks)src/web/mod.rs(2 hunks)tinfoil-proxy/.gitignore(1 hunks)tinfoil-proxy/main.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- src/web/mod.rs
- tinfoil-proxy/.gitignore
- src/proxy_config.rs
- entrypoint.sh
- src/main.rs
- tinfoil-proxy/main.go
🧰 Additional context used
🪛 GitHub Actions: Rust CI
src/web/documents.rs
[error] 1-10: cargo fmt formatting check failed. Code style does not match rustfmt standards; imports need to be reformatted.
[error] 174-175: cargo fmt formatting check failed. Extra trailing whitespace or blank line detected.
🪛 markdownlint-cli2 (0.17.2)
docs/nitro-deploy.md
961-961: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (1)
docs/nitro-deploy.md (1)
842-848: Keep allow-list and service port numbers in sync
You addeddoc-upload.model.tinfoil.sh(port 443) to the allow-list and start a proxy on port 8024. Double-check any firewall rules / health checks so both entries reference the same host/port combo.
AnthonyRonning
force-pushed
the
document-start
branch
from
3a5ec08 to
7351a16
Compare
There was a problem hiding this comment.
PR Summary
Missing PCR history update in the new Nitro Enclave configuration changes for document upload feature.
- No update to
pcrProdHistory.jsondespite PCR value changes inpcrProd.json, violating audit trail requirements - PCR2 value update in
pcrDev.jsonandpcrProd.jsonlacks corresponding documentation on measurement changes - Need to maintain auditable history of PCR changes for secure enclave verification
11 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/nitro-deploy.md (1)
961-973: Specify language for fenced code block
The systemd unit snippet should include a language identifier (e.g.,ini) to satisfy markdown linting and improve readability.Apply this diff:
- ``` + ```ini [Unit] Description=Vsock Tinfoil Document Upload Service After=network.target [Service] User=root ExecStart=/usr/bin/vsock-proxy 8024 doc-upload.model.tinfoil.sh 443 Restart=always [Install] WantedBy=multi-user.target</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3a5ec08dcc9adfba950e0fb9495b931d99a56fc5 and 7351a164be8204603f4cd94c2e6d37885939391f. </details> <details> <summary>⛔ Files ignored due to path filters (4)</summary> * `Cargo.lock` is excluded by `!**/*.lock` * `pcrDev.json` is excluded by `!pcrDev.json` * `pcrProd.json` is excluded by `!pcrProd.json` * `tinfoil-proxy/dist/tinfoil-proxy` is excluded by `!**/dist/**` </details> <details> <summary>📒 Files selected for processing (9)</summary> * `Cargo.toml` (1 hunks) * `docs/nitro-deploy.md` (4 hunks) * `entrypoint.sh` (3 hunks) * `src/main.rs` (2 hunks) * `src/proxy_config.rs` (1 hunks) * `src/web/documents.rs` (1 hunks) * `src/web/mod.rs` (2 hunks) * `tinfoil-proxy/.gitignore` (1 hunks) * `tinfoil-proxy/main.go` (6 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (8)</summary> * Cargo.toml * tinfoil-proxy/.gitignore * src/proxy_config.rs * src/main.rs * src/web/mod.rs * entrypoint.sh * src/web/documents.rs * tinfoil-proxy/main.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>docs/nitro-deploy.md</summary> 961-961: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 100000ms (1)</summary> * GitHub Check: Development Reproducible Build </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>docs/nitro-deploy.md (4)</summary> `845-845`: **Tinfoil proxy allowlist updated correctly** The new entry `- {address: doc-upload.model.tinfoil.sh, port: 443}` aligns with the updated proxy configuration in code. --- `955-958`: **Introduce Tinfoil Document Upload service file** The new heading and `sudo vim` invocation for `vsock-tinfoil-doc-upload.service` are consistent with existing Tinfoil proxy sections. --- `994-997`: **Enable and start the new Document Upload service** The commands to enable, start, and check status of `vsock-tinfoil-doc-upload.service` correctly mirror the pattern used for other proxies. --- `1006-1006`: **Include restart instruction for Document Upload service** Adding `sudo systemctl restart vsock-tinfoil-doc-upload.service` ensures it’s included in the global restart instructions alongside other Tinfoil proxies. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
PR Summary
Added error handling and security improvements to the document upload feature implementation.
- Fixed potential information exposure in
tinfoil-proxy/main.goby sanitizing error messages before client response - Document upload endpoint in
src/web/documents.rsnow includes proper encryption middleware integration - Updated network isolation setup in
entrypoint.shwith connection testing for doc-upload service - Added file type validation checks to prevent malicious uploads
- Implemented proper content-type and request size validation in multipart form handling
11 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
Updated continuum-proxy and PCR history configurations for the document upload feature integration.
- Updated continuum-proxy version from v1.18.0 to v1.19.0 in
justfilewith new SHA256 verification hash - Created empty
continuum-proxyfile instead of intended directory structure which needs correction
6 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/web/documents.rs (1)
72-91: Move size validation before base64 decoding to improve performance.This is the same performance issue identified in previous reviews. Decoding large base64 content before size validation wastes CPU and memory.
Apply the previously suggested optimization:
+// Quick-reject on encoded length (≈33% overhead) before allocating +if body.content_base64.len() > 14 * 1024 * 1024 { + error!("Base64 payload exceeds 10 MB limit"); + return Err(ApiError::BadRequest); +} + // Decode the base64 file content let file_data = general_purpose::STANDARD .decode(&body.content_base64) .map_err(|e| { error!("Failed to decode base64 file content: {:?}", e); ApiError::BadRequest })?; +if file_data.is_empty() { + error!("No file data received"); + return Err(ApiError::BadRequest); +} + +// Enforce decoded size (defense-in-depth) if file_data.len() > 10 * 1024 * 1024 { error!("File size exceeds 10MB limit"); return Err(ApiError::BadRequest); }
🧹 Nitpick comments (1)
src/web/documents.rs (1)
93-109: Consider using a proper multipart library for better maintainability.Manual multipart form construction works but is error-prone. Consider using the
reqwestcrate with multipart support ormultipartcrate for more robust handling.If keeping the manual approach, generate the boundary dynamically:
-let boundary = "----WebKitFormBoundary7MA4YWxkTrZu0gW"; +let boundary = format!("----WebKitFormBoundary{}", uuid::Uuid::new_v4().simple());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (5)
justfile(1 hunks)src/proxy_config.rs(3 hunks)src/web/documents.rs(1 hunks)src/web/mod.rs(2 hunks)tinfoil-proxy/main.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- justfile
🚧 Files skipped from review as they are similar to previous changes (3)
- src/web/mod.rs
- src/proxy_config.rs
- tinfoil-proxy/main.go
🧰 Additional context used
🪛 GitHub Actions: Rust CI
src/web/documents.rs
[error] 139-139: cargo fmt formatting check failed. There is an unexpected trailing comma in the code at line 139.
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (6)
src/web/documents.rs (6)
1-12: LGTM - imports are well-organized and comprehensive.The import statements cover all necessary dependencies for the document upload functionality.
14-25: LGTM - struct definitions are appropriate for the API contract.The request/response structs correctly define the expected fields for document upload functionality.
27-35: LGTM - router setup follows best practices.The router correctly configures the document upload endpoint with appropriate middleware and state management.
46-70: LGTM - excellent security and billing validation.The guest user prevention and billing checks with graceful degradation follow security best practices.
117-152: Fix cargo fmt formatting issue in timeout function call.The HTTP client setup and error handling are well-implemented, but there's a formatting issue flagged by CI.
Apply this fix for the trailing comma issue:
let res = timeout( std::time::Duration::from_secs(300), // 5 minutes - client.request(req) + client.request(req), )
154-180: LGTM - response handling follows best practices.Status validation, error handling, and encrypted response return are all properly implemented.
There was a problem hiding this comment.
PR Summary
Enhanced document upload handler in Tinfoil proxy with improved timeout management and updated PCR measurements for AWS Nitro Enclaves.
- Implemented context-based timeout (5 minutes) in
tinfoil-proxy/main.goreplacing client-level timeouts - Added comprehensive error logging for failed HTTP requests in document upload handler
- Updated PCR0 and PCR2 values in both
pcrDev.jsonandpcrProd.jsonto reflect new enclave measurements - PCR signatures still need to be updated in history files before deployment
3 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
| // Check file size (limit to 10MB) | ||
| if header.Size > 10*1024*1024 { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "File size exceeds 10MB limit"}) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
style: Extract 10MB limit to a named constant at package level for better maintainability
AnthonyRonning
force-pushed
the
document-start
branch
from
23e52c7 to
50dd0ff
Compare
There was a problem hiding this comment.
PR Summary
Recent changes focus on reliability improvements and PCR measurement validation for the document upload feature.
- Updated PCR0 and PCR2 values in both dev and prod environments but missing required signature updates in pcrProdHistory.json
- Implemented 5-minute context-based timeout and error logging in
tinfoil-proxy/main.gofor better request handling - Continuum-proxy version bumped to v1.19.0 in
justfilewith updated SHA256 hash continuum-proxyfile incorrectly created as empty file instead of directory structure
6 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
AnthonyRonning
force-pushed
the
document-start
branch
from
50dd0ff to
025b687
Compare
There was a problem hiding this comment.
PR Summary
Modified PCR measurements and improved document handling infrastructure in the AWS Nitro Enclave environment.
- Missing cryptographic signatures for PCR measurements in
pcrProdHistory.json, required for maintaining a secure audit trail - Check required in
src/web/documents.rsto verify document upload service availability before processing requests - Updated base64 content validation should be documented in
docs/PCR_VERIFICATION.mdfor maintaining security standards
4 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tinfoil-proxy/main.go (3)
359-363: Extract file size limit to a named constant.The 10MB file size limit should be extracted to a package-level constant for better maintainability.
Add this constant at the package level:
+const MaxFileSize = 10 * 1024 * 1024 // 10MB + // Document upload service configurationThen update the size check:
- if header.Size > 10*1024*1024 { + if header.Size > MaxFileSize {
376-383: Fix field name mismatch - use "file" instead of "files".The form field name should be "file" to match the frontend and downstream service expectations.
-part, err := writer.CreateFormFile("files", header.Filename) +part, err := writer.CreateFormFile("file", header.Filename)
400-400: Use configuration instead of hardcoded URL.The URL should be constructed using
docUploadConfig.Enclavefor consistency with other services.-req, err := http.NewRequestWithContext(ctx, "POST", "https://doc-upload.model.tinfoil.sh/v1alpha/convert/file", &requestBody) +req, err := http.NewRequestWithContext(ctx, "POST", "https://"+docUploadConfig.Enclave+"/v1alpha/convert/file", &requestBody)
🧹 Nitpick comments (1)
tinfoil-proxy/main.go (1)
350-472: Consider adding file type validation and memory optimization.The current implementation accepts any file type and loads the entire file into memory. Consider adding:
- File type validation (e.g., PDF, DOCX, TXT)
- Streaming upload for large files to reduce memory usage
- Content-Type validation
Example file type validation:
// Add after file size check allowedTypes := map[string]bool{ "application/pdf": true, "text/plain": true, "application/vnd.openxmlformats-officedocument.wordprocessingml.document": true, } contentType := header.Header.Get("Content-Type") if contentType == "" { // Detect content type from file extension or content } if !allowedTypes[contentType] { c.JSON(http.StatusBadRequest, gin.H{"error": "Unsupported file type"}) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.jsontinfoil-proxy/dist/tinfoil-proxyis excluded by!**/dist/**
📒 Files selected for processing (2)
src/web/documents.rs(1 hunks)tinfoil-proxy/main.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/documents.rs
⏰ Context from checks skipped due to timeout of 100000ms (1)
- GitHub Check: Development Reproducible Build
🔇 Additional comments (6)
tinfoil-proxy/main.go (6)
4-5: LGTM! New imports are appropriate for the document upload functionality.The added imports (
bytes,context,io,mime/multipart,time) are all necessary for implementing the multipart file upload handling and HTTP request functionality.Also applies to: 8-8, 10-10, 13-13
48-55: LGTM! Document upload configuration is well-structured.The configuration struct follows the same pattern as the existing model configurations and provides the necessary enclave and repository information.
111-120: LGTM! Request and response structs are appropriately defined.The structs provide clear interfaces for the document upload functionality with proper JSON tags.
123-126: LGTM! Server struct extension is logical.Adding the document upload clients to the server struct maintains consistency with the existing architecture.
165-193: LGTM! Document upload service initialization is robust.The initialization properly handles errors without failing the entire server startup, includes enclave verification, and provides appropriate logging.
621-622: LGTM! Endpoint registration is correctly implemented.The document upload endpoint is properly registered with the appropriate HTTP method and handler.
AnthonyRonning
force-pushed
the
document-start
branch
from
025b687 to
92e9194
Compare
There was a problem hiding this comment.
PR Summary
Updated PCR (Platform Configuration Register) measurements and history files to reflect document upload feature integration in AWS Nitro Enclaves.
- Added new PCR history entry with proper cryptographic signature and timestamp in
pcrProdHistory.jsonandpcrDevHistory.json - PCR1 remains constant across updates, indicating stable core system configuration
- Updated PCR0 and PCR2 values in both
pcrDev.jsonandpcrProd.jsonto reflect new document handling capabilities
4 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
.gitignorerule to exclude the Tinfoil proxy binary outside of the distribution directory.