3rd-party storage has "Unknown" type

[Joshua Huber]

Hi Tomas,

Thanks for the quick reply! No worries at all for jumping the gun and
submitting a patch for the first option. I think that's a totally
reasonable option, and I'm happy to get it in more quickly.

On Fri, Apr 1, 2022 at 11:51 AM Thomas Lamprecht
<t.lamprecht at proxmox.com> wrote:
>
> That's actually a good point, returning unknown here isn't really helping
> anyone, especially as  it's a place where we known that there can be values
> not under Proxmox VE's control, so the value is definitively the better
> fallback.
>
> I pushed out a commit with your proposed 1) behavior as its a good stop-gap
> measurement in any way and doesn't harm any fancier way we may possibly add
> in the future.
>
> https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=ea9b688a8234bbdb8cd9e0f6498bed29777a1e2c

Based on your feedback regarding JS injection, would that be a concern
here, too? I suppose, because of the restrictive set of permitted
characters, the chances of something problematic getting through are
low. On the other hand, it is technically data returned over the API,
and could (via an additional bug or exploit) result in an XSS payload
being shuttled back via the type field.

Surprisingly, I was able to construct a somewhat complex storage type
string and have it passed back to the browser intact. This seems like
a totally reasonable storage type string, doesn't it!? :D

Name             Type     Status           Total            Used
Available        %
ceph              rbd     active        18891843               3
 18891840    0.00%
cephfs         cephfs   inactive               0               0
        0    0.00%
josh       <svg/onmouseover=alert(1)>     active       786432000
199489296       586942704   25.37%

With the just-pass-the-value-back change in place, the onmouseover
actually fires while viewing the StorageView.

> would need to be encoded to avoid HTML-element or JS injection, e.g., via
> the `Ext.htmlEncode()`, besides that it would seem OK from a technical POV.

Thank you! I'm not familiar with ExtJS, specifically, so assumed there
was some other escaping mechanism in place by default.

> not all to hard feelings but for most new properties we use kebab case, i.e.,
> here it'd be 'type-name' (needs the quotes then too, else perl gets confused
> as you probably already know), but due to backward compat we got quite the mix
> present, at least in Proxmox VE (the newer, rust based PBS is quite a bit more
> consistent).

Good to know, and no problem.

> Possibly alternative: 'type-text' (as one could draw parallels to our
> JSONSchema's internal 'format-text, which is used roughly similarly for
> property format definitions).

I like the sound of that, and the symmetry with format-text.

> > +     description => "User-friendly display name for storage",
> > +     type => 'string',
> > +     optional => 1,
>
> adding a generous maxLength (say something between 64 to 128) and a pattern
> to avoid other whitespace-types than, well, a plain space would IMO be good
> to add.
>
> Without encoding 2) would already be quite close to that, but I'd really
> like to avoid adding generic XSS injection as a feature ;-)

Haha, yes. 100% agree. :D

> Anyhow, if you want to for 2) then please feel free to do so.

Great -- I'm going to submit a couple patches for option 2 in a bit.

Really appreciate the review and the quick feedback!

Josh