fix: add magic-byte and extension validation to payload upload in payload_api#3329
Open
fix: add magic-byte and extension validation to payload upload in payload_api#3329
Conversation
Block dangerous file types (PHP, JSP, ASPX) and restrict extensions.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds server-side payload upload validation using both filename extension checks and content “magic byte” detection, plus introduces unit tests for the new validation behavior.
Changes:
- Introduced
_validate_payload_file()with extension allowlist and dangerous-signature detection. - Wired validation into
post_payloads()by peeking at the first 16 bytes of the upload. - Added new security-focused unit tests covering extensions, magic bytes, and null-byte filenames.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/security/test_payload_validation.py | Adds unit tests for extension, magic-byte, and null-byte validation behaviors. |
| app/api/v2/handlers/payload_api.py | Implements payload validation logic and enforces it during upload handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/api/v2/handlers/payload_api.py
Outdated
Comment on lines
+16
to
+19
| ALLOWED_EXTENSIONS = [ | ||
| '.ps1', '.sh', '.py', '.exe', '.elf', '.bat', '.vbs', '.js', '.go', '.c', | ||
| '.zip', '.tar', '.gz', '.dll', '.bin', '.yaml', '.yml', '.txt', '.json', | ||
| ] |
app/api/v2/handlers/payload_api.py
Outdated
| ] | ||
|
|
||
| DANGEROUS_MAGIC_BYTES = [ | ||
| b'<?php', b'<%@', b'<%!', b'<%@ Page', |
app/api/v2/handlers/payload_api.py
Outdated
Comment on lines
98
to
107
| # Validate filename and magic bytes | ||
| first_bytes = file_field.file.read(16) | ||
| file_field.file.seek(0) | ||
| is_valid, error_msg = _validate_payload_file(file_field.filename, first_bytes) | ||
| if not is_valid: | ||
| raise web.HTTPBadRequest(text=error_msg) | ||
|
|
||
| # Sanitize the file name to prevent directory traversal | ||
| sanitized_filename = self.sanitize_filename(file_field.filename) | ||
|
|
Comment on lines
+2
to
+4
| import os | ||
| import sys | ||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..')) |
- Convert ALLOWED_EXTENSIONS list to frozenset for O(1) lookup and immutability - Remove redundant b'<%@ Page' magic bytes entry (b'<%@' already covers it) - Sanitize filename before validation so extension/null-byte checks use the same name that will be used for storage, closing potential bypass gap - Remove sys.path.insert() from test file; import module directly - Remove __main__ runner block from test module - Add test confirming b'<%@ Page' is still rejected through b'<%@' prefix match
Contributor
There was a problem hiding this comment.
Pull request overview
Adds content-based (magic-byte) and extension validation for payload uploads in payload_api, with accompanying unit tests to prevent disguised file types from being uploaded.
Changes:
- Introduced
ALLOWED_EXTENSIONSallowlist andDANGEROUS_MAGIC_BYTESsignature checks. - Added
_validate_payload_file()and wired it into the upload flow (validating sanitized filenames). - Added a new security-focused test suite for filename and content signatures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/api/v2/handlers/payload_api.py | Adds extension allowlist + magic-byte validation and enforces it during upload. |
| tests/security/test_payload_validation.py | Adds unit tests covering extension/magic-byte/null-byte cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| if '\x00' in filename: | ||
| return False, 'Null byte detected in filename' | ||
| ext = os.path.splitext(filename)[1].lower() |
Comment on lines
+36
to
+37
| for magic in DANGEROUS_MAGIC_BYTES: | ||
| if file_content_start.startswith(magic): |
| def test_dangerous_magic_bytes_php(self): | ||
| ok, msg = _validate_payload_file('test.txt', b'<?php echo "hi";') | ||
| self.assertFalse(ok) | ||
| self.assertIn('Dangerous', msg) |
The assertion was case-sensitive and coupled to exact capitalization of the error message. Use msg.lower() to make the test stable against formatting changes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens payload upload validation by checking both the sanitized filename extension and the uploaded content’s initial bytes (“magic bytes”) to reject disguised script payloads, and adds unit tests for the new behavior.
Changes:
- Added
_validate_payload_file()with an allowlist of extensions and a denylist of dangerous content signatures. - Integrated validation into
post_payloads()using the sanitized filename and the first 16 bytes of the uploaded file. - Added unit tests covering extension allow/deny, magic-byte detection, and null-byte filenames.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/api/v2/handlers/payload_api.py | Adds extension allowlist + magic-byte signature checks and enforces them during upload. |
| tests/security/test_payload_validation.py | Adds tests to verify extension and magic-byte validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
+38
| for magic in DANGEROUS_MAGIC_BYTES: | ||
| if file_content_start.startswith(magic): | ||
| return False, f'Dangerous file signature detected' |
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
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.
Payload uploads validated by filename only. Added magic-byte verification to catch disguised file types. With tests.