search

h5p / h5p-cli

Command Line Interface
MIT License
64 stars 33 forks source link

Using folder name for content id breaks going to fullscreen (and potentially more) #53

Closed otacke closed 1 year ago

otacke commented 1 year ago

Currently, the H5P-CLI tool rewrite is using the unsanitized content's folder name for the content id. Cmp. https://github.com/h5p/h5p-cli/blob/97b95741de79339ed1959171f4f4ebcae3a2509d/assets/templates/view.html#L16 I suggest to not do that.

For instance, the content id will be used to find the H5P iframe which is requested to be sent to full screen. A query selector is used and that query selector composition uses the content id. Cmp. https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L540

If the content id contains characters reserved for query selecting, e.g. a blank or a dot, that will throw off the query selection, no iframe will be found and the request to go to full screen will not be granted - in fact you'll find an error message in the console.

I am not sure where else using a "random" content id might cause trouble in H5P core ...

To reproduce:

  1. Create an Interactive Video content or some other content type that is capable of going to full screen.
  2. Name it "foo bar" or "foo.bar" or give it some other name that contains characters reserved in query selectors. Result: The folder name will contain a blank, a dot, etc.
  3. View the content and try to send it to full screen.

Suggestion A natural choice would be to simply use a UUID for the folder name, as then both the subcontent id and the content id would use the same scheme. That would also prevent the "folder already exists" exception and a developer could call as many contents "test" as he/she wants to :-)

If the folder name should give the developer some insight into what content it contains by using the content's title for the folder name, then it should be sanitized to not contain any characters that could throw off the query selecting.

devland commented 1 year ago

Hey, Oliver. Good catch. :) I've sanitized the user input title that is used for the folder name so that only alpha-numeric & dash characters are used; spaces are converted to dashes. https://github.com/h5p/h5p-cli/commit/2f2978c7183b52707140452a389b35eb495c0d69

otacke commented 1 year ago

@devland That should do, thanks a lot!