Skip to content

Fix false positive in proxy/fastcgi param tests#183

Open
Josue-T wants to merge 2 commits intomainfrom
proxy_param_fix
Open

Fix false positive in proxy/fastcgi param tests#183
Josue-T wants to merge 2 commits intomainfrom
proxy_param_fix

Conversation

@Josue-T
Copy link
Contributor

@Josue-T Josue-T commented Jan 28, 2026

Problem

  • False positive. For instance you have 3 nginx config file and in one you have the proxy_params_auth configuration. So we have sso = true in the manifest, we should check that at last one nginx config have proxy_params_auth, so not for all nginx config file.
  • We should be more tolerant when there are custom fastcgi_param and proxy_set_header. In some case it could be justified to override the default value. For now I created a blacklist of the param that there are no reason to override and a list of the param that it could make sens to override. Also now we check if the value is the default one, so if it's the default one there no reason to override and we rise a warning.

PR checklist

  • PR finished and ready to be reviewed

@Josue-T Josue-T requested a review from alexAubin January 28, 2026 17:40
False positive. For instance you have 3 nginx config file and in one you have the proxy_params_auth configuration. So we have sso = true in the manifest, we should check that at last one nginx config have proxy_params_auth, so not for all nginx config file.
We should be more tolerant when there are custom fastcgi_param and proxy_set_header. In some case it could be justified to override the default value. For now I created a blacklist of the param that there are no reason to override and a list of the param that it could make sens to override. Also now we check if the value is the default one, so if it's the default one there no reason to override and we rise a warning.
Copy link
Contributor

@Salamandar Salamandar left a comment

Choose a reason for hiding this comment

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

LGTM although reusing the variable name reverse_proxy_params_from_includes_that_are_manually_set sounds a bit fishy.

Waiting for a second opinion on the list of variables / values but it should be OK.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants