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

[haydencbarnes]

#678 - reference

[ggrossetie]

As a side note, we might want to use https://www.npmjs.com/package/content-security-policy-builder (or similar) to build CSP and avoid duplications/mistakes/typos.

If we don't want to use a library we could at least join to make it easier to read:

const rules = [
  "default-src 'none'",
  `img-src 'self' ${rule} https: data:`,
  `media-src 'self' ${rule} https: data:`,
  `script-src 'nonce-${nonce}' '${highlightjsInlineScriptHash}'`,
  `style-src 'self' ${rule} 'unsafe-inline' https: data:`,
  `font-src 'self' ${rule} https: data:`
]
rules.join('; ')

Don't make this change in this pull request, that's just an idea to make it easier to maintain in the future.

[ggrossetie]

are you sure it's working if we add the same rule twice? for instance, if the security is strict we will have img-src twice:

img-src 'self' ${rule} https: data:; img-src 'self' ${kroki-server-url};

[haydencbarnes]

Will check tomorrow

[haydencbarnes]

https://chrisguitarguy.com/2019/07/05/working-with-multiple-content-security-policy-headers/ - says that CSP headers can only get more restrictive, @Mogztter is this true in your experience?

• this article also shows that -https://w3c.github.io/webappsec-csp/#multiple-policies

[haydencbarnes]

If this is the case, I think I would need to write an if / else for the csp rules based on if the user has 'kroki-server-url' set not just enablekroki (because kroki.io is already allowed by default as you said.)

[ggrossetie]

${kroki-server-url} is undefined, you need to pass the value

[haydencbarnes]

Oof, I need to think about eliminating duplication of the getConfiguration code here too. Right now, I am thinking in the parser file:

