pjfanning commented on PR #3080: URL: https://github.com/apache/drill/pull/3080#issuecomment-6003118624
*This review was generated by Claude AI (Anthropic), posted on behalf of @pjfanning.* The direction is right: using a rejecting `LinkResolver` is the correct hook, opening read-only is a good extra, and `equals`/`hashCode`/`toString` are all updated. There are a few issues, the first one significant. ### 1. Any query user can switch the protection off (main concern) The README documents enabling this per query, and that works because table-function parameters override the format config: ```sql FROM table(dfs.`x.accdb` (type=>'msaccess', tableName=>'Linked', allowLinkedDatabases=>true)) ``` `THREAT_MODEL.md` treats plugin config as admin-only (§3/§6, `OUT-OF-MODEL: trusted-input`), but this flag can be set by any query user. Someone who can upload an `.accdb` to a writable workspace and query it can turn the flag on themselves. The Drillbit then opens any local or UNC path as its service account, which also exposes it to NTLM hash capture over SMB. That is the attack this PR is meant to stop. Suggestions: - Make this an admin-only setting, such as a system/boot option or a plugin-level setting that table functions can't override. At minimum, have the reader ignore the flag when it comes from a table function. - Remove the "It can also be enabled per query" section from the README. ### 2. Listing tables now fails for any file that has a linked table With no `tableName`, the reader iterates `db.getTableNames()` and calls `db.getTable(name)` for each one in `processMetadataRow`. If Jackcess includes linked tables in `getTableNames()` (I believe it does), the rejecting resolver throws and the whole metadata query fails. Users can't even discover what is in the file. It would be better to skip linked tables, or return them with null counts, instead of failing. For example, check `db.getTableMetaData(name).isLinked()` before calling `getTable`. A test of the metadata query on a file with a linked table would confirm the behaviour. ### 3. Misleading error message `buildSchemaFromTable` wraps the error as `"Table Linked not found. Refusing to open linked database…"`. The table does exist, so "not found" is wrong. A clear `UserException` that says linked tables are disabled, and names the setting that enables them, would be better. ### 4. Test robustness - `testLinkedTableResolvedWhenAllowed` links to `//evil.example.com/share/secret.mdb`. On a Windows build agent with the flag on, Jackcess would attempt real SMB/DNS. A path under `dirTestWatcher` that doesn't exist would avoid that. - The test checks for Jackcess's `"given file does not exist"` text, which may change between versions. A stronger test would create a real target database with a row in it, link to it, and assert the row comes back when the flag is on. - Nothing tests the plugin-level setting on its own, without the table function. ### 5. Minor points - The public `@JsonCreator` constructor changes arity. There are no in-repo callers, so this is fine for a contrib plugin. - Unrelated, but nearby: in `processMetadataRow`, `updated_date` reads `table.getCreatedDate()`. It should probably be `getUpdatedDate()`. - The new `LinkResolver` import is out of alphabetical order. - Release notes should say that linked tables are now off by default, since that changes behaviour for existing users. **Suggest changes before merge:** #1 lets a non-admin user bypass the protection, and #2 is a usability regression worth fixing in the same PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
