I created this patch in advance of writing the signinview plugin. This patch does nothing signinview-specific, but simply refactors wrapper generation a bit so that plugins have some influence over what environment variables a wrapper will preserve.

For example, Wrapper.pm previously hardcoded not only (some of) the RFC 3875 variables needed for a CGI wrapper (and hardcoded its own test for whether it was generating a CGI wrapper), but also the Apache ErrorDocument-specific variables needed by the 404 plugin. Given that signinview, as a 403 handler, would have similar requirements to 404, and it seemed possible other wrappers for other purposes could rely on other environment variables too, it seemed to make sense to move the preserved-variable list out of Wrapper.pm hardcoding, and closer to the plugins or other code relying on the variables.

I was asked by Joey to create a page here for purposes of review. I'm still trying to figure out the preferred workflow for this project ... I'm assuming the github link is ok for looking over the comments and code changes, since they're already pushed to my git fork and (to me, anyway) reviewing changes in a decent repository browser is so much nicer than a diff -u pasted into a page between <pre> tags.

That seemed to be ok for reviewing CGI wrapper doesn't store PERL5LIB environment variable, so I hope it's ok for this one too. If another way would be preferable, please let me know.

-- ?jcflack

This is less about what plugins need, and more about what is safe. If an environment variable is unsafe (in the sense of "can make a setuid executable change its behaviour in dangerous ways") then we must not pass it through, however desirable it might be.

Because the only safe thing we can do is a whitelist, the list is secondarily about what plugins need: if nothing needs a variable, we don't pass it through.

However, if a particular variable is safe, then it's always safe; so if any plugin needs something, we might as well just put it in the big list of things to keep. (In other words, any change to this list is already security-sensitive.)

As such, and because importing CGI into Setup pulls in a bunch of extra code that is normally only imported when we are actually running as a CGI, it might make more sense to have the "master list" stay in Wrapper.

What variables would signinview need? Can we just add them to the list and skip the complexity of per-plugin configurability?

Sorting the list makes sense to me, and so does adding the RFC 3875 set.

This change does seem to have exposed a thing where various plugins that
call checksessionexpiry() (in CGI.pm) have been supplying one more argument
than its prototype allows ... for years ...

I fixed that in ikiwiki 3.20141016. Please don't add the extra ignored parameter to the prototype.

+ if ( $config{needenvkeys} ) {

If this is needed at all, you should include this in the master list of setup keys in IkiWiki.pm so it's documentable. Please mention setuid in the description: "environment variables that are safe to pass through a setuid wrapper" or something.

I think it's safe => 0, advanced => 1.

preserve_env or env_keep (or without the underscore, as you prefer) might be better names for it (terminology stolen from debuild and sudo respectively).

--smcv