Repository navigation
fix: prevent path traversal in datasource importToDb (CWE-22) - #1435
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cause
POST /datasource/importToDbbuilds the file path withos.path.join(path, import_req.filePath).filePathcomes from the request body. An absolute path replaces the upload directory, and../segments lead out of it. The file is then read byread_import_dataframeand imported into a table, so aws_admincaller can import any file the process can read and pandas can parse (a path ending in.csv, or a workbook)./datasource/parseExcelgenerates the name on upload, but/importToDbaccepted any value.Fix
Resolve the path with
os.path.realpathand require that it lies inside the resolved upload directory (os.path.commonpath), as the Excel upload fixes in #1284 and #1350 do. The upload directory itself ("",".") is rejected too. Because the path is resolved, a symlink inside the upload directory that points outside is rejected as well.Rejected requests return 400 with the new
i18n_ds_invalid_file_pathmessage, added to all five locales. A path thatrealpathcannot handle (for example one with a null byte) raises aValueError; it is caught and gets the same 400. The path check runs before the existence check, so the response does not reveal whether a file exists outside the upload directory. Names returned by/parseExcelkeep working.Tests
New file
backend/tests/test_datasource_import_path_traversal.py, offline, in the style oftest_excel_http_errors.py(real router and middleware; identity, audit log, session, engine and file reader are stubs). 20 cases:../variants,..,nested/..: 400../excel_backup/...): 400i18n_ds_invalid_file_path, notFile not found""and".": 400filePath: 400File not found(unchanged)Without the fix 16 of the 20 cases fail. With the fix, run with Python 3.11.16, sqlbot-xpack 0.0.5.41 and pytest 7.4.4 on base
96f06c7e:BASE_DIR,UPLOAD_DIR,EXCEL_PATH,MCP_IMAGE_PATH,LOCAL_MODEL_PATHandSCRIPT_DIRpointed to a temporary directory because/opt/sqlbotis not writable on the test machine. Of the 83 skipped tests, 77 need an opt-in PostgreSQL database and 6 need the frontend npm dependencies; both groups are existing skips.datasource.pykeeps its 18 existing Ruff findings and its existing formatting; no unrelated formatting was applied.The upload and import flow (
/parseExcelwith CSV and XLSX files, then/importToDbwith the returnedfilePath) was also checked locally with Chinese names, spaces and several dots in the file name; the frontend sends thefilePathfrom/parseExcelback unchanged.Not covered
The issue also reports that the import pipeline has no size, row or sheet limit before parsing the workbook in memory. This PR does not change that.
Fixes #1434