AssertionError if [inherit] htdocs-dir is empty or not defined

[Ryan J Ollos]

This was initially posted in comment:2:ticket:11294.

If [inherit] htdocs_dir is empty or doesn't exist, then the following traceback can be seen in the console when we try to load a page that uses an image from shared:

02:45:12 PM Trac[main] ERROR: Internal Server Error: 
Traceback (most recent call last):
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/main.py", line 497, in _dispatch_request
    dispatcher.dispatch(req)
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/main.py", line 214, in dispatch
    resp = chosen_handler.process_request(req)
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/chrome.py", line 621, in process_request
    assert os.path.commonprefix([dir, path]) == dir
AssertionError: 

If I'm using the Image macro to display trac_logo.png from the shared directory, but that directory doesn't exist, I see:

03:05:29 PM Trac[chrome] WARNING: File trac_logo.png not found in any of [u'/var/www/shared/htdocs']

However, if [inherit] htdocs_dir = , or the option isn't defined in trac.ini, the traceback results.

If I change the code slightly to show the values being asserted:

03:09:21 PM Trac[main] ERROR: Internal Server Error: 
Traceback (most recent call last):
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/main.py", line 497, in _dispatch_request
    dispatcher.dispatch(req)
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/main.py", line 214, in dispatch
    resp = chosen_handler.process_request(req)
  File "/home/user/Workspace/t11294/teo-rjollos.git/trac/web/chrome.py", line 621, in process_request
    assert os.path.commonprefix([dir, path]) == dir, [os.path.commonprefix([dir, path]), dir]
AssertionError: ['', '.']

Another way to reproduce the traceback is to set [header_logo] src to a file in the shared htdocs_dir and leave [inherit] htdocs_dir empty.

If we remove the assertion, we get a more useful error in the log:

03:22:34 PM Trac[chrome] WARNING: File trac_logo.png not found in any of ['.']
03:22:34 PM Trac[main] WARNING: [127.0.0.1] HTTPNotFound: 404 Not Found (File trac_logo.png not found)

I'm not sure what the purpose of that assert statement is. It was added in [1888], and I can't come up with a case for which the assertion could have been false in [1188]. However, after [1892] the assertion can be false if os.normpath(dir) != dir.

Provided the assertion isn't needed, I propose to remove it and possibly improve the log message for the case that the user tries to serve an image file from the shared htdocs dir, but [inherit] htdocs_dir is empty or not defined.

[Jun Omae]

No. If without the assertion, a remote attacker can retrieve the system file using like this.

http://example.org/chrome/site/..%2Fconf%2Ftrac.ini
http://example.org/chrome/common/..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd

[Remy Blank]

If the assert has security implications, it should be changed to a test and a raise ASAP, because assertions are removed when running with -O, and we don't want people's Trac to become vulnerable just because they run with "optimizations" turned on.

[Ryan J Ollos]

I've prepared some changes based on Rémy's suggestion: rjollos.git:t11295.

I'm not sure if the TracErrors's message should be more descriptive than Invalid chrome path.

[Ryan J Ollos]

I added a fix and test case in rjollos.git:t11295 so that a TracError is not raised when there is a request at the path /chrome/shared and [inherit] htdocs_dir is missing or empty.

[Ryan J Ollos]

I'll rebase the changes on the 0.12 branch before committing.

[Ryan J Ollos]

I searched through the rest of the codebase for uses of assert. There is one case where it is used in tags/trac-1.0.1/trac/web/auth.py@:158-159#L130​ and since the message is translated it looks like maybe the message is intended to be shown to users. I'll have to look more closely at the code to understand, but should that be potentially replaced with an if ...: raise TracError?

Every other use of assert that I saw in the codebase was either in a unit test, or appeared to be intended to catch programming errors. I don't think this is a big deal, but the frequent uses of assert in unit tests puzzles me, for example tags/trac-1.0.1/trac/web/tests/auth.py@:111#L97​. Is there a technical or stylistic factor that determines when one might want to use an assert rather than an assert method of the TestCase class? I don't know a reason this is "bad" other than the optimization issue Rémy noted in comment:2, but it just seems odd to mix the use of the two, and the assert methods of the UnitTest class can be more readable (and become more readable once we extend the class with unittest2 methods such as assertIn). If there's no particular reason to mix the two, I might do some refactoring after #11284.

[Remy Blank]

Personally, I'd be in favor of replacing all uses of assert, by conditions and raise in "normal" code and by asserters in test code.

[Ryan J Ollos]

Committed to 0.12-stable in [12050], merged to 1.0-stable in [12051]: Replaced several uses of assert with a conditional that raisees a TracError since the latter won't be removed when running Python with the optimization (-O) flag.

Committed to 1.0-stable in [12052], merged to trunk in [12053]: An HTTPError is raised when accessing a file on the /chrome/shared path if [inherit] htdocs_dir is empty or not defined.

[Ryan J Ollos]

Replying to rblank:

Personally, I'd be in favor of replacing all uses of assert, by conditions and raise in "normal" code and by asserters in test code.

In #11419 I'll replace the asserts in trunk/trac/ticket/model.py​ with raises.

[Ryan J Ollos]

Replying to rjollos:

Replying to rblank:

Personally, I'd be in favor of replacing all uses of assert, by conditions and raise in "normal" code and by asserters in test code.

In #11419 I'll replace the asserts in trunk/trac/ticket/model.py​ with raises.

… and in the rest of the codebase in #12004.