Ticket #89 (closed defect: fixed)

Opened 9 years ago

Last modified 4 years ago

Files Action - Windows Paths Issue

Reported by: dartar Owned by: BrianKoontz
Priority: normal Milestone: 1.3.1
Component: actions Version: 1.1.6.1
Severity: normal Keywords:
Cc:

Description (last modified by DarTar) (diff)

(reported by QualousHere) On a Windows/IIS installation, including the {{files}} action causes errors similar to the following:

	Warning: mkdir(uploads\SandBox): Permission denied in D:\sitepath\wwwroot\wikka\actions\files.php on line 12
	Warning: opendir(uploads\SandBox): failed to open dir: Invalid argument in D:\sitepath\wwwroot\wikka\actions\files.php on line 156
	Warning: readdir(): supplied argument is not a valid Directory resource in D:\sitepath\wwwroot\wikka\actions\files.php on line 157
	Warning: closedir(): supplied argument is not a valid Directory resource in D:\sitepath\wwwroot\wikka\actions\files.php on line 200 

This is caused by the assumption of "/" as the directory seperator instead of Window's "\". Using the DIRECTORY_SEPARATOR variable corrects this issue. In actions/files.php change:

	$upload_path = $this->config['upload_path'].'/'.$this->GetPageTag();

into

	$upload_path = $this->config['upload_path'].DIRECTORY_SEPARATOR.$this->GetPageTag();

and also:

	$destfile = $upload_path.'/'.$strippedname;

into

	$destfile = $upload_path.DIRECTORY_SEPARATOR.$strippedname;

Warning: This has only been tested so far under Windows.

Related tickets

#72 #46

Change History

Changed 8 years ago by DarTar

  • description modified (diff)

Changed 8 years ago by DarTar

  • milestone changed from 1.1.6.2 to 1.1.6.3

Changed 8 years ago by BrianKoontz

  • owner changed from unassigned to BrianKoontz
  • status changed from new to assigned

Tested under Unix/OS X, appears to work OK.

DIRECTORY_SEPARATOR is part of the Pear library. Is this a standard part of the PHP installation? Here's the functional expansion:

DIRECTORY_SEPARATOR = strtoupper(substr(PHP_OS,0,3)=='WIN')?'\\':'/'

Changed 8 years ago by BrianKoontz

  • status changed from assigned to closed
  • resolution set to fixed

Appears to have been fixed in [217].

Changed 8 years ago by JavaWoman

Working on #340 (Internationalization) and removing all markup from language strings, I came across error messages in the files action that still contained literal '/'. While that will not break anything, it may be confusing. Fixing those, too... done in [294].

Note: DIRECTORY_SEPARATOR is part of standard PHP - see:  http://php.net/manual/en/reserved.constants.php

Changed 8 years ago by JavaWoman

1.1.6.3: [402] applies [294]:[330] from trunk (with some conflicts resolution) for #312; also some initializations added (#38), slashes replaced by DIRECTORY_SEPARATOR (#89), and a lot of docblocks added or updated. (phew)

Changed 8 years ago by JavaWoman

also:
[394] applies DIRECTORY_SEPARATOR in diff.php
[396] applies DIRECTORY_SEPARATOR in files.xml.php

Changed 7 years ago by JavaWoman

(In [428]) implementing DIRECTORY_SEPARATOR - refs #89

Changed 7 years ago by JavaWoman

(In [429]) implementing DIRECTORY_SEPARATOR - refs #89

Changed 7 years ago by JavaWoman

(In [431]) Handles several problems related to IncludeBuffered(); there is no ticket for this, comments refer to [SEC] instead! Extra-long commit message to overcome lack of ticket info.

IncludeBuffered is used by Action(), Method() and Format(); the latter "calls" Action() (so IncludeBuffered() is "passed" twice when handling actions in the source). To make this complex more secure, all four methods need to be regarded (and including their interaction). Apart from comments, changes include:

1. Action()

  • more direct parsing of the action code, combined with validation of syntactically-valid action name
  • regard only parameters that actually have a name
  • sanitize all parameters with htmlspecialchars_ent() (if an action really needs "raw" HTML input it's its own responsibility to unescape this)

2. Method()

  • added a "syntax check" for method name, similar to that in Action(); the RE covers at least all current handlers
  • implements DIRECTORY_SEPARATOR instead of '/' for constructing $methodLocation ( refs #89 )
  • make Method name in URL case-sensitive by converting valid method name to lowercase (again, as with Action())

3. Format()

  • added a "syntax check" for method name, similar to that in Action(); the RE covers at least all current formatters (and even those of GeSHi)
  • make formatter name case-sensitive by converting valid formatter name to lowercase (again, as with Action())

4. IncludeBuffered()

  • $path parameter (filled by the calling methods with a path specified in the configuration file) is no longer allowed to contain a list of paths, to enforce strict directory structure
  • implements DIRECTORY_SEPARATOR instead of '/' for constructing path to file to be included ( refs #89 )
  • added EXTR_SKIP parameter to extract() so (action) parameters can no longer override existing variables in the current scope
  • if the path to the file to be included is not found, the provided error message (which may contain user-provided data such as a "hackish" path in a link) is sanitized before being returned for display

This is a "commented" version for extra clarification of what was observed, and then done; a cleanup version removing some of these will follow.

Changed 7 years ago by DarTar

Excellent job, thanks. I cannot test this right now, but I'll do later next week. Once this has been tested, we should port this code to trunk.

Changed 7 years ago by DotMG

(In [468]) refs #89. a little issue in the path (inopportune DIRECTORY_SEPARATOR)

Changed 7 years ago by DarTar

Mahefa, can you please give some explanations about the code changes committed in [468]? inopportune doesn't say much and I don't understand why this constant was removed, whereas it was introduced in [431].

Please every time you commit code, try to give detailed explanations about the reasons for a specific code change so that other developers can build on this change.

Changed 7 years ago by DotMG

Sorry! As one can see in the diff in [468], I removed leading DIRECTORY_SEPARATOR before $upload_path, ie replaced

'<tt>'.DIRECTORY_SEPARATOR.$upload_path.'</tt>'
with
'<tt>'.$upload_path.'</tt>'

The older version throws error message : Please make sure that the server has write access to a folder named /uploads; instead of Please make sure that the server has write access to a folder named uploads.

The $configupload_path?, as we defined it, is relative to wikka installation, not to root directory. The former version may mislead the user to create a folder named uploads at the root directory and give write access to it.

Hope that is clear enough, :)

Changed 7 years ago by DarTar

  • status changed from closed to reopened
  • resolution fixed deleted

Changed 7 years ago by DarTar

Shouldn't DIRECTORY_SEPARATOR be used anytime files are included via include/require ?

There's a require_once in Wakka.class.php (line 408) and two in wikka.php (line 88 & 130) that do not make use of DIRECTORY_SEPARATOR. I'm reopening this ticket accordingly.

Changed 7 years ago by BrianKoontz

(In [541]) Replaced more instances of Unix-style file separators with DIRECTORY_SEPARATOR define. Refs #89.

Changed 7 years ago by BrianKoontz

  • status changed from reopened to closed
  • resolution set to fixed

Changed 6 years ago by DarTar

  • milestone changed from 1.2 to 1.3

Retargeting to 1.3, this ticket has already been closed in trunk, from which 1.3 will be branched. Consider backporting urgent issues to 1.2.X

Changed 4 years ago by BrianKoontz

  • milestone changed from 1.3 to 1.3.1

Updated milestone to 1.3.1

Note: See TracTickets for help on using tickets.