Add Sanitize API #349
Add Sanitize API #349
Conversation
|
|
proto/sanitizer.proto
Outdated
| bytes domPurifyConfig = 3; | ||
| bool allowAllLinksInSvgUseTags = 4; |
There was a problem hiding this comment.
maybe we have this proto definition be more "open" to non svg thigns in the future?
Perhaps
| bytes domPurifyConfig = 3; | |
| bool allowAllLinksInSvgUseTags = 4; | |
| string sanitizer = 3; // 'svg' | ... | |
| bytes sanitizerConfig = 4; // json blob that depends on the sanitier type |
There was a problem hiding this comment.
yeah - thats better. For string sanitizer I would probably use the library name or "sanitization logic type" instead of the file type so we avoid having an abstraction over the library config. DOMPurify has a pretty extensive API and it can support html, svg or mathml out of the box. Soo, something like this:
sanitizer: 'dompurify'
config: { dompurifyConfig: { ... }, domPurifyExtraFunctionalitiesBuiltByUsConfig: { ... } }
and
sanitizer: 'some-other-image-sanitizer'
config: { some-other-image-sanitizerConfig': { ... }}
what do you think?
|
|
||
| message SanitizeResponse { | ||
| string error = 1; | ||
| bytes sanitized = 2; |
There was a problem hiding this comment.
is there relevant info we may also return? I guess we can add that later
There was a problem hiding this comment.
not sure, I could add a generic string info; field and for now populate it with "sanitized" / "not sanitized" depending on whether the hash changed. We could use that field in the future if we choose to migrate to some other library that returns some info on itself.
There was a problem hiding this comment.
it would be difficult to differentiate between sanitized vs unsanitized files even right now, as dompurify modifies all svgs. I think we can do it later if we'd like
src/types.ts
Outdated
| export type SanitizeRequest = { | ||
| filename: string; | ||
| content: string; | ||
| contentType?: string; | ||
| domPurifyConfig?: DOMPurify.Config; | ||
| allowAllLinksInSvgUseTags?: boolean; | ||
| }; |
There was a problem hiding this comment.
Seems like this is never used, is there a reason to not remove it and remove the V2 of SanitizeRequestV2?
src/service/http-server.ts
Outdated
| @@ -49,6 +61,14 @@ export class HttpServer { | |||
|
|
|||
| this.app.get('/render', asyncMiddleware(this.render)); | |||
| this.app.get('/render/csv', asyncMiddleware(this.renderCSV)); | |||
| this.app.post( | |||
| '/render/sanitize', | |||
There was a problem hiding this comment.
| '/render/sanitize', | |
| '/sanitize', |
This would be better if we could get rid of the render prefix and do something in Grafana to replace /render in the renderer URL with /sanitize for example.
Else, it will mess up the metrics we collect about image renderer (set up here: https://github.com/grafana/grafana-image-renderer/blob/master/src/service/metrics_middleware.ts#L49). It could be interesting to have metrics for this endpoint as well but we should keep them separate.
src/types.ts
Outdated
| @@ -1,3 +1,5 @@ | |||
| import * as DOMPurify from 'dompurify'; | |||
There was a problem hiding this comment.
What about putting all these new types in a types.ts file in sanitizer folder? I think it would help keep things separated.
There was a problem hiding this comment.
done, extracted all of them to sanitizer/types.ts
…derer into #50597-sanitizer
|
hey... wondering what is the use of this feature? would you mind brief explain this @ArturWierzbicki |
|
hey @Bujupah! The use of this feature is server side sanitization of SVGs hosted in Grafana. There is some more context here if you are interested: grafana/grafana#50597. |
grafana/grafana#50936 for context
See https://github.com/grafana/grafana/pull/50936/files#diff-7d6b47ef00ca0a64668dd9913f6b4279b06c3d2a0d975c70daad20691abab72a for an exemplary HTTP request
The HTTP request requires two files:
fileconfig, example{ "config": { "USE_PROFILES": { "svg": true, "svgFilters": true }, "ADD_TAGS": [ "use" ], "allowAllLinksInSvgUseTags": false }, "configType": "DOMPurify" }