-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for other asset stores than S3 #59
Changes from 6 commits
3b1ca23
aa34e7b
6602b92
201250c
e7c5950
c6b1e04
7ec75ff
32a2378
20b763d
f39e7ff
13f300d
8042d9a
d9f268c
3c4364e
5aaa0f3
93631de
c670d5e
6acb777
03dce6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,32 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
services: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
hollo: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
image: ghcr.io/dahlia/hollo:canary | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dahlia marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ports: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- "3000:3000" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
environment: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DATABASE_URL: "postgres://user:password@postgres:5432/database" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dahlia marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SECRET_KEY: "${SECRET_KEY}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
LOG_LEVEL: "${LOG_LEVEL}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BEHIND_PROXY: "${BEHIND_PROXY}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DRIVE_DISK: fs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ASSET_URL_BASE: http://localhost:3000/assets/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
FS_ASSET_PATH: /var/lib/hollo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
depends_on: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- postgres | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
volumes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- assets_data:/var/lib/hollo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
restart: unless-stopped | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add health check and improve service configuration. Consider the following improvements:
ports:
- - "3000:3000"
+ - "127.0.0.1:3000:3000"
+ healthcheck:
+ test: ["CMD", "curl", "-f", "http://localhost:3000/health"]
+ interval: 30s
+ timeout: 10s
+ retries: 3
+ deploy:
+ resources:
+ limits:
+ memory: 1G 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
postgres: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
image: postgres:17 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
environment: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
POSTGRES_USER: user | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
POSTGRES_PASSWORD: password | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
POSTGRES_DB: database | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
volumes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- postgres_data:/var/lib/postgresql/data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
restart: unless-stopped | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve Postgres service security and reliability. Several improvements are recommended for the Postgres service:
postgres:
image: postgres:17
environment:
- POSTGRES_USER: user
- POSTGRES_PASSWORD: password
- POSTGRES_DB: database
+ POSTGRES_USER: ${POSTGRES_USER}
+ POSTGRES_PASSWORD: ${POSTGRES_PASSWORD}
+ POSTGRES_DB: ${POSTGRES_DB}
+ POSTGRES_INITDB_ARGS: "--auth-host=scram-sha-256"
+ healthcheck:
+ test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER} -d ${POSTGRES_DB}"]
+ interval: 10s
+ timeout: 5s
+ retries: 5
+ deploy:
+ resources:
+ limits:
+ memory: 512M 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
volumes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
postgres_data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
assets_data: |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,9 +8,10 @@ services: | |||||||||
SECRET_KEY: "${SECRET_KEY}" | ||||||||||
LOG_LEVEL: "${LOG_LEVEL}" | ||||||||||
BEHIND_PROXY: "${BEHIND_PROXY}" | ||||||||||
DRIVE_DISK: s3 | ||||||||||
ASSET_URL_BASE: http://localhost:9000/hollo/ | ||||||||||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review the storage configuration. There are several concerns with the current configuration:
Consider these changes: - DRIVE_DISK: s3
- ASSET_URL_BASE: http://localhost:9000/hollo/
+ DRIVE_DISK: ${DRIVE_DISK:-s3}
+ ASSET_URL_BASE: ${ASSET_URL_BASE:-http://localhost:9000/hollo/} 📝 Committable suggestion
Suggested change
|
||||||||||
S3_REGION: us-east-1 | ||||||||||
S3_BUCKET: hollo | ||||||||||
S3_URL_BASE: http://localhost:9000/hollo/ | ||||||||||
S3_ENDPOINT_URL: http://minio:9000 | ||||||||||
S3_FORCE_PATH_STYLE: "true" | ||||||||||
AWS_ACCESS_KEY_ID: minioadmin | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add documentation and example values for new storage configuration.
The new variables need documentation to guide users on proper configuration:
DRIVE_DISK
: Specify valid options (e.g., 's3', 'local')ASSET_URL_BASE
: Indicate format and HTTPS recommendationFS_ASSET_PATH
: Provide path format and permission requirementsApply this diff to add documentation:
📝 Committable suggestion
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Storage configuration requires additional security measures
The code review reveals several security considerations that need to be addressed:
FS_ASSET_PATH
is used withFSDriver
from theflydrive
package withvisibility: "public"
setting, but there are no explicit permission checks or directory security validationsASSET_URL_BASE
is used directly without URL sanitization, though it's only used as a base URL prefixDRIVE_DISK=fs
, assets are served directly throughserveStatic
middleware without additional security controlsRecommendations:
ASSET_URL_BASE
to ensure it's a valid URL and ends with a trailing slashFSDriver
to ensureFS_ASSET_PATH
has correct permissionsserveStatic
middlewareFS_ASSET_PATH
in the deployment guide🔗 Analysis chain
Verify security implications of new storage configuration.
The new storage configuration introduces potential security considerations:
FS_ASSET_PATH
is properly secured with appropriate permissionsASSET_URL_BASE
is properly sanitized to prevent XSSLet's check for any security-related code:
Let me gather more information about how these environment variables are used in the codebase:
Let me check one more aspect regarding file system security by looking at the FSDriver implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 1217
Script:
Length of output: 5126
Script:
Length of output: 308