Repository navigation
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no problems. The fallback branch now rejects every traversal I tried and still serves package results and info_output/ files. commonpath compares whole path components, so a sibling directory such as donated-results-x or a file such as donated-resultss is not treated as being inside the result path, which a plain startswith check would have allowed. The other handlers (/head-, /diff-, /check_library- and so on) only use the URL text to match message ids or function names. They do not build file paths from it. I did not check the upload path (os.path.join(resultPath, res.group(1))), which this PR does not change.
Testing
This PR only changes Python, so I did not build cppcheck or testrunner.
- I ran the test the same way CI does (
scriptcheck.yml):PYTHONPATH=tools python3 -m pytest -Werror --strict-markers -vv test/tools/donate_cpu_server_test.pyinpr/. Both tests passed. - I copied the new test into
base/and ran it there.test_file_outside_result_pathfails, so the test does catch the bug. - I wrote
scratch/edge.py, which callsHttpClientThread(...).run()directly with a temporary result directory. It also creates a secret file next to it, a sibling directorydonated-results-x/and a filedonated-resultss. I ran each request with the result path written three ways: absolute, absolute with a trailing/, and relative.- These URLs all got
404and nothing leaked:/../s,/../donated-results-x/s,s(no leading slash),/.././s,//../s,/info_output/../../s,..,/%2e%2e/s(not percent-decoded, so harmless). - These URLs still got
200with the right content:/pkg,/./pkg,/a/../pkg,/info_output/i,/. - The relative result path did not raise an exception in
commonpath.
- These URLs all got
Automated review of 74f8769 by Claude. The pull request was built and tested locally.
the fallback branch in HttpClientThread.run serves self.resultPath + url for any GET path that is not a known report, and the path from parse_req is not normalized. a raw request like
GET /../../../etc/passwd HTTP/1.1(browsers collapse the dots, a socket orcurl --path-as-isdoes not) gets back any text file the daca server user can read. normalize the joined path and answer 404 unless it stays under the result path, so package results and info_output/ files are served as before. test_file_outside_result_path in test/tools/donate_cpu_server_test.py fails without the change. there is no trac ticket for this yet, please create one or let me know and i will update the title.