Adding allowlist content types #2180
Conversation
| @@ -205,6 +217,24 @@ function( | |||
| return $len; | |||
| } | |||
|
|
|||
| // Check if Content-Type is allowed | |||
| if ($name === 'content-type') { | |||
| $mimeType = strtolower(trim(explode(';', $value)[0])); // Normalize and strip charset | |||
There was a problem hiding this comment.
is there any way ; would be there for different reasons than the charset?
|
|
||
| // If not in allowlist, check for special case | ||
| if (!in_array($mimeType, $allowed_content_types, true)) { | ||
| // Special case: allow application/octet-stream for .zip files |
There was a problem hiding this comment.
There's no guarantee that a path ending with .zip would actually serve a zip file. Conversely, a path not ending with zip, such as /github/ci/run/85978429875/artifact/25, may be a zip file. At this point, the most reliable thing to do is to just allow the octet-stream content type.
Also, we'll need the git-related content types to keep the JS Git client working – anything starting with application/x-git- is fine.
There was a problem hiding this comment.
Updated this condition.
| @@ -15,6 +15,18 @@ | |||
| $server_host = $_SERVER['HTTP_HOST'] ?? ''; | |||
| $origin = $_SERVER['HTTP_ORIGIN'] ?? ''; | |||
|
|
|||
| // Define allowlist of Content-Types | |||
| $allowed_content_types = [ | |||
| 'application/json', | |||
There was a problem hiding this comment.
There's also non-standard text/json and text/javascript. Also, there are image content types, font content types, I wonder what else
|
Thank you for this PR! As I reviewed, I started questioning the original premise of restricting the content-type. I've realized there are git-related content-types, there are images, fonts, REST APIs, and who knows what else. For sure maintaining a list of allowed content-types would be a big pain. I wonder what kind of additional security would it actually provide 🤔 Looping in @brandonpayton for thoughts |
Draft PR for #1977
This PR implements a Content-Type allowlist in the CORS proxy logic to help reduce potential abuse vectors, as discussed in issue #1977. It also introduces a flexible exception mechanism for certain file types — for example, allowing application/octet-stream only for .zip files.
I've created this as a Draft PR based on the idea discussed in #1977. The goal is to explore a possible implementation for restricting allowed response Content-Types in the CORS proxy, with conditional logic for cases like allowing .zip files under application/octet-stream.
Please let me know if this approach seems reasonable, or if you'd suggest any changes in direction.
Also, I’d appreciate any guidance on the best way to test this locally within the repo — especially considering how interconnected some of the files are.
Thanks in advance for taking a look whenever you get a chance! 😊
cc @adamziel @brandonpayton @bgrgicak