plantuml / plantuml-server

PlantUML Online Server
https://plantuml.com/
GNU General Public License v3.0
1.65k stars 472 forks source link

%getenv() and %dirpath return empty #232

Closed scmdavid closed 1 year ago

scmdavid commented 2 years ago

Version: 1.2022.5

Test the follow with the server directly

@startuml
!log %version()
!log %getenv("OS")
!log %dirpath()
@enduml

From server stdout

[Log] 1.2022.5
[Log]
[Log]

I would like use the %getenv() to construct the !include, but it returned empty (same for OS, PATH or JAVA_HOME)

The-Lum commented 2 years ago

Hello @scmdavid, and all,

See perhaps this Security Page (that defines security profile and affects the usage of the builtin functions):

If that can help, Regards.

The-Lum commented 2 years ago

Then see also, on the forum, those pages:

scmdavid commented 2 years ago

Hi @The-Lum

Thanks for your info

I set the PLANTUML_SECURITY_PROFILE to UNSECURE from the environment and started the server locally with Maven's Jetty plugin

>echo %PLANTUML_SECURITY_PROFILE%
UNSECURE

>mvn jetty:run "-Djetty.http.port=9999"
[INFO] Scanning for projects...
[INFO]

However, I get the same result

@startuml
!log %version()
!log %dirpath()
!log %getenv("JAVA_HOME")
@enduml
[Log] 1.2022.5
[Log]
[Log]
HeinrichAD commented 1 year ago

The reason for %getenv(...) does not work as expected lies here: plantuml code (no PlantUML Server code).

ALLOW_INCLUDE (which is the ALLOW_PLANTUML_INCLUDE environment variable) instead of PLANTUML_SECURITY_PROFILE is checked. If you start plantuml as jar file manually, ALLOW_INCLUDE seems to be set to true by default (see here). As a security feature it is deactivated by default inside the PlantUML server.

Therefore this is working:

java -jar target/plantuml.jar -tpng diagram.txt

while PlantUML server is not. At least as long as ALLOW_PLANTUML_INCLUDE=true is not set as environment variable. Hence, the work around would be to set the ALLOW_PLANTUML_INCLUDE environment variable to true.

diagram.txt ```plantuml @startuml !log %version() !log %dirpath() !log %getenv("USER") @enduml ```

%dirpath() is not even working via java -jar target/plantuml.jar -DPLANTUML_SECURITY_PROFILE=UNSECURE -tpng diagram.txt. Only if you set the PLANTUML_SECURITY_PROFILE as OS environment variable it works: PLANTUML_SECURITY_PROFILE=UNSECURE java -jar target/plantuml.jar -tpng diagram.txt. For my this seems to be a bug, therefore I created an issue in the plantuml project itself. See: https://github.com/plantuml/plantuml/issues/1450 If this part is clear, a further investigation could be started.

In general it might be a good idea to set the PLANTUML_SECURITY_PROFILE variable to INTERNET by default for the PlantUML Server. People could always change it back to LEGACY if necessary or add special paths to the allowlist. More information can be found here: https://plantuml.com/security

Also issues #172 is also related to this.

@arnaudroques I don't know if this is intended but at least for this scenario / use case it is bewildering. I do not know if this function is also used for other stuff, but since it should mainly be there to get the environment value via getenv I thing this function should check the PLANTUML_SECURITY_PROFILE instead of ALLOW_INCLUDE, should it? Something like if (SecurityUtils.getSecurityProfile() == SecurityProfile.UNSECURE) maybe? Or does this also have something to do with the !include mechanism?

Edit: Only checking SecurityProfile.UNSECURE does not make much sense. It may be a better idea to check SecurityProfile.UNSECURE or if the environment variable starts with PLANTUML.

proposal ```diff public TValue executeReturnFunction(TContext context, TMemory memory, LineLocation location, List values, Map named) throws EaterException, EaterExceptionLocated { + final String name = values.get(0).toString(); // ::comment when __CORE__ - if (OptionFlags.ALLOW_INCLUDE == false) + if (SecurityUtils.getSecurityProfile() != SecurityProfile.UNSECURE && !name.toUpperCase().startsWith("PLANTUML")) { // ::done return TValue.fromString(""); } // ::comment when __CORE__ - final String name = values.get(0).toString(); final String value = getenv(name); if (value == null) return TValue.fromString(""); return TValue.fromString(value); // ::done } ```
arnaudroques commented 1 year ago

I thing this function should check the PLANTUML_SECURITY_PROFILE instead of ALLOW_INCLUDE, should it?

Yes, you are right.

The ALLOW_INCLUDE is an old stuff that was introduced a long time ago to specifically prevent web users to get information from a running PlantUML web server.

We should now use instead the more recent PLANTUML_SECURITY_PROFILE. We certainly will. We are also going to follow your suggestion to check if the environment variable starts with PLANTUML.

arnaudroques commented 1 year ago

The ALLOW_INCLUDE has been removed in last beta of the core library. Maybe you can have a look at the code, if that's fine for you. Next steps could be:

Any though?

HeinrichAD commented 1 year ago

I took a quick look into your code changes and had one question to a specific part. See comment https://github.com/plantuml/plantuml/commit/fbe7fa3b25b4c887d83927cffb1009ec6cb8ab1e#r117852424.

modify plantuml-server, remove use of ALLOW_INCLUDE and default PLANTUML_SECURITY_PROFILE to INTERNET.

Done (inside PR #301). But this PR required PlantUML core 1.2023.9.

So, if you decide to to publish PlantUML core version 1.2023.9 in the future, the next step would be to run the tests of PR #301 again. If these result in no problems, it should be ready to merge.

HeinrichAD commented 1 year ago

This should now be done with version 1.2023.9

Therefore, this issue can now be closed.