// Check if the enableKroki configuration is set to true
  const enableKroki = vscode.workspace.getConfiguration('asciidoc.extensions', null).get('enableKroki');
  if (enableKroki) {
    // Check if the krokiServerUrl property is set
    if (vscode.workspace.getConfiguration('asciidoc.extensions', null).has('krokiServerUrl')) {
      // Get the krokiServerUrl property
      const krokiServerUrl = vscode.workspace.getConfiguration('asciidoc.extensions', null).get('krokiServerUrl');

      // Append the kroki-server-url to the CSP rule
      cspRule += `; img-src 'self' ${krokiServerUrl};`
    } 

And then change the if statement to:

// Check if the krokiServerUrl property is set
if (vscode.workspace.getConfiguration('asciidoc.extensions', null).has('krokiServerUrl')) {
  // Get the krokiServerUrl property
  const krokiServerUrl = vscode.workspace.getConfiguration('asciidoc.extensions', null).get('krokiServerUrl');

  // Check if the enableKroki configuration is set to true
  const enableKroki = vscode.workspace.getConfiguration('asciidoc.extensions', null).get('enableKroki');
  if (enableKroki) {
    // Append the kroki-server-url to the CSP rule
    cspRule += `; img-src 'self' ${krokiServerUrl};`
  }
}

[ggrossetie]

kroki-server-url is an AsciiDoc attribute not a VS Code configuration, that's why you need to use the AsciidocPreviewConfiguration class.
The default value is https://kroki.io if the value is not explicitly defined.

[haydencbarnes]

Okay, so I need to add kroki-server-url as an AsciiDoc attribute to the AsciidocPreviewConfiguration class

export class AsciidocPreviewConfiguration {
  public static getForResource (resource: vscode.Uri) {
    return new AsciidocPreviewConfiguration(resource)
  }

  // ... other properties and methods

  public readonly krokiServerUrl: string
  
  private constructor (resource: vscode.Uri) {
    // ... other property assignments

    const asciidocConfig = vscode.workspace.getConfiguration('asciidoc', resource)
    this.krokiServerUrl = asciidocConfig.get<string>('preview.krokiServerUrl', 'https://kroki.io')
  }
}

Then the value of kroki-server-url should pass to the AsciidocPreviewConfiguration class. Then come back to the cspRules here and add:

// Check if the enableKroki configuration is set to true
const enableKroki = vscode.workspace.getConfiguration('asciidoc.extensions', null).get('enableKroki')
if (enableKroki) {
  // Get the value of the krokiServerUrl property from the AsciidocPreviewConfiguration class
  const config = AsciidocPreviewConfiguration.getForResource(resource)
  const krokiServerUrl = config.krokiServerUrl

  // Append the krokiServerUrl to the CSP rule
  cspRule += `; img-src 'self' ${krokiServerUrl};`
}

[ggrossetie]

Not exactly, we need to load the AsciiDoc document and call getAttribute():

asciidoctor-vscode/src/asciidocParser.ts

Line 203 in 117449b

const document = processor.load(text, options)

You can have a look at: https://github.com/Mogztter/asciidoctor-kroki/blob/059510b500ad9e39733c27ba88b563736fc17ade/src/kroki-client.js#L76

const krokiServerUrl = document.getAttribute('kroki-server-url') || 'https://kroki.io'

Then, we need to pass this value to AsciidoctorWebViewConverter in order to compute the CSP correctly.

[haydencbarnes]

@Mogztter Do I need to load the Asciidoc document into the private constructor method or somewhere else within the AsciidocPreviewConfiguration class?

Or are you saying it needs to be loaded into the asciidoctorWebViewConverter? In place of:

  // Get the value of the krokiServerUrl property from the AsciidocPreviewConfiguration class
  const config = AsciidocPreviewConfiguration.getForResource(resource)
  const krokiServerUrl = config.krokiServerUrl

I am confused about where to implement loading the document (and calling the attribute from Asciidoctor) because I was initially under the impression that the AsciidocPreviewConfirguration class just pulled from the user's vscode workspace?

I can see that kroki-client you sent me now is doing something else on its end besides just using the users vscode workspace.

[ggrossetie]

I think we should load the Asciidoctor document using the header_only before we instantiate the AsciidoctorWebViewConverter class in:

asciidoctor-vscode/src/asciidocParser.ts

Lines 148 to 157 in 1e07c19

	const asciidoctorWebViewConverter = new AsciidoctorWebViewConverter(
	doc,
	editor,
	cspArbiter.getSecurityLevelForResource(doc.uri),
	cspArbiter.shouldDisableSecurityWarnings(),
	this.contributionProvider.contributions,
	previewConfigurationManager.loadAndCacheConfiguration(doc.uri),
	antoraDocumentContext,
	line
	)

const asciidoctorDocument = processor.load(text, { header_only: true })
const krokiServerUrl = document.getAttribute('kroki-server-url') || 'https://kroki.io'

new AsciidoctorWebViewConverter(/* pass the krokiServerUrl here */)

As a side note, the AsciidoctorWebViewConverter constructor has quite a few arguments and we should probably refine it but we can do that later. For now, just add a new constructor argument named krokiServerUrl.

Does it make sense?

[haydencbarnes]

Okay, so to summarize before I make changes to my commit. I will be loading the Asciidoctor document using the header_only before we instantiate the AsciidoctorWebViewConverter in asciidocParser.ts, as shown by:

// load the asciidoc header only to get kroki-server-url attribute
const asciidoctorDocument = processor.load(text, { header_only: true })
const krokiServerUrl = document.getAttribute('kroki-server-url') || 'https://kroki.io'

const asciidoctorWebViewConverter = new AsciidoctorWebViewConverter( 
   doc, 
   editor, 
   cspArbiter.getSecurityLevelForResource(doc.uri), 
   cspArbiter.shouldDisableSecurityWarnings(), 
   this.contributionProvider.contributions, 
   previewConfigurationManager.loadAndCacheConfiguration(doc.uri), 
   antoraDocumentContext, 
   line,
   krokiServerUrl
 ) 

Then krokiServerUrl should be defined at the parser level, and the value passed to the AsciidoctorWebViewConverter .

"For now, just add a new constructor argument named krokiServerUrl" - To do this, I would need to add another argument after the state argument:

state?: any,
private readonly krokiServerUrl?: string

Question: Do I also need to add the krokiServerUrl here as initial data? -

 this.initialData = {
      source: textDocumentUri.toString(),
      line,
      lineCount: textDocument.lineCount,
      scrollPreviewWithEditor: this.config.scrollPreviewWithEditor,
      scrollEditorWithPreview: this.config.scrollEditorWithPreview,
      doubleClickToSwitchToEditor: this.config.doubleClickToSwitchToEditor,
      preservePreviewWhenHidden: this.config.preservePreviewWhenHidden,
      disableSecurityWarnings: cspArbiterShouldDisableSecurityWarnings,
      krokiServerUrl: krokiServerUrl || 'https://kroki.io'
    }

Then, in order 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.

[ggrossetie]

Yes that's correct.

Do I also need to add the krokiServerUrl here as initial data? -

I don't think so.