Repository navigation
Conversation
carson-codeigniter4
Bot
added
the
bug
| $this->assertSame('http://example.com/test.jpg', strip_image_tags('<img alt=\'test "quoted" photo\' src=\'http://example.com/test.jpg\'>')); | ||
|
|
||
| // Empty src or tags without src should be stripped | ||
| $this->assertSame('', strip_image_tags('<img src="" onerror="alert(1)">')); |
There was a problem hiding this comment.
A single-pass preg_replace introduces an XSS bypass when the tag is a nested one, like: <im<img>g src=x onerror=alert(1)>. Please do the replacement inside a do-while loop until it's clean.
| [ | ||
| '#<img[\s/]+.*?src\s*=\s*(["\'])([^\\1]+?)\\1.*?\>#i', | ||
| '#<img[\s/]+.*?src\s*=\s*?(([^\s"\'=<>`]+)).*?\>#i', | ||
| '#<img[\s/]+[^>]*?src\s*=\s*(?|(["\'])(.*?)\1|()([^\s"\'=<>`]+))[^>]*>#is', |
There was a problem hiding this comment.
does this need to look exclusively for src? if yes, this regex will match data-src
| function strip_image_tags(string $str): string | ||
| { | ||
| return preg_replace( | ||
| return (string) preg_replace( |
There was a problem hiding this comment.
don't use the string cast. instead use ?? $str
e938e43 to
2965a8d
Compare
| '#<img[\s/]+(?:[^>]*?[\s/])?src\s*=\s*(?|(["\'])(.*?)\1|()([^\s"\'=<>`]+))[^>]*>#is', | ||
| '#<img(?:\s[^>]*|/[^>]*|)>#is', |
There was a problem hiding this comment.
This treats > inside a quoted attribute as the end of the tag. For example, <img alt="><svg onload=alert(1)>" src="photo.jpg"> now exposes an active SVG element, while the previous implementation returned photo.jpg. Please handle quoted attribute boundaries correctly.
|
|
||
| do { | ||
| $original = $str; | ||
| $str = preg_replace($patterns, $replacements, $str) ?? $str; |
There was a problem hiding this comment.
On PCRE failure, ?? $str returns the original, unfiltered HTML. With default limits, '<img ' . str_repeat(' ', 1000) . 'alt=x><img src=x onerror=alert(1)>' already triggers this and preserves the event handler. Please handle regex errors without returning unfiltered input.
| do { | ||
| $original = $str; | ||
| $str = preg_replace($patterns, $replacements, $str) ?? $str; | ||
| } while ($str !== $original); |
There was a problem hiding this comment.
This loop removes only one level of nesting per iteration, requiring a full scan of the string each time. That seems like a significant performance hit.
2965a8d to
2c887b5
Compare
Description
strip_image_tags()used an invalid PCRE character class ([^\1]) that allowed HTML attribute injection upon mixed quotes, while also swallowing intervening text between multiple tags and failing to strip image tags withoutsrc. This change resolves these issues using a branch-reset pattern and ensures tags withoutsrcare stripped.Checklist: