Re: [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file

[Gabriel Goller <g.goller@proxmox.com>]

On 04.12.2024 11:16, Thomas Lamprecht wrote:
>Am 04.12.24 um 10:16 schrieb Gabriel Goller:
>> On 03.12.2024 18:24, Thomas Lamprecht wrote:
>>> Am 03.12.24 um 16:29 schrieb Gabriel Goller:
>>>> The consent-text paramter is the base64-encoded content of the optional
>>>> consent-banner which can be displayed before login.
>>>
>>> This can get quite big, would be good to set some relatively high limit here,
>>> I think, as our pmxcfs files have a max size and not being able to change other
>>> options due to this property being close to the pmxcfs file size limit would
>>> not be ideal.
>>
>> To keep the textareafield widget generic, I would add the maxLength
>> parameters to pbs and pve separately.
>>
>> About the limit itself, the example banner in the bug report [0] has 500
>> characters and these legal texts can sometimes get quite long – so maybe
>> a limit of 1000 characters?
>
>The one I copied over from some example link [0] comes out as 3733 B in base64,
>but I did not encode the image it contained as base64 data-url, so might be
>bigger if the text should spot some logo, or patriotic flag image like it was
>in my example ^^
>To get a real world example for how much bigger I encoded the image from [0]
>now, and it comes out as ~ 350 KiB of base64, if converted to WebP format first
>it  gets down to ~ 32 KiB.
>So maybe go for bigger limit, say 128 KiB, as that still gives quite a bit of
>headroom to the pmxcfs per-file limit and allows (efficiently encoded) data-URL
>images. 

According to my conservative calculations that would then be a max input
length (in characters) of circa 24000.

> Btw. your Ext.htmlEncode() breaks images with data URLs, our wrapper
>around the Markdown parser we use already sanitizes problematic HTML, so an
>additional HTML encode step might not be warranted here?

Yep, that's right we do the hmtlEncode in the Markdown.parse function
already so this is not needed. Nice catch!

>[0]: https://businessportal.dla.mil/scon/sys-msg/index-ext.html
>
>> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5463
>>
>>> btw. Did we ever talk about placing this into a dedicated, separate file?
>>> (just out of interest, might be also an option to consider here if we did
>>> not talked already about it)
>>
>> Yep, my first version used a separate file – this was discussed here:
>> https://lore.proxmox.com/pbs-devel/fc22ec23-a300-488d-821f-bea1285881a8@proxmox.com/
>
>Hmm, I remember now; with the future-proofing this format makes sense, but
>we could have also split content and how it's acted on, e.g., use a separate
>file for the notes and add the settings for a future enforcement or the like
>to the datacenter.cfg once it happens, I could have thought about that variant
>back then already, sorry. The reason I asked this is that for PDM we use a
>separate file for the generic notes to avoid cluttering config files comments
>with that potential bigger encoded text. But for PBS we already went this way,
>and it's OK enough (with an explicit limit enforced by the API schema) so fine
>by me to keep as is.

Yep, I'll include the limit in pbs as well.

Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel