If you select the "Private files" upload destination in the UI, the destination is not saved into the field configuration.

Expected

file:
  '#type': managed_file
  '#title': File
  '#uri_scheme': private
  '#max_filesize': '5'
  '#file_extensions': pdf

Actual

file:
  '#type': managed_file
  '#title': File
  '#max_filesize': '5'
  '#file_extensions': pdf

I guess it works if Private is the default system file destination, but most sites I know of are not configured that way.

I am marking this critical in light of #2817535: Drupal file upload by anonymous or untrusted users into public file systems -- PSA-2016-003 and PSA-2016-003. Someone who creates a form may think they are uploading to the private file system but they are not.

CommentFileSizeAuthor
#15 Screen Shot 2016-10-24 at 11.51.39 AM.png32.16 KBjrockowitz
#11 managed_file_element-2821231-11.patch5.77 KBjrockowitz
#11 Test managed_file element YAML Form Demo Site.png791.13 KBjrockowitz
#4 managed_file_element-2821231-4.patch1.41 KBjrockowitz

Comments

cilefen created an issue. See original summary.

cilefen’s picture

cilefen commented
Issue summary: View changes
cilefen’s picture

cilefen commented
Issue tags: +Needs tests
jrockowitz’s picture

jrockowitz commented
Status: Active » Needs review
StatusFileSize
new managed_file_element-2821231-4.patch1.41 KB

I think that PSA-2016-003 is huge problem for modules like Webforms and YAML Forms because generally file uploads are exposed to anonymous users.

Since YAML Form is still in beta, so I decided to default file uploads to be private unless the site administrator explicitly enables public file uploads.

There is a bug where file uploads are not defaulting to private (ie your example above). The attached patch fixes this issue.

BTW, only properties that override default values are written to an element's YAML, which is why '#uri_scheme': private is missing from your example.

cilefen’s picture

cilefen commented
Status: Needs review » Needs work

BTW, only properties that override default values are written to an element's YAML, which is why '#uri_scheme': private is missing from your example.

Understood. But, the patch doesn't fix it.

cilefen’s picture

cilefen commented

If it helps to know this, when I select public, '#uri_scheme': public is saved, even though that is the site default. Private should be the override.

jrockowitz’s picture

jrockowitz commented

Actually, for the YAML Form module I want 'private' to be the default and 'public' the override.

It is worth noting that for new installs the managed file element is disabled unless private file uploads are enabled or the site admin explicitly enables public file uploads for the YAML Form module.

I will look into the issue.

cilefen’s picture

cilefen commented

Actually, for the YAML Form module I want 'private' to be the default and 'public' the override.

I agree. But I thought you meant override with respect to Form API.

cf74’s picture

cf74 commented

The patch does not work.

  • jrockowitz committed 77fb12f on 2821231-private-upload 
    Issue #2821231 by jrockowitz: Managed file element private upload...
jrockowitz’s picture

jrockowitz commented
StatusFileSize
new Test managed_file element YAML Form Demo Site.png791.13 KB
new managed_file_element-2821231-11.patch5.77 KB

I was able to figure out the problem. Still it is very difficult to write tests for the file uploads.

If you enable the yamlform_devel module, it provides an Element test form (/admin/structure/yamlform/settings/elements/managed_file/test) that now, with the attached patch, lets you view an element's serialized array and its final render array.

The below screenshot shows that the 'private://' uri schema is being correctly applied as the default destination for managed_file elements.

Please test the attached patch.

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
cf74’s picture

cf74 commented

Patch #11 is working.

cilefen’s picture

cilefen commented
Status: Needs review » Reviewed & tested by the community

#11 works. It also doesn't break the opt-in public destination. I am RTBC-ing this and leave test coverage to your discretion.

With respect to #2817535: Drupal file upload by anonymous or untrusted users into public file systems -- PSA-2016-003 and UX, it may be a good idea to put warnings about disabled elements on the 'entity.yamlform.edit_form' route in addition to the system status report. If that interests anyone I will open an issue.

(edited with the correct route name)

jrockowitz’s picture

jrockowitz commented
StatusFileSize
new Screen Shot 2016-10-24 at 11.51.39 AM.png32.16 KB

@cilefen I am not sure which routes should have the warning.

I did recently add link to the admin settings to the "Disabled element" warning message.

  • jrockowitz committed bc7bdd9 on 8.x-1.x 
    Issue #2821231 by jrockowitz, cilefen, cf74: Managed file element...
jrockowitz’s picture

jrockowitz commented
Status: Reviewed & tested by the community » Fixed
cilefen’s picture

cilefen commented

The warning in #15 is good and useful, but I do not understand the circumstances in which it would be seen. I assume the intent is to display it if there is a managed files field with public storage when the module configuration bans public files. But I do not see the warning in that situation.

Something along the lines of: "The Managed file element is disabled because..." when on the form editor would help when forms are being authored as a way to explain why the element is not available. Again, I don't want to clog up this issue. I can open another one.

jrockowitz’s picture

jrockowitz commented

The message is only displayed if a form contains a disabled element, either from copying a template or manually trying to use a disable element.

@cilefen Feel free to create a new ticket.

cilefen’s picture

cilefen commented

#2821775: Display a warning about unavailable form elements on the entity.yamlform.edit_form route

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.