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

(#678 , #680)feat: add ability for asciidocParser to pass the krokiServerUrl #701

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

haydencbarnes
Copy link
Contributor

@haydencbarnes haydencbarnes commented Feb 10, 2023

References - #678, #680 - Add VSCode rule to allow kroki-server-url by default for local server content

Note: This is the first part of #680 - Adding the ability for asciidocParser to pass the krokiServerUrl to the asciidoctorWebViewConverter. This is done by loading the Asciidoctor document using the header_only before we instantiate the AsciidoctorWebViewConverter in asciidocParser.ts. Then since krokiServerUrl should be defined at the parser level, and the value passed to the AsciidoctorWebViewConverter, I have added a new constructor argument named krokiServerUrl in asciidoctorwebviewconverter.

The next step will be to compute the CSP correctly. When enableKroki is true, we should always allow the krokiServerUrl no matter which security level is selected. -> Not sure 100% how I am going to make this work as CSP rules only get more restrictive.

Questions I have on this commit because I am very inexperienced with CSP rules and the complexity of the codebase here:

  • It was originally stated that we might need to "add enableKroki and krokiServerUrl to the AsciidocPreviewConfiguration, then pass the configuration to the getCspForResource method" in #678 - Add VSCode rule to allow kroki-server-url by default for local server content #680. But it was later stated that we should not need to change the security level when enableKroki is true we should allow the krokiServerUrl no matter which security level is selected. This commit defines const krokiServerUrl in the asciidocParser and adds a new krokiServerUrl constructor to AsciidoctorWebViewConverter, but the cspRule will be looking for krokiServerUrl before the AsciidoctorWebViewConverter class is defined. So my question is, is it necessary to define krokiServerUrl before the AsciidoctorWebViewConverter class?

  • For an example, settings.json section for the AsciiDoc extension looks like:

"asciidoc.extensions.enableKroki": true,
"asciidoc.preview.asciidoctorAttributes": {
  "kroki-server-url": "http://localhost:8000"
}

If now, the Asciidoctor attribute kroki-server-url is passed to the asciidocParser and AsciidoctorWebViewConverter, do we still also want/need to pass the "enableKroki" value from the vscode config vscode.workspace.getConfiguration('asciidoc.extensions') to the AsciidoctorWebViewConverter to compute the else, if for the cspRule?

@ggrossetie

…er to pass the krokiServerUrl to asciidoctorWebViewConverter
@ggrossetie
Copy link
Member

vscode.workspace.getConfiguration('asciidoc.extensions', null).get('enableKroki') is available everywhere (global object) so we don't need to pass the configuration value to AsciidoctorWebViewConverter we can get the configuration from AsciidoctorWebViewConverter (if needed).

If kroki-server-url is defined but Kroki is disabled enableKroki then we should not allow kroki-server-url (or the default value https://kroki.io) in the CSP.

Copy link
Contributor Author

@haydencbarnes haydencbarnes left a comment

Choose a reason for hiding this comment

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

Missed a comma @ggrossetie

@ggrossetie ggrossetie merged commit fba9fb7 into asciidoctor:master Feb 11, 2023
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