🛡️ Sentinel: [HIGH] Fix file upload validation bypass #48
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,9 @@ export function sanitizeLog(data: any): any { | |
| // Check if key matches any sensitive pattern | ||
| if (SENSITIVE_PATTERNS.some((pattern) => pattern.test(key))) { | ||
| sanitized[key] = "***REDACTED***"; | ||
| } else if (typeof sanitized[key] === "string") { | ||
| // Prevent log injection by removing newlines from strings | ||
| sanitized[key] = sanitized[key].replace(/[\r\n]/g, ''); | ||
| } else if (typeof sanitized[key] === "object" && sanitized[key] !== null) { | ||
| // Recursively sanitize objects | ||
| sanitized[key] = sanitizeLog(sanitized[key]); | ||
|
|
@@ -40,3 +43,59 @@ export function sanitizeLog(data: any): any { | |
|
|
||
| return sanitized; | ||
| } | ||
|
|
||
| /** | ||
| * Validates audio file signatures to prevent malicious uploads. | ||
| * Checks for MP3, WAV, OGG, FLAC, and AAC/M4A magic bytes. | ||
|
Comment on lines
47
to
49
There was a problem hiding this comment. The comment uses "Sentinel 🛡️:" as a branded prefix, which violates the coding guidelines. According to the project guidelines, comments should not use branded prefixes. Remove the "Sentinel 🛡️:" prefix and use a simple descriptive comment instead, such as "Validates audio file signatures to prevent malicious uploads."
Owner
Author
There was a problem hiding this comment. @copilot open a new pull request to apply changes based on this feedback |
||
| */ | ||
| export function verifyAudioFileSignature(buffer: Buffer): boolean { | ||
| if (!buffer || buffer.length < 12) return false; | ||
|
|
||
| const header = buffer.subarray(0, 12); | ||
|
|
||
| // Magic bytes constants | ||
| const SIGNATURES = { | ||
| ID3: Buffer.from([0x49, 0x44, 0x33]), | ||
| RIFF: Buffer.from([0x52, 0x49, 0x46, 0x46]), | ||
| WAVE: Buffer.from([0x57, 0x41, 0x56, 0x45]), | ||
| OGG: Buffer.from([0x4F, 0x67, 0x67, 0x53]), | ||
| FLAC: Buffer.from([0x66, 0x4C, 0x61, 0x43]), | ||
| FTYP: Buffer.from([0x66, 0x74, 0x79, 0x70]), | ||
| }; | ||
|
|
||
| // MP3: ID3v2 tag or Sync Frame. | ||
| // Sync frame: FF Fx (MPEG-1 Layer III: FF FB/FA, MPEG-2 Layer III: FF F3/F2, MPEG-2.5 Layer III: FF E3/E2) | ||
| // We explicitly check for Layer III to avoid false positives like UTF-16 LE BOM (FF FE) which looks like MPEG-1 Layer I. | ||
| const isMp3 = | ||
| header.subarray(0, 3).equals(SIGNATURES.ID3) || | ||
| (header[0] === 0xff && ( | ||
| header[1] === 0xfb || header[1] === 0xfa || // MPEG-1 Layer III | ||
| header[1] === 0xf3 || header[1] === 0xf2 || // MPEG-2 Layer III | ||
| header[1] === 0xe3 || header[1] === 0xe2 // MPEG-2.5 Layer III | ||
| )); | ||
|
|
||
| // WAV: RIFF header with WAVE format | ||
| const isWav = | ||
| header.subarray(0, 4).equals(SIGNATURES.RIFF) && | ||
| header.subarray(8, 12).equals(SIGNATURES.WAVE); | ||
|
|
||
| // OGG: OggS capture pattern | ||
| const isOgg = header.subarray(0, 4).equals(SIGNATURES.OGG); | ||
|
|
||
| // FLAC: fLaC marker | ||
| const isFlac = header.subarray(0, 4).equals(SIGNATURES.FLAC); | ||
|
|
||
| // M4A/MP4: ftyp box at offset 4 AND valid audio brand | ||
| // Brands: M4A, M4B, M4P | ||
| const isM4a = | ||
| header.subarray(4, 8).equals(SIGNATURES.FTYP) && | ||
| (header.subarray(8, 12).equals(Buffer.from("M4A ")) || | ||
| header.subarray(8, 12).equals(Buffer.from("M4B ")) || | ||
| header.subarray(8, 12).equals(Buffer.from("M4P "))); | ||
|
|
||
| // AAC ADTS: Sync word (12 bits of 1s) | ||
| // header[0] == 0xFF, header[1] & 0xF6 == 0xF0 (MPEG-4: F1, MPEG-2: F9, valid protection/layer bits) | ||
| const isAacAdts = header[0] === 0xff && (header[1] & 0xf6) === 0xf0; | ||
|
|
||
| return isMp3 || isWav || isOgg || isFlac || isM4a || isAacAdts; | ||
| } | ||
|
Comment on lines
51
to
101
Contributor
There was a problem hiding this comment. The function To improve clarity and maintainability, I suggest refactoring this function to use named constants for the magic byte sequences and export function verifyAudioFileSignature(buffer: Buffer): boolean {
if (!buffer || buffer.length < 12) return false;
const header = buffer.subarray(0, 12);
const SIGNATURES = {
ID3: Buffer.from([0x49, 0x44, 0x33]),
RIFF: Buffer.from([0x52, 0x49, 0x46, 0x46]),
WAVE: Buffer.from([0x57, 0x41, 0x56, 0x45]),
OGG: Buffer.from([0x4F, 0x67, 0x67, 0x53]),
FLAC: Buffer.from([0x66, 0x4C, 0x61, 0x43]),
FTYP: Buffer.from([0x66, 0x74, 0x79, 0x70]),
};
// MP3: ID3v2 tag or Sync Frame. A more general check for MPEG frames is used.
const isMp3 =
header.subarray(0, 3).equals(SIGNATURES.ID3) ||
(header[0] === 0xff && (header[1] & 0xe0) === 0xe0);
// WAV: RIFF header with WAVE format
const isWav =
header.subarray(0, 4).equals(SIGNATURES.RIFF) &&
header.subarray(8, 12).equals(SIGNATURES.WAVE);
// OGG: OggS capture pattern
const isOgg = header.subarray(0, 4).equals(SIGNATURES.OGG);
// FLAC: fLaC marker
const isFlac = header.subarray(0, 4).equals(SIGNATURES.FLAC);
// M4A/MP4: ftyp box at offset 4
const isM4a = header.subarray(4, 8).equals(SIGNATURES.FTYP);
// AAC ADTS: Sync word (12 bits of 1s)
const isAacAdts = header[0] === 0xff && (header[1] & 0xf0) === 0xf0;
return isMp3 || isWav || isOgg || isFlac || isM4a || isAacAdts;
} |
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test case for rejecting generic MP4 video files (e.g., files with 'ftyp' followed by 'isom' brand). This is critical for verifying the M4A validation doesn't allow MP4 videos to pass. Add a test with Buffer containing [0x00, 0x00, 0x00, 0x20, 0x66, 0x74, 0x79, 0x70, 0x69, 0x73, 0x6F, 0x6D] (ftyp + isom brand) and expect it to return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the tests to include UTF-16 LE BOM and generic MP4 video cases as requested.