Skip to content

Add cgi.security_limit_extensions - #24010

Open
DanielEScherzer wants to merge 1 commit into
php:masterfrom
DanielEScherzer:cgi-limit-extensions
Open

DanielEScherzer wants to merge 1 commit into
php:masterfrom
DanielEScherzer:cgi-limit-extensions

Conversation

@DanielEScherzer

Copy link
Copy Markdown
Member

No description provided.

Comment thread sapi/cgi/cgi_main.c
* character after the extension, or the null terminating byte. */
char *after = next + extension_len;
if (
(next == allowed_extensions || (*(next - 1) == ' ') || (*(next - 1) == '\t'))

@devnexen devnexen Sep 30, 2026 •

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.

nit: you can probably simplify all of this (e.g. str*spn)

@Sjord Sjord left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have some more context to this? Does this have an issue, RFC or mailing list thread? What is this supposed to accomplish exactly?

Comment thread sapi/cgi/cgi_main.c
}
/* }}} */

zend_result cgi_limit_extensions(char *path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense if this returned a bool directly? Returning SUCCESS or FAILURE does not make much sense here; the function does not fail, it just determined that it is a filtered extension.

Comment thread sapi/cgi/cgi_main.c
char *after = next + extension_len;
if (
(next == allowed_extensions || (*(next - 1) == ' ') || (*(next - 1) == '\t'))
&& (*after == '\0' || *after == ' ' || *after == '\t')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems the extension list is whitespace separated, but I could also see people using comma's in it. Would it be helpful to handle comma's as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants