Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kroki does not render included PlantUML sprites #33

Closed
vainikkaj opened this issue Nov 11, 2019 · 10 comments
Closed

Kroki does not render included PlantUML sprites #33

vainikkaj opened this issue Nov 11, 2019 · 10 comments

Comments

@vainikkaj
Copy link

Given following image is given to Kroki

@startuml
!pragma revision 1

'!define AzurePuml https://raw.githubusercontent.com/RicardoNiepel/Azure-PlantUML/master/dist
'!includeurl AzurePuml/AzureRaw.puml
'!includeurl AzurePuml/Databases/AzureCosmosDb.puml
'!includeurl AzurePuml/Compute/AzureFunction.puml

!include <azure/AzureRaw>
!include <azure/Databases/AzureCosmosDb>
!include <azure/Compute/AzureFunction>

component "<color:red><$AzureFunction></color>" as myFunction
database "<color:#0072C6><$AzureCosmosDb></color>" as myCosmosDb
rectangle "<color:AZURE_SYMBOL_COLOR><$AzureCosmosDb></color>" as mySecondCosmosDb
AzureFunction(mySecondFunction, "Stream Processing", "Consumption")

myFunction --> myCosmosDb
mySecondFunction --> mySecondCosmosDb
@enduml

Then rendering ends with following error message Error 400: Syntax Error? (line: 12)

Image will be rendered when AzureFunction line removed but even then all sprites are missing.

Selection_031

Below is a link how the image should look in Kroki

http://www.plantuml.com/plantuml/uml/XL5FRzem5B_xKvXiqZPf4BR39g4eWeNr10K5SMWloDYlmL9iH_Ppg_RJrv0QIbhQet__hJTso2nwLPDUOrcb6BLmBvqqcluYv5jFm9tKG2TFti8ooEWEiN6ZDBNiOL19tFcjTs2vqGWQ1zoej9ISMM6k9JHGfmTdVrapZUlvB5NC8TXKI8SXNMfUUm7UrgU6rb66-2QyBecc3DcMEN2jl31E6JVTVkWfZ6eyGkkuyffZ-6Rh8D51CxPdZoFoBdEf-Otm_RgS41vmeq4ZJJ9kQcD75aIUVJtNPUc1onFA75MF4IVYfVve_Z8S_lrT_8a1SKyt8EB40aUcg_gKCBbTb_ytgvlvlyLiKoncY_Api1M4Wul8ahFbtoCTWPyqMQ45fkZI6WxEILqb0IoCTburUqdoWv3J7sc_dxzU70CxV6V46BG8ftm6

@ggrossetie
Copy link
Member

Hello @vainikkaj

I've decided to prune 'include and 'includeurl directives for security reasons.
I'm not sure how http://www.plantuml.com prevents abuse but allowing the server to make an HTTP call to an untrusted server seems really dangerous to me.

However I'm willing to include collections of sprites and macros for well-known services like AWS or Azure and/or add a whitelist of URLs.
What do you think?

@ggrossetie
Copy link
Member

Apparently Azure is already available in the stdlib: http://plantuml.com/fr/stdlib
I will try to allow the !include directive only on stdlib.

@ggrossetie
Copy link
Member

!includeurl does not seem to be enabled in http://plantuml.com

The following does not work on http://plantuml.com:

@startuml
!pragma revision 1

'!define AzurePuml https://raw.githubusercontent.com/RicardoNiepel/Azure-PlantUML/master/dist
'!includeurl AzurePuml/AzureRaw.puml
'!includeurl AzurePuml/Databases/AzureCosmosDb.puml
'!includeurl AzurePuml/Compute/AzureFunction.puml

component "<color:red><$AzureFunction></color>" as myFunction
database "<color:#0072C6><$AzureCosmosDb></color>" as myCosmosDb
rectangle "<color:AZURE_SYMBOL_COLOR><$AzureCosmosDb></color>" as mySecondCosmosDb
AzureFunction(mySecondFunction, "Stream Processing", "Consumption")

myFunction --> myCosmosDb
mySecondFunction --> mySecondCosmosDb
@enduml

But the following do (because Azure is available in the stdlib):

@startuml
!pragma revision 1

!include <azure/AzureRaw>
!include <azure/Databases/AzureCosmosDb>
!include <azure/Compute/AzureFunction>

component "<color:red><$AzureFunction></color>" as myFunction
database "<color:#0072C6><$AzureCosmosDb></color>" as myCosmosDb
rectangle "<color:AZURE_SYMBOL_COLOR><$AzureCosmosDb></color>" as mySecondCosmosDb
AzureFunction(mySecondFunction, "Stream Processing", "Consumption")

myFunction --> myCosmosDb
mySecondFunction --> mySecondCosmosDb
@enduml

@vainikkaj
Copy link
Author

Hi @Mogztter
Thanks for the speedy reply.

I think it's security-wise ok to trust that the content of embedded stdlib is safe and enable include on it.

I agree that includeurl is problematic and if people really need to use that then they can open new issue for it.

ggrossetie added a commit that referenced this issue Nov 13, 2019
@ggrossetie
Copy link
Member

It's fixed and available at https://kroki.io

test

@ggrossetie
Copy link
Member

@vainikkaj Does it work for you?

@vainikkaj
Copy link
Author

👍
Thanks for the quick fix.

@ggrossetie
Copy link
Member

Thanks for your feedback, I'm glad it's working for you 😉

@AXGKl
Copy link

AXGKl commented Jan 10, 2020

@Mogztter Hi (again), this time just a comment :-)

Your security argument against !includeurl is absolutely clear.
For !include (file) I dare to beg you for reconsideration, at least for self hosted situations (maybe check an env var or so).
Background: I personally tried C4 and assumed kroki supports includes...
Should have tried others before but no, spent quite a while on a set of themes in our CI, which users of my self hosted server could simply include - but nope :-)
Reading the java source I understand you make a hardcoded Exception for C4 but that is not very obvious for new users like myself.

=> Maybe allow an env var like $PLANTHOME and if set, try find given filenames (without dirs) on it? Can't see a security problem here.
Because include definitely is a very powerful mechanic, i.e. we have a pretty tough restriction of kroki, compared to original plantuml.

PS: Your productivity is... depressing ;-)

@ggrossetie
Copy link
Member

For !include (file) I dare to beg you for reconsideration, at least for self hosted situations (maybe check an env var or so).

Indeed we could enable this feature for self hosted server.
But I need to check how the include directive works in PlantUML. More specifically, I don't know if we can specify the location of includes.

@AXGKl Could you please open another issue with this feature request? Thanks!

PS: Your productivity is... depressing ;-)

😉

Your security argument against !includeurl is absolutely clear.

We could also enable this feature on self hosted server. Not sure if it should be enabled by default or not.

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

No branches or pull requests

3 participants