✨(buildpack) add PaaS deployment support, tested with Scalingo#2293
Open
zeylos wants to merge 1 commit into
Open
✨(buildpack) add PaaS deployment support, tested with Scalingo#2293zeylos wants to merge 1 commit into
zeylos wants to merge 1 commit into
Conversation
WalkthroughAdds Scalingo buildpack deployment support: Procfile entries, post-compile and post-frontend buildpack scripts (frontend relocation, optional SSRF-protected custom logo download/validation, theme JSON injection, asset moves), a start script launching uvicorn, y-provider, and Nginx with shutdown handling, an ERB Nginx template for proxying/API/collaboration/media with auth_request, Django DATABASE_URL support via dj-database-url, a frontend build script update, and extensive Scalingo deployment documentation and README updates. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/buildpack_postcompile.sh`:
- Around line 8-9: The first rm command includes the redundant path
"src/frontend/apps/e2e" which is already removed by the subsequent "rm -rf
src/frontend/apps"; delete "src/frontend/apps/e2e" from the first rm list or
remove the entire first rm line so only the "rm -rf src/frontend/apps" remains,
keeping the other entries (docker, docs, env.d, gitlint) intact.
In `@bin/buildpack_postfrontend.sh`:
- Around line 19-20: The HOSTNAME extraction and SSRF check around
THEME_CUSTOMIZATION_LOGO_URL is insufficient and can be bypassed; replace the
fragile sed-based extraction with a proper URL parser to get the scheme, host
(without user:pass), and port, then perform DNS resolution of that host and
validate the resolved IP(s) instead of just the hostname; explicitly block all
private/special ranges (IPv4: 0.0.0.0/8, 10.0.0.0/8, 127.0.0.0/8,
169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16; IPv6: ::1, fe80::/10, fc00::/7,
etc.), handle multiple A/AAAA results and deny if any resolved address is in a
disallowed range, and prefer an allowlist of trusted hosts or use a
proxy/validation service; ensure malformed URLs or ones containing credentials
are rejected up-front by the parser rather than relying on sed.
- Around line 51-56: Validate THEME_CUSTOMIZATION_JSON before writing it to
impress/configuration/theme/default.json: parse the string with a JSON validator
(e.g., jq -e . or python -c 'import json,sys;json.loads(sys.stdin.read())') and
check the exit status; only create the directory and write the file if
validation succeeds, otherwise emit an error message (include the validation
tool's error) and exit non‑zero. Ensure you reference the environment variable
THEME_CUSTOMIZATION_JSON, the destination file
impress/configuration/theme/default.json, and the validation step before the
mkdir/echo steps.
- Around line 34-41: The SVG check is too weak: stop only reading the first 100
bytes (HEADER) and replace it with a full-file safe validation of TMP_FILE—read
the entire file, normalize case and strip XML/HTML comments, then verify the
root element is an actual <svg ...> tag (not just an occurrence inside
text/comment) and that no disallowed constructs exist (no <script> tags, no on*
event attributes, and no javascript: URIs); implement these checks using
TMP_FILE and the IS_SVG flag (or a new VALID_SVG variable) and fail with the
same error message if any check fails, and document that for production you
should use a dedicated sanitizer library.
In `@bin/buildpack_start.sh`:
- Around line 3-4: The uvicorn start command currently relies on the default
port; update the uvicorn invocation (the line launching
impress.asgi:application) to explicitly set the port (use --port or -p, e.g.,
--port=8000) so the server binding is unambiguous and easier to
configure/override in deployments.
- Around line 15-18: Add simple error reporting around the wait -n/ pkill -P $$
shutdown logic: after wait -n capture the exited child's PID and exit code (use
shell builtins like $!/$? or inspect /proc if needed), log a message identifying
the PID and its exit status (e.g., "service PID X exited with code Y") and
optionally the command (from /proc/<pid>/cmdline), then run pkill -P $$ to
terminate others; ensure the script exits with the same non-zero code so callers
know which service failed.
- Around line 12-13: The SIGTERM trap uses incorrect pkill syntax and expands $$
too early; update the trap command in buildpack_start.sh so it calls pkill with
the signal flag (pkill -SIGTERM -P $$) and wrap the whole trap string in single
quotes to defer expansion until the signal is handled (i.e., change the trap
line that currently references pkill and $$ to use single-quoted 'pkill -SIGTERM
-P $$').
In `@deploy/paas/servers.conf.erb`:
- Around line 87-98: The /media-auth location is publicly exposed; change it to
an internal, exact-match authentication handler by converting the block
referenced as "location /media-auth" to use exact matching ("location =
/media-auth") and add the "internal;" directive so it can only be invoked via
auth_request; keep the existing headers and proxy_pass (proxy_pass
http://backend_server/api/v1.0/documents/media-auth/) but ensure the location is
no longer externally reachable.
In `@docs/installation/README.md`:
- Line 12: Replace the non-descriptive link text "here" with a descriptive
phrase that explains the target (e.g., "Compose installation instructions" or
"Install with Docker Compose") for the links pointing to
/docs/installation/compose.md; update both occurrences (the "here" at line 12
and the similar one at line 17) in README.md so the markdown link text is
meaningful and accessible while keeping the same URL.
- Line 15: Add a blank line above the "## Scalingo" heading in
docs/installation/README.md to satisfy markdownlint MD022; locate the heading
line containing "## Scalingo" and insert a single empty line immediately before
it so the heading is preceded by a blank line.
In `@docs/installation/scalingo.md`:
- Line 95: Replace the raw URL in the sentence "For email notifications see
https://doc.scalingo.com/platform/app/sending-emails:" with a Markdown link
(e.g., [Scalingo: Sending
emails](https://doc.scalingo.com/platform/app/sending-emails)) so the text reads
as a descriptive clickable link; update the trailing colon as needed to match
surrounding sentence punctuation.
- Around line 167-169: The one-line scalingo env-set with
THEME_CUSTOMIZATION_JSON is fragile due to shell escaping; add a safer
file-based option: instruct users to save the JSON to a file (e.g., theme.json)
and set the env var using command substitution (scalingo env-set
THEME_CUSTOMIZATION_JSON="$(cat theme.json)") so multiline/complex JSON is
preserved; mention THEME_CUSTOMIZATION_JSON and the alternative steps (create
theme.json, run scalingo env-set with cat) in the docs near the existing
example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f5c83d6-e0d9-4965-a864-91c94bbdf27b
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Procfilebin/buildpack_postcompile.shbin/buildpack_postfrontend.shbin/buildpack_start.shdeploy/paas/servers.conf.erbdocs/installation/README.mddocs/installation/scalingo.mdsrc/backend/impress/settings.pysrc/backend/pyproject.tomlsrc/frontend/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/impress/settings.py (1)
86-110:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Database configuration fallback logic is broken.
The conditional uses
values.DatabaseURLValue(...)in a ternary expression at class definition time. This creates a Value object (which is always truthy), not the resolved environment variable value. Django-configurations only resolves Value objects during setup, after this ternary has already evaluated.Impact: The condition is always
True, sodj_database_url.config()is always called and theDB_*fallback is never used. WhenDATABASE_URLis not set,config()returns{}, causing Django to fail with an invalid database configuration.The class docstring (lines 65-68) explicitly mentions
DB_*variables as the expected configuration method, confirming the fallback is not just optional.🔧 Recommended fix using post_setup
# Database + DATABASE_URL = values.DatabaseURLValue( + None, environ_name="DATABASE_URL", environ_prefix=None + ) DATABASES = { - "default": dj_database_url.config() - if values.DatabaseURLValue( - None, environ_name="DATABASE_URL", environ_prefix=None - ) - else { + "default": { "ENGINE": values.Value( "django.db.backends.postgresql", environ_name="DB_ENGINE", environ_prefix=None, ), "NAME": values.Value( "impress", environ_name="DB_NAME", environ_prefix=None ), "USER": values.Value("dinum", environ_name="DB_USER", environ_prefix=None), "PASSWORD": SecretFileValue( "pass", environ_name="DB_PASSWORD", environ_prefix=None ), "HOST": values.Value( "localhost", environ_name="DB_HOST", environ_prefix=None ), "PORT": values.Value(5432, environ_name="DB_PORT", environ_prefix=None), # Psycopg pool can be configured in the post_setup method } }Then in
post_setup(around line 1136, before the Sentry block):`@classmethod` def post_setup(cls): """Post setup configuration. This is the place where you can configure settings that require other settings to be loaded. """ super().post_setup() + # Override database config with DATABASE_URL if provided + if cls.DATABASE_URL: + cls.DATABASES["default"] = dj_database_url.config() + # The SENTRY_DSN setting should be available to activate sentry for an environment if cls.SENTRY_DSN is not None:Alternative simpler approach using dj-database-url's built-in default:
# Database DATABASES = { - "default": dj_database_url.config() - if values.DatabaseURLValue( - None, environ_name="DATABASE_URL", environ_prefix=None - ) - else { + "default": dj_database_url.config( + default={ "ENGINE": values.Value( "django.db.backends.postgresql", environ_name="DB_ENGINE", environ_prefix=None, ), # ... rest of config - } + } + ) }However, this approach has a caveat: the
defaultdict is only used whenDATABASE_URLis completely absent. Thevalues.Valueobjects inside it are still evaluated even when not used, so allDB_*environment variables must be valid. Thepost_setupapproach is more robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/impress/settings.py` around lines 86 - 110, The DATABASES ternary currently evaluates values.DatabaseURLValue at class-definition time (always truthy), causing dj_database_url.config() to be called unconditionally and the DB_* fallback to be ignored; fix by moving resolution to the class post_setup method: in post_setup call dj_database_url.config() only after reading the resolved DATABASE_URL from the configuration (or self.values) and if it's empty/None, construct DATABASES from the existing values.Value/SecretFileValue DB_* entries (or assign the dict of DB_* defaults to self.DATABASES), otherwise set self.DATABASES to the dj_database_url.config() result; reference symbols: DATABASES, post_setup, dj_database_url.config, values.DatabaseURLValue, SecretFileValue.
♻️ Duplicate comments (7)
bin/buildpack_start.sh (2)
12-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix signal handling: incorrect pkill syntax and improper variable expansion.
Two critical issues:
- Incorrect pkill syntax: should be
pkill -SIGTERM -P $$(signal flag requires dash)- Improper variable expansion: double quotes cause
$$to expand at trap-set time, not when SIGTERM is receivedAs per static analysis hint from Shellcheck, use single quotes to defer expansion until the signal is actually received.
🐛 Fix signal handling
-trap "pkill SIGTERM -P $$" SIGTERM +trap 'pkill -SIGTERM -P $$' SIGTERM🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_start.sh` around lines 12 - 13, The trap invocation uses incorrect pkill syntax and expands $$ at trap-setup time; update the trap command (the line using trap and pkill) to call pkill with the signal flag (e.g., -SIGTERM) and ensure $$ is expanded only when the signal is delivered by wrapping the trap string in single quotes so the PID is evaluated at signal time; modify the trap that currently runs pkill SIGTERM -P $$ to use the correct signal flag and single-quoted command.
3-4: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider making the port explicit for clarity.
The uvicorn command relies on the default port (8000). Making it explicit would improve clarity and prevent confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_start.sh` around lines 3 - 4, The uvicorn start command currently relies on the default port; update the startup invocation (the uvicorn --app-dir=... --host=0.0.0.0 --timeout-graceful-shutdown=300 --limit-max-requests=20000 --lifespan=off impress.asgi:application) to explicitly specify the port (use --port or -p with the intended port number, e.g., --port=8000) so the bind port is clear and unambiguous when launching the ASGI app.deploy/paas/servers.conf.erb (1)
87-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
/media-authinternal-only to prevent direct external access.The location should only be used internally via
auth_requestdirectives. Addinternal;directive and use exact matching to align with Nginx best practices for authentication handlers.🔒 Make auth endpoint internal
- location /media-auth { + location = /media-auth { + internal; proxy_pass http://backend_server/api/v1.0/documents/media-auth/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/paas/servers.conf.erb` around lines 87 - 98, The /media-auth location must be made internal and exact-matching for auth_request usage: change the location block identifier from "location /media-auth" to an exact match (e.g., "location = /media-auth") and add the "internal;" directive inside the block so only internal requests (like auth_request) can reach the proxy_pass; keep the existing proxy_* settings and proxy_pass target unchanged.bin/buildpack_postfrontend.sh (3)
51-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider validating JSON structure before writing.
The script writes
$THEME_CUSTOMIZATION_JSONdirectly without validating it's well-formed JSON. Malformed JSON may cause runtime failures with unclear error messages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 51 - 56, The script writes THEME_CUSTOMIZATION_JSON directly which may be malformed; add JSON validation before writing by testing the variable with a JSON validator (e.g., jq -e '.' or python -c 'import json,sys; json.load(sys.stdin)') and if validation fails, log an error via the same prefix (e.g., "[custom-theme] ERROR: ..."), do not create/overwrite impress/configuration/theme/default.json, and exit non-zero; update the conditional block around THEME_CUSTOMIZATION_JSON to run the validator, write the file only on success, and include the failing validator's message in the error log.
34-41:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSVG validation is weak and can be bypassed.
Even after fixing the quote issue, the validation only checks the first 100 bytes and has no XSS prevention (missing checks for
<script>tags or event handlers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 34 - 41, The current SVG check using HEADER/IS_SVG and only reading head -c 100 of TMP_FILE is insufficient and can be bypassed; update the validation in buildpack_postfrontend.sh to read and parse the entire TMP_FILE (not just the first 100 bytes) and validate it as well-formed XML/SVG (use a safe XML validator like xmllint or an SVG-aware parser invoked from the script) and then explicitly scan the full content for disallowed constructs (reject if <script> tags or any “on...” event attributes like onclick/onload are present, and reject external entity references) so the functions/variables HEADER, IS_SVG and TMP_FILE are replaced/augmented by a full-file parse + explicit checks before accepting the file.
19-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSSRF protection is incomplete and can be bypassed.
The current IP blocking has several gaps: missing
169.254.0.0/16, limited IPv6 coverage, missing full0.0.0.0/8range, and no DNS rebinding prevention. Additionally, the hostname extraction could fail for URLs with authentication credentials.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 19 - 20, The HOSTNAME extraction from THEME_CUSTOMIZATION_LOGO_URL using sed and the subsequent regex (HOSTNAME and THEME_CUSTOMIZATION_LOGO_URL) is unsafe and incomplete for SSRF protection: replace the fragile sed extraction with a proper URL parse (strip userinfo and require http/https scheme) and obtain the canonical host (reject URLs with embedded credentials); then resolve the host to IP(s) and block any address in private or non-routable ranges including 0.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16 (link-local), ::1 and ::/128, fc00::/7 (ULA) and fe80::/10 (IPv6 link-local), and other non-routable blocks; also reject raw IP literals in those ranges and implement a DNS-rebinding check by re-resolving and re-checking IPs immediately before use; if any check fails, log "[custom-logo] ERROR: SSRF blocked: $HOSTNAME" to stderr and exit 1 as before.bin/buildpack_postcompile.sh (1)
8-9: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRedundant removal of
src/frontend/apps/e2e.Line 8 removes
src/frontend/apps/e2e, but line 9 removes the parent directorysrc/frontend/appsentirely, making the first removal redundant.♻️ Simplify by removing redundant path
-rm -rf docker docs env.d gitlint src/frontend/apps/e2e +rm -rf docker docs env.d gitlint rm -rf src/frontend/apps🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postcompile.sh` around lines 8 - 9, Remove the redundant deletion: the first command that runs "rm -rf docker docs env.d gitlint src/frontend/apps/e2e" is unnecessary because the subsequent "rm -rf src/frontend/apps" removes the parent directory; delete the earlier "src/frontend/apps/e2e" removal (i.e., remove or simplify the first rm -rf line) so only the broader "rm -rf src/frontend/apps" remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/buildpack_postfrontend.sh`:
- Around line 38-39: The regex checks use literal quote characters and thus
never match; update the two conditionals that set IS_SVG so they look for raw
tags instead of quoted strings: replace the pattern [[ "$HEADER" =~ ^.*"<svg".*$
]] with [[ "$HEADER" =~ <svg ]] && IS_SVG=true, and replace [[ "$HEADER" =~
^.*"<?xml".*"<svg".*$ ]] with a pattern that matches an XML prolog followed by
an svg tag (for example [[ "$HEADER" =~ \<\?xml.*<svg ]] && IS_SVG=true), so the
script correctly detects <svg and <?xml without surrounding literal quotes.
In `@deploy/paas/servers.conf.erb`:
- Around line 104-106: The location block matching UUIDs currently uses
try_files with a literal "[id]" instead of a capture; replace the try_files
usage with a rewrite that captures the UUID from the regex (use parentheses in
the pattern) and rewrites to /docs/$1/index.html, then add "break" to stop
further processing if needed; update the location block that contains the regex
and remove the incorrect try_files line, using a rewrite directive referencing
$1 so the UUID path maps to the correct index.html.
---
Outside diff comments:
In `@src/backend/impress/settings.py`:
- Around line 86-110: The DATABASES ternary currently evaluates
values.DatabaseURLValue at class-definition time (always truthy), causing
dj_database_url.config() to be called unconditionally and the DB_* fallback to
be ignored; fix by moving resolution to the class post_setup method: in
post_setup call dj_database_url.config() only after reading the resolved
DATABASE_URL from the configuration (or self.values) and if it's empty/None,
construct DATABASES from the existing values.Value/SecretFileValue DB_* entries
(or assign the dict of DB_* defaults to self.DATABASES), otherwise set
self.DATABASES to the dj_database_url.config() result; reference symbols:
DATABASES, post_setup, dj_database_url.config, values.DatabaseURLValue,
SecretFileValue.
---
Duplicate comments:
In `@bin/buildpack_postcompile.sh`:
- Around line 8-9: Remove the redundant deletion: the first command that runs
"rm -rf docker docs env.d gitlint src/frontend/apps/e2e" is unnecessary because
the subsequent "rm -rf src/frontend/apps" removes the parent directory; delete
the earlier "src/frontend/apps/e2e" removal (i.e., remove or simplify the first
rm -rf line) so only the broader "rm -rf src/frontend/apps" remains.
In `@bin/buildpack_postfrontend.sh`:
- Around line 51-56: The script writes THEME_CUSTOMIZATION_JSON directly which
may be malformed; add JSON validation before writing by testing the variable
with a JSON validator (e.g., jq -e '.' or python -c 'import json,sys;
json.load(sys.stdin)') and if validation fails, log an error via the same prefix
(e.g., "[custom-theme] ERROR: ..."), do not create/overwrite
impress/configuration/theme/default.json, and exit non-zero; update the
conditional block around THEME_CUSTOMIZATION_JSON to run the validator, write
the file only on success, and include the failing validator's message in the
error log.
- Around line 34-41: The current SVG check using HEADER/IS_SVG and only reading
head -c 100 of TMP_FILE is insufficient and can be bypassed; update the
validation in buildpack_postfrontend.sh to read and parse the entire TMP_FILE
(not just the first 100 bytes) and validate it as well-formed XML/SVG (use a
safe XML validator like xmllint or an SVG-aware parser invoked from the script)
and then explicitly scan the full content for disallowed constructs (reject if
<script> tags or any “on...” event attributes like onclick/onload are present,
and reject external entity references) so the functions/variables HEADER, IS_SVG
and TMP_FILE are replaced/augmented by a full-file parse + explicit checks
before accepting the file.
- Around line 19-20: The HOSTNAME extraction from THEME_CUSTOMIZATION_LOGO_URL
using sed and the subsequent regex (HOSTNAME and THEME_CUSTOMIZATION_LOGO_URL)
is unsafe and incomplete for SSRF protection: replace the fragile sed extraction
with a proper URL parse (strip userinfo and require http/https scheme) and
obtain the canonical host (reject URLs with embedded credentials); then resolve
the host to IP(s) and block any address in private or non-routable ranges
including 0.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16
(link-local), ::1 and ::/128, fc00::/7 (ULA) and fe80::/10 (IPv6 link-local),
and other non-routable blocks; also reject raw IP literals in those ranges and
implement a DNS-rebinding check by re-resolving and re-checking IPs immediately
before use; if any check fails, log "[custom-logo] ERROR: SSRF blocked:
$HOSTNAME" to stderr and exit 1 as before.
In `@bin/buildpack_start.sh`:
- Around line 12-13: The trap invocation uses incorrect pkill syntax and expands
$$ at trap-setup time; update the trap command (the line using trap and pkill)
to call pkill with the signal flag (e.g., -SIGTERM) and ensure $$ is expanded
only when the signal is delivered by wrapping the trap string in single quotes
so the PID is evaluated at signal time; modify the trap that currently runs
pkill SIGTERM -P $$ to use the correct signal flag and single-quoted command.
- Around line 3-4: The uvicorn start command currently relies on the default
port; update the startup invocation (the uvicorn --app-dir=... --host=0.0.0.0
--timeout-graceful-shutdown=300 --limit-max-requests=20000 --lifespan=off
impress.asgi:application) to explicitly specify the port (use --port or -p with
the intended port number, e.g., --port=8000) so the bind port is clear and
unambiguous when launching the ASGI app.
In `@deploy/paas/servers.conf.erb`:
- Around line 87-98: The /media-auth location must be made internal and
exact-matching for auth_request usage: change the location block identifier from
"location /media-auth" to an exact match (e.g., "location = /media-auth") and
add the "internal;" directive inside the block so only internal requests (like
auth_request) can reach the proxy_pass; keep the existing proxy_* settings and
proxy_pass target unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cadeebbe-95a7-46ae-b4b0-4c88c995a906
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdProcfilebin/buildpack_postcompile.shbin/buildpack_postfrontend.shbin/buildpack_start.shdeploy/paas/servers.conf.erbdocs/installation/README.mddocs/installation/scalingo.mdsrc/backend/impress/settings.pysrc/backend/pyproject.tomlsrc/frontend/package.json
add PaaS deployment scripts, tested on Scalingo
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
bin/buildpack_postfrontend.sh (4)
38-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Regex patterns contain literal quote characters that will never match valid SVG files.
The regex checks on these lines include literal double-quote characters (
"<svg"and"<?xml"), which will never match actual SVG content. Valid SVG files contain<svgor<?xmlwithout surrounding quotes, so the validation will always fail with "not a valid SVG file" error.🐛 Proposed fix to remove literal quotes from regex patterns
- [[ "$HEADER" =~ ^.*"<svg".*$ ]] && IS_SVG=true - [[ "$HEADER" =~ ^.*"<?xml".*"<svg".*$ ]] && IS_SVG=true + [[ "$HEADER" =~ <svg ]] && IS_SVG=true + [[ "$HEADER" =~ \<\?xml.*<svg ]] && IS_SVG=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 38 - 39, The regex checks for SVG detection use literal double-quote characters and therefore never match; update the two conditional expressions that inspect HEADER and set IS_SVG to remove the quoted patterns so they match real tags (match <svg and <?xml followed by <svg) — locate the conditionals that reference HEADER and IS_SVG and change the regexes to look for <svg and <?xml.*<svg without the surrounding quote characters so valid SVG content is correctly detected.
51-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider validating JSON structure before writing.
The script directly echoes
$THEME_CUSTOMIZATION_JSONto the file without validating it's well-formed JSON. If malformed JSON is provided, the application may fail at runtime with unclear error messages.✅ Add JSON validation
# Inject custom theme JSON if [ -n "$THEME_CUSTOMIZATION_JSON" ]; then echo "[custom-theme] INFO: Deploying the custom theme from THEME_CUSTOMIZATION_JSON." mkdir -p impress/configuration/theme - echo "$THEME_CUSTOMIZATION_JSON" >impress/configuration/theme/default.json + + # Validate JSON before writing + if echo "$THEME_CUSTOMIZATION_JSON" | python -m json.tool >/dev/null 2>&1; then + echo "$THEME_CUSTOMIZATION_JSON" >impress/configuration/theme/default.json + else + echo "[custom-theme] ERROR: THEME_CUSTOMIZATION_JSON is not valid JSON" >&2 + exit 1 + fi + echo "[custom-theme] INFO: Custom theme deployed successfully." fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 51 - 56, Validate THEME_CUSTOMIZATION_JSON before writing it to impress/configuration/theme/default.json: parse the contents (e.g., via jq -e or a small python -c "import json,sys; json.load(sys.stdin)") and only create the directory and write the file if the parse succeeds; if validation fails, log an error like "[custom-theme] ERROR: invalid JSON in THEME_CUSTOMIZATION_JSON" and exit non‑zero. Ensure the script checks availability of the chosen validator (jq or python) and falls back or errors clearly, and reference THEME_CUSTOMIZATION_JSON and the target path impress/configuration/theme/default.json when implementing the checks.
34-41:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSVG validation is weak and can be bypassed.
The current validation only checks the first 100 bytes for an
<svgsubstring, which can be easily bypassed:
- Large whitespace or comments before the
<svg>tag can push it beyond byte 100- No validation that the SVG is safe (could contain
<script>tags or event handlers)A malicious actor could upload a file with
<svgin a comment within the first 100 bytes, followed by malicious content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 34 - 41, The SVG validation in buildpack_postfrontend.sh (variables TMP_FILE, HEADER, IS_SVG) is too weak—change it to validate the entire file and to reject unsafe content: read the full TMP_FILE rather than only the first 100 bytes, verify it is a well-formed XML/SVG (e.g., run an XML validator like xmllint against TMP_FILE and ensure the root element is svg), and additionally scan TMP_FILE for disallowed constructs (e.g., <script> tags, javascript: URIs, and inline event handlers like attributes starting with "on") and fail if any are present; update the error messages to reflect these specific checks and remove the HEADER/100-byte check logic.
19-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSSRF protection is incomplete and can be bypassed.
The current IP blocking has several gaps:
- Only blocks
172.16-31.*(missing172.32-255.*)- Missing
169.254.0.0/16(link-local addresses)- Missing IPv6 private ranges
- DNS rebinding attacks aren't prevented (DNS resolution happens after this check)
Additionally, the hostname extraction via
sedcould fail for malformed URLs or URLs with authentication credentials.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_postfrontend.sh` around lines 19 - 20, The current HOSTNAME extraction from THEME_CUSTOMIZATION_LOGO_URL using sed and the single-regex SSRF check is unsafe; replace it by robustly parsing THEME_CUSTOMIZATION_LOGO_URL (reject URLs with embedded credentials or malformed schemes), extract the host (handle IPv6 literals and brackets), then perform DNS resolution on the hostname and validate every returned IP (and any literal IP in the URL) against a comprehensive denylist: 0.0.0.0/8, 127.0.0.0/8, 10.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16, link‑local and reserved IPv6 ranges (::1, fe80::/10, fc00::/7, ::/128, ff00::/8, etc.); if parsing, resolution, or any IP check fails, log an error including HOSTNAME and offending IP(s) and exit non‑zero to block the request (update the HOSTNAME assignment and the SSRF check logic accordingly).deploy/paas/servers.conf.erb (2)
87-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
/media-authinternal-only to prevent direct external access to the auth subrequest endpoint.The location at line 87 is currently exposed publicly but should only be used internally via
auth_requestdirectives. Add theinternal;directive and use exact matching (location =) to align with Nginx best practices for authentication handlers.Suggested fix
- location /media-auth { + location = /media-auth { + internal; proxy_pass http://backend_server/api/v1.0/documents/media-auth/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/paas/servers.conf.erb` around lines 87 - 98, The /media-auth location is publicly exposed; change its declaration from "location /media-auth" to an exact-match internal location by replacing it with "location = /media-auth { internal; ... }" so Nginx only allows internal auth_request subrequests to reach the proxy_pass to backend_server/api/v1.0/documents/media-auth/; keep the existing proxy_pass, proxy_set_header and proxy_pass_request_body/off lines intact inside the modified block and verify any auth_request directives target the exact "/media-auth" path.
104-106:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix broken UUID path rewrite - literal
[id]is not a capture group.The regex on line 104 matches document UUIDs, but line 105 uses the literal string
[id]instead of a captured variable. This will attempt to serve a file literally named/docs/[id]/index.html, which doesn't exist.Nginx regex captures require parentheses and
$1,$2, etc. references. However,location ~withtry_filescannot reference captures. You need to use arewritedirective instead.🐛 Fix UUID path rewrite
location ~ "^/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}/?$" { - try_files $uri /docs/[id]/index.html; + rewrite ^(/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})/?$ $1/index.html break; + try_files $uri =404; }Alternatively, if the intent is SPA fallback for all doc UUID paths:
location ~ "^/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}/?$" { - try_files $uri /docs/[id]/index.html; + try_files $uri $uri/index.html /index.html =404; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/paas/servers.conf.erb` around lines 104 - 106, The current location block using "location ~ \"^/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}/?$\"" incorrectly uses try_files with a literal "[id]"; change this to capture the UUID with parentheses (e.g. replace the bracketed pattern with a capture group) and use a rewrite directive (not try_files) to rewrite to the captured value like "/docs/$1/index.html" (or use rewrite ... last) because try_files cannot reference regex captures; update the block containing the location regex and try_files accordingly.bin/buildpack_start.sh (1)
13-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix signal handling: incorrect pkill syntax and improper variable expansion.
Two critical issues with the SIGTERM handler:
- Incorrect pkill syntax:
pkill SIGTERM -P $$should bepkill -SIGTERM -P $$(signal flag requires a dash).- Improper variable expansion: The double quotes cause
$$to expand immediately when the trap is set, not when SIGTERM is received. Use single quotes to defer expansion.🐛 Fix signal handling
# if the current shell is killed, also terminate all its children -trap "pkill SIGTERM -P $$" SIGTERM +trap 'pkill -SIGTERM -P $$' SIGTERM🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/buildpack_start.sh` at line 13, The trap handler currently uses incorrect pkill syntax and causes $$ to expand at trap definition time; update the SIGTERM trap invocation (the trap command that runs pkill) to use the correct signal flag and defer PID expansion by wrapping the handler in single quotes, e.g. invoke pkill with -SIGTERM and pass the parent PID via $$ evaluated at signal time (use 'pkill -SIGTERM -P $$' as the trap body) so pkill receives the proper signal flag and the PID is expanded when the trap runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/installation/README.md`:
- Around line 15-16: Add a single blank line after the "## Scalingo" markdown
heading so the heading is separated from the following paragraph; update the
section where the heading "Scalingo" appears to insert one empty line
immediately after that header to comply with Markdown heading spacing rules.
In `@src/backend/impress/settings.py`:
- Around line 87-90: The conditional currently tests a values.DatabaseURLValue
descriptor which is always truthy, causing dj_database_url.config() to be called
even when DATABASE_URL is unset; change the logic to call
dj_database_url.config(default=None) and use the returned value (which will be
None if DATABASE_URL is not set) with the or fallback to the explicit
configuration so the fallback executes when DATABASE_URL is absent; update the
expression around dj_database_url.config() and values.DatabaseURLValue to rely
on the config() return value rather than the descriptor's truthiness.
---
Duplicate comments:
In `@bin/buildpack_postfrontend.sh`:
- Around line 38-39: The regex checks for SVG detection use literal double-quote
characters and therefore never match; update the two conditional expressions
that inspect HEADER and set IS_SVG to remove the quoted patterns so they match
real tags (match <svg and <?xml followed by <svg) — locate the conditionals that
reference HEADER and IS_SVG and change the regexes to look for <svg and
<?xml.*<svg without the surrounding quote characters so valid SVG content is
correctly detected.
- Around line 51-56: Validate THEME_CUSTOMIZATION_JSON before writing it to
impress/configuration/theme/default.json: parse the contents (e.g., via jq -e or
a small python -c "import json,sys; json.load(sys.stdin)") and only create the
directory and write the file if the parse succeeds; if validation fails, log an
error like "[custom-theme] ERROR: invalid JSON in THEME_CUSTOMIZATION_JSON" and
exit non‑zero. Ensure the script checks availability of the chosen validator (jq
or python) and falls back or errors clearly, and reference
THEME_CUSTOMIZATION_JSON and the target path
impress/configuration/theme/default.json when implementing the checks.
- Around line 34-41: The SVG validation in buildpack_postfrontend.sh (variables
TMP_FILE, HEADER, IS_SVG) is too weak—change it to validate the entire file and
to reject unsafe content: read the full TMP_FILE rather than only the first 100
bytes, verify it is a well-formed XML/SVG (e.g., run an XML validator like
xmllint against TMP_FILE and ensure the root element is svg), and additionally
scan TMP_FILE for disallowed constructs (e.g., <script> tags, javascript: URIs,
and inline event handlers like attributes starting with "on") and fail if any
are present; update the error messages to reflect these specific checks and
remove the HEADER/100-byte check logic.
- Around line 19-20: The current HOSTNAME extraction from
THEME_CUSTOMIZATION_LOGO_URL using sed and the single-regex SSRF check is
unsafe; replace it by robustly parsing THEME_CUSTOMIZATION_LOGO_URL (reject URLs
with embedded credentials or malformed schemes), extract the host (handle IPv6
literals and brackets), then perform DNS resolution on the hostname and validate
every returned IP (and any literal IP in the URL) against a comprehensive
denylist: 0.0.0.0/8, 127.0.0.0/8, 10.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12,
192.168.0.0/16, link‑local and reserved IPv6 ranges (::1, fe80::/10, fc00::/7,
::/128, ff00::/8, etc.); if parsing, resolution, or any IP check fails, log an
error including HOSTNAME and offending IP(s) and exit non‑zero to block the
request (update the HOSTNAME assignment and the SSRF check logic accordingly).
In `@bin/buildpack_start.sh`:
- Line 13: The trap handler currently uses incorrect pkill syntax and causes $$
to expand at trap definition time; update the SIGTERM trap invocation (the trap
command that runs pkill) to use the correct signal flag and defer PID expansion
by wrapping the handler in single quotes, e.g. invoke pkill with -SIGTERM and
pass the parent PID via $$ evaluated at signal time (use 'pkill -SIGTERM -P $$'
as the trap body) so pkill receives the proper signal flag and the PID is
expanded when the trap runs.
In `@deploy/paas/servers.conf.erb`:
- Around line 87-98: The /media-auth location is publicly exposed; change its
declaration from "location /media-auth" to an exact-match internal location by
replacing it with "location = /media-auth { internal; ... }" so Nginx only
allows internal auth_request subrequests to reach the proxy_pass to
backend_server/api/v1.0/documents/media-auth/; keep the existing proxy_pass,
proxy_set_header and proxy_pass_request_body/off lines intact inside the
modified block and verify any auth_request directives target the exact
"/media-auth" path.
- Around line 104-106: The current location block using "location ~
\"^/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}/?$\""
incorrectly uses try_files with a literal "[id]"; change this to capture the
UUID with parentheses (e.g. replace the bracketed pattern with a capture group)
and use a rewrite directive (not try_files) to rewrite to the captured value
like "/docs/$1/index.html" (or use rewrite ... last) because try_files cannot
reference regex captures; update the block containing the location regex and
try_files accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa91f6ba-215e-4eec-b80b-f7bde05eb344
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdProcfilebin/buildpack_postcompile.shbin/buildpack_postfrontend.shbin/buildpack_start.shdeploy/paas/servers.conf.erbdocs/installation/README.mddocs/installation/scalingo.mdsrc/backend/impress/settings.pysrc/backend/pyproject.tomlsrc/frontend/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #1020
deploy/directory, making room for a deploy/docker directory laterTested successfuly on Scalingo