Skip to content

Harden upload and lazy endpoints - #9

Merged
xentixar merged 2 commits into
4.xfrom
fix/upload-and-lazy-security
Oct 30, 2026
Merged

xentixar merged 2 commits into
4.xfrom
fix/upload-and-lazy-security

Conversation

@abdulmajidcse

@abdulmajidcse abdulmajidcse commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes two security issues in the request endpoints.

Uploads (/ripple/upload)

  • The component id i is now validated ([A-Za-z0-9_-]{1,64}) before it is used as a storage folder. Invalid ids return 400.
  • Uploads must match a configurable extension allowlist, file_upload.allowed_extensions (RIPPLE_FILE_ALLOWED_EXTENSIONS, array or comma-separated). The default covers common images, documents, archives and media. Anything else is rejected with 422.
  • Server-executable or config extensions (php*, phtml, phar, htaccess, ini, cgi, py, sh, …) are always rejected, even if listed, including double extensions such as x.php.jpg.
  • The stored temporary file extension is lowercased and limited to [a-z0-9]{1,16}.
  • The same checks run inside FileUploadService::storeTemporary() for direct callers.

Lazy loading (/ripple/lazy)

  • Lazy placeholders now carry a signature over class, id and mount params (data-ripple-lx), which the client sends back as x.
  • /ripple/lazy answers 403 with {"ok": false, "message": …} when the signature does not match, so mount params (which may set #[Locked] properties) can no longer be supplied by the client.
  • loadLazy() refuses components without #[Lazy].
  • The client keeps the placeholder when a lazy load is rejected instead of blanking the component.
  • Lazy signatures use a derived key and a JSON-encoded payload, so no state signature can ever validate as a lazy signature.

Notes

  • Pages rendered before upgrading have unsigned placeholders; those lazy components load again after a page reload.
  • Apps that accept other file types must add them to allowed_extensions; uploads without an extension are rejected.
  • resources/dist/ripple.min.js is rebuilt from source.

Tests

  • 11 new tests covering forged/missing lazy signatures, non-lazy classes, signature separation, traversal ids, the allowlist and blocked extensions.
  • composer test (156 tests), composer analyse and composer cs-check pass.

- Validate the upload component id before it is used in the storage path
- Reject server-executable file extensions, including double extensions
- Sign lazy placeholders (class, id, params) and verify on /ripple/lazy
- Refuse to lazy-load components without #[Lazy]
- Use a derived key for lazy signatures so state signatures never match

@xentixar xentixar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdulmajidcse
Thanks, looks good overall. Two nits before I approve:

  1. Invalid lazy signatures should return 403, not 500.
  2. Prefer an upload extension allowlist (configurable) over the current denylist.

- Throw InvalidSignatureException and answer /ripple/lazy with 403 JSON
- Keep the lazy placeholder when the load is rejected
- Add configurable file_upload.allowed_extensions (RIPPLE_FILE_ALLOWED_EXTENSIONS)
- Keep executable extensions blocked even when listed in the allowlist
@abdulmajidcse

Copy link
Copy Markdown
Member Author

@xentixar Thanks for the review. Both points are addressed in f195fd1:

  1. 403 for invalid lazy signatures. /ripple/lazy now returns 403 with {"ok": false, "message": "Invalid Ripple lazy signature."} (new InvalidSignatureException). The client keeps the placeholder when a load is rejected instead of blanking the component.
  2. Configurable extension allowlist. New file_upload.allowed_extensions setting (RIPPLE_FILE_ALLOWED_EXTENSIONS, array or comma-separated). The default covers common images, documents, archives and media; anything else returns 422. I kept the executable-extension block (php*, phar, htaccess, …) as a fixed backstop, so those stay rejected even if someone adds them to the allowlist. Happy to drop that if you'd rather have the allowlist alone.

Tests are updated (156 passing), and CI is green. Ready for another look.

@xentixar xentixar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xentixar
xentixar merged commit 7ed6efc into 4.x Oct 30, 2026
3 checks passed
@xentixar
xentixar deleted the fix/upload-and-lazy-security branch October 30, 2026 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants