Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

#11397 closed defect (fixed)

TracAdmin allows attachment to be added to non-existent resource

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: admin/console Version:
Severity: normal Keywords: attachment
Cc: Branch:
Release Notes:

Raise an exception when adding an attachment through trac-admin if the parent resource doesn't exist. Raise an error if navigating to an attachment page for which the parent resource doesn't exist.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

To reproduce:

  • trac-admin $ENV attachment add wiki:SomeNonExistentPage file
  • Navigate to /attachment/wiki/SomeNonExistentPage and the attachment will be found

  • Navigate to /wiki/SomeNonExistentPage to find the following:

Even worse, an attachment could be created for a non-existent realm:

$ trac-admin $ENV attachment add some:none file1

When attempting to view the attachment at the path /attachment/some/none/file1, the permission check will fail: ATTACHMENT_VIEW privileges are required to perform this operation on Attachment 'file1' in some:none. You don't have the required permissions.. However, inspection of the database or execution of the trac-admin attachment list command shows that the attachment exists.

Proposed changes can be found in log:rjollos.git:t11397. Besides the fix for the issue in this ticket, there is one additional change proposed:

  • Raise an error if navigating to an attachment page for which the parent resource doesn't exist.

Attachments (2)

SomeNonExistentPage.png (16.9 KB ) - added by Ryan J Ollos 7 years ago.
WikiPage.png (22.4 KB ) - added by Ryan J Ollos 7 years ago.

Download all attachments as: .zip

Change History (11)

by Ryan J Ollos, 7 years ago

Attachment: SomeNonExistentPage.png added

comment:1 by Ryan J Ollos, 7 years ago

Component: attachmentadmin/console
Description: modified (diff)
Status: newassigned

by Ryan J Ollos, 7 years ago

Attachment: WikiPage.png added

comment:2 by Ryan J Ollos, 7 years ago

Description: modified (diff)

comment:3 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12330] and merged to trunk in [12331].

comment:4 by Jun Omae, 7 years ago

The test_attachment_add_nonexistent_resource fails on Windows.

FAIL: test_attachment_add_nonexistent_resource (trac.admin.tests.console.TracadminTestCase)
Tests the 'attachment add' command in trac-admin, on a non-existent
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\usr\src\trac\trac.git\trac\admin\tests\console.py", line 257, in test_attachment_add_nonexistent_resource
    self.assertEqual(self.expected_results[test_name], output)
  File "C:\usr\src\trac\trac.git\trac\admin\tests\console.py", line 159, in assertEqual
    output, diff()))
AssertionError: u"ResourceNotFound: NonExistentPage doesn't exist, can't create attachment\n" != u"IOError: [Errno 13] Permission denied: u'c:\\\\docume~1\\\\jun66j5\\\\locals~1\\\\temp\\\\tmp8snnqg'\n"
--- expected
+++ actual
@@ -1,2 +1,2 @@
-ResourceNotFound: NonExistentPage doesn't exist, can't create attachment
+IOError: [Errno 13] Permission denied: u'c:\\docume~1\\jun66j5\\locals~1\\temp\\tmp8snnqg'

----------------------------------------------------------------------
Ran 1437 tests in 123.453s

FAILED (failures=1)

In Windows, NamedTemporaryFile create a temporary file with os.O_TEMPORARY flag. The temporary file with the flag cannot be opened again. It leads the failure.

Python 2.5.4 (r254:67916, Dec 23 2008, 15:10:54) [MSC v.1310 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> fd = os.open('C:/tmp/blah.txt',
...              os.O_RDWR | os.O_CREAT | os.O_EXCL | os.O_BINARY | os.O_TEMPORARY,
...              0600)
>>> fd
3
>>> open('C:/tmp/blah.txt')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IOError: [Errno 13] Permission denied: 'C:/tmp/blah.txt'
>>>

It would be simple to use __file__ rather than tempfile.

  • trac/admin/tests/console.py

    diff --git a/trac/admin/tests/console.py b/trac/admin/tests/console.py
    index 1615316..2fe20ce 100644
    a b class TracadminTestCase(unittest.TestCase):  
    250250        """Tests the 'attachment add' command in trac-admin, on a non-existent
    251251        resource."""
    252252        test_name = sys._getframe().f_code.co_name
    253         file = tempfile.NamedTemporaryFile()
    254253        rv, output = self._execute('attachment add wiki:NonExistentPage %s'
    255                                    % file.name)
     254                                   % __file__)
    256255        self.assertEqual(2, rv)
    257256        self.assertEqual(self.expected_results[test_name], output)

comment:5 by Ryan J Ollos, 7 years ago

Thanks. Committed to 1.0-stable in [12390:12391], and merged to trunk in [12392].

comment:6 by Jun Omae, 7 years ago

Resolution: fixed
Status: closedreopened

After r12330, unit tests failed with PostgreSQL. Also the same occur on r12494.

$ git log --max-count=1 HEAD | grep git-svn-id
    git-svn-id: http://trac.edgewall.org/intertrac/log:/branches/1.0-stable@12330 af82e41b-90c4-0310-8c96-b1721e28e2e2
$ make python=26 db=postgres unit-test
Python version: Python 2.6.6
figleaf:
coverage: /home/jun66j5/bin/coverage
PYTHONPATH=.:
TRAC_TEST_DB_URI=postgres://tracuser:password@localhost:5432/tractest?schema=tractest
server-options= -p 3000 -a '*,/home/jun66j5/src/trac-htdigest.txt,auth' -s -r -e
python setup.py egg_info
running egg_info
writing requirements to Trac.egg-info/requires.txt
writing Trac.egg-info/PKG-INFO
writing top-level names to Trac.egg-info/top_level.txt
writing dependency_links to Trac.egg-info/dependency_links.txt
writing entry points to Trac.egg-info/entry_points.txt
reading manifest file 'Trac.egg-info/SOURCES.txt'
writing manifest file 'Trac.egg-info/SOURCES.txt'
python ./trac/test.py --skip-functional-tests
/home/jun66j5/venv/py26/lib/python2.6/site-packages/twill/other_packages/_mechanize_dist/_auth.py:14: DeprecationWarning: the md5 module is deprecated; use hashlib instead
  import re, base64, urlparse, posixpath, md5, sha, sys, copy
/home/jun66j5/venv/py26/lib/python2.6/site-packages/twill/other_packages/_mechanize_dist/_auth.py:14: DeprecationWarning: the sha module is deprecated; use the hashlib module instead
  import re, base64, urlparse, posixpath, md5, sha, sys, copy
EEEEEEEEEEEEE...................................................................
........................EEEEEEEE................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
..........................
======================================================================
ERROR: test_delete (trac.tests.attachment.AttachmentTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/trac.git/trac/tests/attachment.py", line 65, in setUp
    self.env.db_transaction("INSERT INTO wiki (name) VALUES ('WikiStart')")
  File "/home/jun66j5/src/trac.git/trac/db/api.py", line 122, in execute
    return db.execute(query, params)
  File "/home/jun66j5/src/trac.git/trac/db/util.py", line 121, in execute
    cursor.execute(query, params)
  File "/home/jun66j5/src/trac.git/trac/db/util.py", line 56, in execute
    r = self.cursor.execute(sql)
IntegrityError: null value in column "version" violates not-null constraint

...

======================================================================
ERROR: Test mailto: not obfuscated, unlike plain email
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/trac.git/trac/wiki/tests/formatter.py", line 190, in setUp
    wiki.save('joe', 'Entry page', '::1', datetime.now(utc))
  File "/home/jun66j5/src/trac.git/trac/wiki/model.py", line 143, in save
    self.readonly))
  File "/home/jun66j5/src/trac.git/trac/db/util.py", line 121, in execute
    cursor.execute(query, params)
  File "/home/jun66j5/src/trac.git/trac/db/util.py", line 54, in execute
    r = self.cursor.execute(sql_escape_percent(sql), args)
IntegrityError: duplicate key value violates unique constraint "wiki_pk"


----------------------------------------------------------------------
Ran 1466 tests in 237.282s

FAILED (errors=21)
make: *** [unit-test] Error 1

comment:7 by Jun Omae, 7 years ago

Proposed fix can be found in jomae.git@t11397.1.

comment:8 by Ryan J Ollos, 7 years ago

The changes look good. Tested with Python 2.7; PostgreSQL, MySQL and SQLite on Ubuntu 13.04.

Thank you for fixing this. I've been sloppy by not running the unit tests for all databases, but with the makefile it is so easy that there is really no excuse.

comment:9 by Jun Omae, 7 years ago

Resolution: fixed
Status: reopenedclosed

Thanks for testing. I tested with Python 2.5-2.6, SQLite 3.3.6, PostgreSQL 8.1.23 and MySQL 5.0.95. No worries. I had not run the tests for all databases, also.

Committed and merged in [12498-12499].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.