Bug 61189 - CGIServlet should be able to set specific environment variables
Summary: CGIServlet should be able to set specific environment variables
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.0.M21
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2017-06-15 16:11 UTC by jm009
Modified: 2017-09-12 20:25 UTC (History)
0 users



Attachments
Allow to configure CGI specific environment variables. (1.53 KB, patch)
2017-09-05 08:01 UTC, jm009
Details | Diff
All variable declarations in one <param-value>. (1.45 KB, patch)
2017-09-05 19:52 UTC, jm009
Details | Diff
Variable name in <param-name>. (1.00 KB, patch)
2017-09-05 20:16 UTC, jm009
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jm009 2017-06-15 16:11:57 UTC
org.apache.catalina.servlets.CGIServlet has a param 'passShellEnvironment'.
It would give more flexibility to allow for different environment variables per CGI.

Examples:

- set HOME and HTTP_HOME to the nextcloud directory
- set SQWEBMAIL_MAXMSGSIZE for sqwebmail


For the sqwebmail example this could look for example like

      <init-param>
         <param-name>EnvironmentVariables</param-name>
         <param-value>SQWEBMAIL_MAXARGSIZE=20000000|SQWEBMAIL_MAXATTSIZE=20000000|SQWEBMAIL_MAXMSGSIZE=20000000</param-value>
      </init-param>

I wrote my own CGI executor servlet for sqwebmail.
And today I need environment variables for Nextcloud.

I don't like to put them in /etc/init.d/tomcat or catalina.sh, because they may be lost after a Tomcat update.
Comment 1 Konstantin Kolinko 2017-06-16 14:53:25 UTC
> I don't like to put them in /etc/init.d/tomcat or catalina.sh, because they
> may be lost after a Tomcat update.

The place for custom environment variables in Apache Tomcat is bin/setenv.sh.
This is documented in RUNNING.txt.

http://tomcat.apache.org/tomcat-8.5-doc/RUNNING.txt
Comment 2 jm009 2017-09-05 08:01:57 UTC
Created attachment 35291 [details]
Allow to configure CGI specific environment variables.

This patch makes configuration of additional environment variables similar to the configuration of executable-arg-1, executable-arg-2, and so on

   <init-param>
      <param-name>environment-variable-1</param-name>
      <param-value>SQWEBMAIL_MAXARGSIZE=20000000</param-value>
   </init-param>
   <init-param>
      <param-name>nvironment-variable-2</param-name>
      <param-value>SQWEBMAIL_MAXATTSIZE=20000000</param-value>
   </init-param>
   <init-param>
      <param-name>nvironment-variable-3</param-name>
      <param-value>SQWEBMAIL_MAXMSGSIZE=20000000</param-value>
   </init-param>
Comment 3 Christopher Schultz 2017-09-05 15:09:43 UTC
I don't like the use of unique names that don't have any meaning, like `environment-variable-N`. Might I suggest one or the other of these schemes:

    <init-param>
      <param-name>environment-variables</param-name>
      <param-value>
        ENV_VAR_1=foo
        ENV_VAR_2=bar
        ...
      </param-value>
    </init-param>

Or:

    <init-param>
        <param-name>environment-variable-ENV_VAR_1</param-name>
        <param-value>foo</param-value>
    </init-param>
    <init-param>
        <param-name>environment-variable-ENV_VAR_2</param-name>
        <param-value>bar</param-value>
    </init-param>
    ...


I think I'd prefer the former over the latter.

Second, I think it makes sense to strip whitespace from around the names and values of the environment variables. Basically, just add ".trim()" to all of your .substring() calls. This will solve silly problems like people placing leading/trailing spaces around names/values/equal signs. XML allows a lot of optional whitespace, and some editors (and humans) like to be verbose.
Comment 4 jm009 2017-09-05 19:52:51 UTC
Created attachment 35296 [details]
All variable declarations in one <param-value>.

So here is a patch, that would implement your first suggestion.
But, while implementing it, I thought of some inconveniences:

- People may think: Oh, I can put whitespace here, so I will do that somewhere else too...

(- It is impossible, to put a line break in the variable value, or to put white space at the beginning or the end of the variable value (its probably not used very often))

I think I'd prefer your second suggestion...
I'll create a patch for that one too.
Comment 5 jm009 2017-09-05 20:16:25 UTC
Created attachment 35297 [details]
Variable name in <param-name>.

This patch implements your second suggestion.
For the moment it is my preferred one.
It also is the patch that adds the least number of lines of code :-)

First and second patch would be fine for me too :-)
Comment 6 Mark Thomas 2017-09-12 20:25:42 UTC
Thanks for the patches. I like the second one too.

Fixed in:
- trunk for 9.0.0.M27 onwards
- 8.5.x for 8.5.21 onwards
- 8.0.x for 8.0.47 onwards
- 7.0.x for 7.0.82 onwards