Attached patch

  1. fixes Windows compatibility
  2. increases security by escaping shell arguments
  3. improves performance from 120+ seconds (fatal max_exec timeout...) to 40 seconds

Every single git command shell execution via PHP exec() takes 120ms on my (Windows) machine. I've tried a range of alternative shell execution methods, but none of them improved the performance in any way. I'm considering to add a filemtime() based (or alternative) cache to git_deploy to remedy that. But that's for another issue.

CommentFileSizeAuthor
#14 escapeshellarg-1471178-14.patch7.24 KBdarren oh
#14 interdiff-13-14.txt922 bytesdarren oh
#13 escapeshellarg-1471178-13.patch7.24 KBdarren oh
#13 interdiff-12-13.txt2.1 KBdarren oh
#12 escapeshellarg-1471178-12.patch7.66 KBdarren oh
#11 escapeshellarg-1471178-11.patch7.63 KBdarren oh
#9 escapeshellarg-1471178-9.patch6.2 KBdarren oh
#7 git_deploy-escapeshellarg-1471178-7.patch3.21 KBhswong3i
#5 git_deploy-escapeshellarg-1471178-5.patch2.1 KBhswong3i
#4 git_deploy-escapeshellarg-1471178-4.patch64 byteshswong3i
#3 git_deploy-escapeshellarg-1471178-3.patch2.09 KBhswong3i
git_deploy.performance-security-windows.0.patch3.61 KBsun

Comments

halstead’s picture

halstead commented

Thank you for this. I'll merge it into dev for testing soon.

darrell_ulm’s picture

darrell_ulm commented

Tested this (3 weeks ago) patch, works well.

That was three weeks ago, I just tried it again, and it finds contrib modules OK, but misreported the Drupal Core version as missing.

The Core .git dir and files are present.

Thank you

hswong3i’s picture

hswong3i commented
Title: Performance, Security, Windows-compatibility » Increases security by escaping shell arguments
StatusFileSize
new git_deploy-escapeshellarg-1471178-3.patch2.09 KB

IMHO let's focus on each issues one by one?

I give a quick cleanup for above patch and just focus on escapeshellarg() issue.

Any idea?

hswong3i’s picture

hswong3i commented
StatusFileSize
new git_deploy-escapeshellarg-1471178-4.patch64 bytes

I fork this issue to github: https://github.com/pantarei/drupal-git_deploy/tree/7.x-2.x-1471178

Just retouch the patch file with format-patch.

hswong3i’s picture

hswong3i commented
StatusFileSize
new git_deploy-escapeshellarg-1471178-5.patch2.1 KB

Sorry wrong file, here is the correct one.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

If you do that, then you need to take over the inline comments I've intentionally added to explain why some things are escaped and some others are not.

I don't see why you'd need a branch to re-create the patch with just the shell argument escaping changes.

hswong3i’s picture

hswong3i commented
StatusFileSize
new git_deploy-escapeshellarg-1471178-7.patch3.21 KB

Sorry for the miss of comment and here is a re-roll.

A branch to re-create patch just for better maintenance; on the other hand, since my team are keep maintenance our own Drupal distribution with git submodule (https://github.com/pantarei/pantarei-drupal), we need a public git reference point which can go live even upstream drupal.org still not yet accept the patch ;-)

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Issue summary: View changes
Issue tags: +Needs reroll
darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Title: Increases security by escaping shell arguments » [D7] Fix insecure handling of shell arguments
Assigned: sun » Unassigned
Issue tags: -Performance, -Windows, -Needs reroll
StatusFileSize
new escapeshellarg-1471178-9.patch6.2 KB
darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Status: Needs work » Needs review
darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new escapeshellarg-1471178-11.patch7.63 KB

Reroll.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new escapeshellarg-1471178-12.patch7.66 KB

Another reroll.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new interdiff-12-13.txt2.1 KB
new escapeshellarg-1471178-13.patch7.24 KB
darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
StatusFileSize
new interdiff-13-14.txt922 bytes
new escapeshellarg-1471178-14.patch7.24 KB

  • Darren Oh committed 6ed27ee on 7.x-2.x 
    Issue #1471178 by hswong3i, sun, Darren Oh: Fix insecure handling of...
darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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