✨(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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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🎯 4 (Complex) | ⏱️ ~45 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
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
lunika
reviewed
May 18, 2026
Member
lunika
left a comment
lunika
left a comment
There was a problem hiding this comment.
Just one comment about S3: it is not optional to have a bucket storage; without it, you cannot save a docs content.
There was a problem hiding this comment.
♻️ Duplicate comments (12)
docs/installation/README.md (2)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse descriptive link text instead of "here".
Both lines 12 and 17 use "here" as link text, which reduces accessibility and triggers markdown lint warnings.
Also applies to: 17-17
🤖 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 `@docs/installation/README.md` at line 12, Replace non-descriptive "here" link text in docs/installation/README.md (both occurrences around lines 12 and 17) with meaningful, accessible link text that describes the target (e.g., "Compose installation instructions" or "Docker Compose setup") so the link to /docs/installation/compose.md reads descriptively; update both instances to avoid markdown-lint warnings and improve accessibility.
15-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a blank line after the Scalingo heading.
The heading should be followed by a blank line to comply with markdown spacing rules (MD022).
🤖 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 `@docs/installation/README.md` around lines 15 - 16, The "## Scalingo" Markdown heading is missing a following blank line which violates MD022; open the README and add a single blank line immediately after the heading "## Scalingo" so the heading is separated from the following paragraph to restore proper Markdown spacing.docs/installation/scalingo.md (2)
169-171: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winProvide a safer file-based alternative for complex JSON configuration.
The single-line JSON command on line 170 is fragile due to shell escaping complexity and difficult to maintain.
Suggested alternative approach
Add this guidance after the current example:
**Alternative (recommended for complex JSON):** For easier editing and to avoid escaping issues, save your theme JSON to a file and use command substitution: 1. Create a file `theme.json` with your theme configuration: ```json { "footer": { "default": { "externalLinks": [...] } } }
- Set the environment variable from the file:
scalingo env-set THEME_CUSTOMIZATION_JSON="$(cat theme.json)"This approach is less error-prone and makes it easier to validate your JSON locally before deployment.
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/installation/scalingo.mdaround lines 169 - 171, Replace the fragile
single-line scalingo env-set example with a safer file-based workflow: advise
users to save the complex JSON into a local file (e.g., theme.json) and then set
THEME_CUSTOMIZATION_JSON using command substitution (scalingo env-set
THEME_CUSTOMIZATION_JSON="$(cat theme.json)"), mentioning
THEME_CUSTOMIZATION_JSON and the scalingo env-set command so readers know where
to put the file and how to apply it; note this avoids shell-escaping issues and
makes local validation of theme.json easier.</details> --- `97-97`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Use more descriptive link text for the Scalingo email documentation.** The link text currently displays the full URL. Consider using descriptive text instead. <details> <summary>Suggested doc fix</summary> ```diff -For email notifications see [https://doc.scalingo.com/platform/app/sending-emails](https://doc.scalingo.com/platform/app/sending-emails): +For email notifications, see the [Scalingo email sending documentation](https://doc.scalingo.com/platform/app/sending-emails):🤖 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 `@docs/installation/scalingo.md` at line 97, Replace the raw URL text with descriptive link text: change the link in the sentence that currently shows "https://doc.scalingo.com/platform/app/sending-emails" to a human-friendly label such as "Scalingo documentation on sending emails" (or "Scalingo: Sending emails") while keeping the same URL target; update the markdown link so the visible text is the descriptive label and the href remains the original URL.deploy/paas/servers.conf.erb (2)
87-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict
/media-authto internal exact-match auth subrequests.Line 87 exposes the auth endpoint publicly. This handler should be internal-only for
auth_request.Patch
- 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 block is currently publicly exposed; change it to be internal-only and to match exact URI for auth subrequests by making the location directive internal and using an exact-match modifier so only nginx auth_request can reach it; update the location /media-auth line to an exact internal location (preserving the proxy_pass, proxy_pass_request_body off, and header settings like proxy_set_header X-Original-URL and X-Original-Method) so external requests are rejected and only auth subrequests succeed.
104-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix UUID docs rewrite:
[id]is a literal path, not a capture.Line 105 points to
/docs/[id]/index.htmlliterally, so UUID routes will not resolve as intended.Patch
- 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; + location ~ "^(/docs/[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})/?$" { + rewrite ^ $1/index.html break; + try_files $uri =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 - 105, The rewrite is using a literal "/docs/[id]/index.html" instead of a regex capture; update the regex in the location block to capture the UUID and reference that capture in try_files. For example, change the location pattern to include parentheses around the UUID (the existing location ~ "^/docs/([0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})/?$") and change the try_files line to use the capture ($1) like try_files $uri /docs/$1/index.html so the UUID segment is forwarded correctly; adjust the existing location block and the try_files directive accordingly.src/backend/impress/settings.py (1)
87-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix DB selection logic: current conditional is always truthy.
Line 88-Line 90 tests a descriptor object, not env presence; fallback config never executes.
Patch
- "default": dj_database_url.config() - if values.DatabaseURLValue( - None, environ_name="DATABASE_URL", environ_prefix=None - ) - else { + "default": dj_database_url.config(default=None) + or {🤖 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 87 - 90, The DB selection conditional is testing the descriptor object values.DatabaseURLValue instead of the actual environment variable, so the fallback never runs; change the conditional to check the actual env var (e.g., os.environ.get("DATABASE_URL") or similar) and use dj_database_url.config() only when that env var is present. Locate the expression involving values.DatabaseURLValue(...) and dj_database_url.config() in settings.py and replace the truthiness test of the descriptor with a real environment presence check (import os if needed) so the fallback path executes correctly.bin/buildpack_postfrontend.sh (3)
38-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix SVG detection regex: current patterns include literal quotes.
Line 38 and Line 39 look for
"<svg"/"<?xml"(with quote chars), so valid SVG files are rejected.Minimal fix
- [[ "$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 SVG detection mistakenly matches literal quote characters; update the regex checks that set IS_SVG by removing the embedded double quotes and correctly matching the tags in HEADER. Specifically, change the two tests that reference HEADER/IS_SVG so the first checks for <svg (e.g. HEADER contains <svg) and the second checks for an XML prolog plus an SVG tag (e.g. HEADER contains <?xml and <svg), using proper regex escaping for the <?xml token if needed; update the lines that currently contain the quoted patterns to these two corrected checks.
19-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHarden SSRF controls beyond hostname regex checks.
Line 19 and Line 20 only validate a parsed hostname string and do not safely handle credentials, multi-record DNS, or private/special resolved IPs. This is still bypassable for internal-target SSRF.
Suggested direction
- HOSTNAME=$(echo "$THEME_CUSTOMIZATION_LOGO_URL" | sed -E 's|^https://([^/:]+).*|\1|') - [[ "$HOSTNAME" =~ ^(localhost|127\.|10\.|172\.(1[6-9]|2[0-9]|3[01])\.|192\.168\.|0\.0\.0\.0|\[::1\]) ]] && echo "[custom-logo] ERROR: SSRF blocked: $HOSTNAME" >&2 && exit 1 + # Parse URL robustly, reject credentials, resolve A/AAAA, and deny private/special ranges. + # Deny if any resolved address is internal/link-local/loopback/unspecified.🤖 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 check only inspects the parsed HOSTNAME from THEME_CUSTOMIZATION_LOGO_URL and can be bypassed; update the validation around THEME_CUSTOMIZATION_LOGO_URL/HOSTNAME to (1) strictly allow only https scheme and reject any URL containing user:pass credentials, (2) parse the URL robustly (scheme, host, port) rather than naive sed, (3) perform DNS resolution of the host to all A/AAAA addresses (e.g., via getent/host/python) and reject if any resolved IP falls into private/loopback/metadata ranges, and (4) reject hosts that resolve to multiple addresses if any are disallowed; replace the single regex HOSTNAME check with these steps and ensure errors are logged to stderr and the script exits non-zero on failure.
51-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
THEME_CUSTOMIZATION_JSONbefore writing to disk.Line 54 writes raw env content directly; malformed JSON will fail later at runtime with poor diagnostics.
Minimal fix
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 + echo "$THEME_CUSTOMIZATION_JSON" | python -c 'import json,sys;json.loads(sys.stdin.read())' >/dev/null + mkdir -p impress/configuration/theme + echo "$THEME_CUSTOMIZATION_JSON" >impress/configuration/theme/default.json 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 - 54, The script writes THEME_CUSTOMIZATION_JSON directly to impress/configuration/theme/default.json without validating it; validate THEME_CUSTOMIZATION_JSON before writing by parsing it (e.g., via jq or a short python -c json.loads) and if parsing fails emit a clear error to stderr (including the env var name) and exit non‑zero, otherwise create the directory and write the validated content to default.json; update the block that references THEME_CUSTOMIZATION_JSON to perform this check and error handling so malformed JSON is caught early.bin/buildpack_start.sh (2)
16-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate failing child exit status after
wait -n.Line 16-Line 18 currently kills siblings but may hide the original failure code, reducing platform observability and restart correctness.
Patch
-wait -n +wait -n +status=$? # then kill all the other tasks pkill -P $$ +exit $status🤖 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 16 - 18, After wait -n returns, capture its exit status (e.g. status=$?) before killing sibling processes so the original child's failure code isn't lost; then run pkill -P $$ to terminate other tasks and exit the script with the captured status (exit $status) so the failing child's exit code is propagated to the platform.
13-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix SIGTERM trap syntax and delayed expansion.
Line 13 should use
pkill -SIGTERMand single quotes so$$is evaluated on signal handling, not at trap definition.Patch
-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 current trap uses double quotes and the wrong pkill flag which causes $$ to be expanded at definition time and passes SIGTERM as an argument; update the trap command that currently uses trap "pkill SIGTERM -P $$" SIGTERM so it uses pkill with the -SIGTERM flag and is wrapped in single quotes (so $$ is evaluated when the signal is delivered), e.g. change the trap invocation to use 'pkill -SIGTERM -P $$' for SIGTERM handling.
🤖 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.
Duplicate comments:
In `@bin/buildpack_postfrontend.sh`:
- Around line 38-39: The SVG detection mistakenly matches literal quote
characters; update the regex checks that set IS_SVG by removing the embedded
double quotes and correctly matching the tags in HEADER. Specifically, change
the two tests that reference HEADER/IS_SVG so the first checks for <svg (e.g.
HEADER contains <svg) and the second checks for an XML prolog plus an SVG tag
(e.g. HEADER contains <?xml and <svg), using proper regex escaping for the <?xml
token if needed; update the lines that currently contain the quoted patterns to
these two corrected checks.
- Around line 19-20: The current check only inspects the parsed HOSTNAME from
THEME_CUSTOMIZATION_LOGO_URL and can be bypassed; update the validation around
THEME_CUSTOMIZATION_LOGO_URL/HOSTNAME to (1) strictly allow only https scheme
and reject any URL containing user:pass credentials, (2) parse the URL robustly
(scheme, host, port) rather than naive sed, (3) perform DNS resolution of the
host to all A/AAAA addresses (e.g., via getent/host/python) and reject if any
resolved IP falls into private/loopback/metadata ranges, and (4) reject hosts
that resolve to multiple addresses if any are disallowed; replace the single
regex HOSTNAME check with these steps and ensure errors are logged to stderr and
the script exits non-zero on failure.
- Around line 51-54: The script writes THEME_CUSTOMIZATION_JSON directly to
impress/configuration/theme/default.json without validating it; validate
THEME_CUSTOMIZATION_JSON before writing by parsing it (e.g., via jq or a short
python -c json.loads) and if parsing fails emit a clear error to stderr
(including the env var name) and exit non‑zero, otherwise create the directory
and write the validated content to default.json; update the block that
references THEME_CUSTOMIZATION_JSON to perform this check and error handling so
malformed JSON is caught early.
In `@bin/buildpack_start.sh`:
- Around line 16-18: After wait -n returns, capture its exit status (e.g.
status=$?) before killing sibling processes so the original child's failure code
isn't lost; then run pkill -P $$ to terminate other tasks and exit the script
with the captured status (exit $status) so the failing child's exit code is
propagated to the platform.
- Line 13: The current trap uses double quotes and the wrong pkill flag which
causes $$ to be expanded at definition time and passes SIGTERM as an argument;
update the trap command that currently uses trap "pkill SIGTERM -P $$" SIGTERM
so it uses pkill with the -SIGTERM flag and is wrapped in single quotes (so $$
is evaluated when the signal is delivered), e.g. change the trap invocation to
use 'pkill -SIGTERM -P $$' for SIGTERM handling.
In `@deploy/paas/servers.conf.erb`:
- Around line 87-98: The /media-auth location block is currently publicly
exposed; change it to be internal-only and to match exact URI for auth
subrequests by making the location directive internal and using an exact-match
modifier so only nginx auth_request can reach it; update the location
/media-auth line to an exact internal location (preserving the proxy_pass,
proxy_pass_request_body off, and header settings like proxy_set_header
X-Original-URL and X-Original-Method) so external requests are rejected and only
auth subrequests succeed.
- Around line 104-105: The rewrite is using a literal "/docs/[id]/index.html"
instead of a regex capture; update the regex in the location block to capture
the UUID and reference that capture in try_files. For example, change the
location pattern to include parentheses around the UUID (the existing location ~
"^/docs/([0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})/?$")
and change the try_files line to use the capture ($1) like try_files $uri
/docs/$1/index.html so the UUID segment is forwarded correctly; adjust the
existing location block and the try_files directive accordingly.
In `@docs/installation/README.md`:
- Line 12: Replace non-descriptive "here" link text in
docs/installation/README.md (both occurrences around lines 12 and 17) with
meaningful, accessible link text that describes the target (e.g., "Compose
installation instructions" or "Docker Compose setup") so the link to
/docs/installation/compose.md reads descriptively; update both instances to
avoid markdown-lint warnings and improve accessibility.
- Around line 15-16: The "## Scalingo" Markdown heading is missing a following
blank line which violates MD022; open the README and add a single blank line
immediately after the heading "## Scalingo" so the heading is separated from the
following paragraph to restore proper Markdown spacing.
In `@docs/installation/scalingo.md`:
- Around line 169-171: Replace the fragile single-line scalingo env-set example
with a safer file-based workflow: advise users to save the complex JSON into a
local file (e.g., theme.json) and then set THEME_CUSTOMIZATION_JSON using
command substitution (scalingo env-set THEME_CUSTOMIZATION_JSON="$(cat
theme.json)"), mentioning THEME_CUSTOMIZATION_JSON and the scalingo env-set
command so readers know where to put the file and how to apply it; note this
avoids shell-escaping issues and makes local validation of theme.json easier.
- Line 97: Replace the raw URL text with descriptive link text: change the link
in the sentence that currently shows
"https://doc.scalingo.com/platform/app/sending-emails" to a human-friendly label
such as "Scalingo documentation on sending emails" (or "Scalingo: Sending
emails") while keeping the same URL target; update the markdown link so the
visible text is the descriptive label and the href remains the original URL.
In `@src/backend/impress/settings.py`:
- Around line 87-90: The DB selection conditional is testing the descriptor
object values.DatabaseURLValue instead of the actual environment variable, so
the fallback never runs; change the conditional to check the actual env var
(e.g., os.environ.get("DATABASE_URL") or similar) and use
dj_database_url.config() only when that env var is present. Locate the
expression involving values.DatabaseURLValue(...) and dj_database_url.config()
in settings.py and replace the truthiness test of the descriptor with a real
environment presence check (import os if needed) so the fallback path executes
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2d40a2e-e6a2-4445-a187-9c91489c92a1
⛔ 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
lunika
approved these changes
May 18, 2026
add PaaS deployment scripts, tested on Scalingo
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/backend/impress/settings.py`:
- Around line 87-91: post_setup currently overwrites
DATABASES["default"]["OPTIONS"] when DB_PSYCOPG_POOL_ENABLED is true, discarding
URL-derived options from dj_database_url.config(); change post_setup to
merge/update the existing OPTIONS instead of replacing them: read the existing
options from DATABASES["default"].get("OPTIONS", {}), copy/update that dict with
the pool-specific keys before assigning back, so URL parameters like sslmode are
preserved when enabling pooling; reference the post_setup function,
dj_database_url.config(), and DATABASES["default"]["OPTIONS"] in your change.
🪄 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: 00337e37-214b-4d5d-9a73-efbfb4acc777
⛔ 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
Comment on lines
+87
to
+91
| "default": dj_database_url.config() | ||
| if values.DatabaseURLValue( | ||
| None, environ_name="DATABASE_URL", environ_prefix=None | ||
| ) | ||
| else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the settings file and examine lines around 87-91
cat -n src/backend/impress/settings.py | head -120 | tail -60Repository: suitenumerique/docs
Length of output: 2376
🏁 Script executed:
# Also search for the post_setup method
rg "def post_setup" src/backend/impress/settings.py -A 30Repository: suitenumerique/docs
Length of output: 1253
🏁 Script executed:
# Search for DB_PSYCOPG_POOL_ENABLED usage
rg "DB_PSYCOPG_POOL_ENABLED\|psycopg_pool" src/backend/impress/settings.py -B 2 -A 5Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
# Get the complete post_setup method - it seems to have been cut off
rg "def post_setup" src/backend/impress/settings.py -A 100Repository: suitenumerique/docs
Length of output: 3571
🏁 Script executed:
# Search for where OPTIONS are assigned in the database config
rg "OPTIONS" src/backend/impress/settings.py -B 2 -A 2Repository: suitenumerique/docs
Length of output: 1210
Preserve URL-derived DB options when pooling is enabled.
When dj_database_url.config() is used as the database configuration source, URL parameters such as sslmode are stored under DATABASES["default"]["OPTIONS"]. The post_setup() method replaces OPTIONS wholesale with .update({"OPTIONS": {...}}) when DB_PSYCOPG_POOL_ENABLED=true, silently dropping any URL-derived settings. This causes managed Postgres connections to fail once pooling is enabled.
Suggested fix
if psycopg_pool_enabled:
- cls.DATABASES["default"].update(
- {
- "OPTIONS": {
- # https://www.psycopg.org/psycopg3/docs/api/pool.html#psycopg_pool.ConnectionPool
- "pool": {
- "min_size": values.IntegerValue(
- 4,
- environ_name="DB_PSYCOPG_POOL_MIN_SIZE",
- environ_prefix=None,
- ),
- "max_size": values.IntegerValue(
- None,
- environ_name="DB_PSYCOPG_POOL_MAX_SIZE",
- environ_prefix=None,
- ),
- "timeout": values.IntegerValue(
- 3,
- environ_name="DB_PSYCOPG_POOL_TIMEOUT",
- environ_prefix=None,
- ),
- }
- },
- }
- )
+ options = dict(cls.DATABASES["default"].get("OPTIONS", {}))
+ options["pool"] = {
+ # https://www.psycopg.org/psycopg3/docs/api/pool.html#psycopg_pool.ConnectionPool
+ "min_size": values.IntegerValue(
+ 4,
+ environ_name="DB_PSYCOPG_POOL_MIN_SIZE",
+ environ_prefix=None,
+ ),
+ "max_size": values.IntegerValue(
+ None,
+ environ_name="DB_PSYCOPG_POOL_MAX_SIZE",
+ environ_prefix=None,
+ ),
+ "timeout": values.IntegerValue(
+ 3,
+ environ_name="DB_PSYCOPG_POOL_TIMEOUT",
+ environ_prefix=None,
+ ),
+ }
+ cls.DATABASES["default"]["OPTIONS"] = options🤖 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 87 - 91, post_setup currently
overwrites DATABASES["default"]["OPTIONS"] when DB_PSYCOPG_POOL_ENABLED is true,
discarding URL-derived options from dj_database_url.config(); change post_setup
to merge/update the existing OPTIONS instead of replacing them: read the
existing options from DATABASES["default"].get("OPTIONS", {}), copy/update that
dict with the pool-specific keys before assigning back, so URL parameters like
sslmode are preserved when enabling pooling; reference the post_setup function,
dj_database_url.config(), and DATABASES["default"]["OPTIONS"] in your change.
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
3 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