feat: require explicit registration of absolute database paths#50
feat: require explicit registration of absolute database paths#50onehumandev wants to merge 1 commit into
Conversation
|
@velocitysystems For your review :) Thanks! |
|
@onehumandev Added some comments. |
4af4219 to
096fe5f
Compare
| tauri::Builder::default() | ||
| .plugin( | ||
| Builder::new() | ||
| .on_setup(|app, reg| { |
There was a problem hiding this comment.
The dynamic-path setup reads as more boilerplate than the old add_migrations("main.db", …) one-liner, and some of that looks avoidable without giving up the closure's flexibility (arbitrary, app-derived absolute paths).
Two things stand out in the example:
-
We register the same database twice —
allow_path(&db)andadd_migrations(...)— and have to keep the keys in sync by hand. That's the same coupling behind the byte-identical-key concern: if the two spellings ever drift, migrations silently attach to a key load() never matches. -
The two calls take different types for the same value: allow_path takes
&db(a path) while add_migrations takesdb.to_string_lossy().into_owned()(a String).
Two ways to collapse this to a singlecall, both keeping &Path and canonicalizing to the same key internally:
Option A — add_migrations auto-allowlists. Have it take impl AsRef<Path> and register the path on the allowlist itself. You can't migrate a database you're not allowed to open, so allow-listing a migration target is implied:
.on_setup(|app, reg| {
let db = app.path().app_data_dir()?.join("main.db");
reg.add_migrations(&db, sqlx::migrate!("./migrations")); // allows + migrates
Ok(())
})allow_path(&db) stays for databases without migrations. Downside: add_migrations now has an allowlist side effect, which is slightly less obvious from its name.
Option B — chained handle. Keep allow_path as the single allowlist entry point and have it return a per-path handle you attach migrations to:
.on_setup(|app, reg| {
let db = app.path().app_data_dir()?.join("main.db");
reg.allow_path(&db).with_migrations(sqlx::migrate!("./migrations"));
Ok(())
})This keeps add_migrations/allow_path responsibilities cleanly separated (allow-listing stays the one explicit gate) while still guaranteeing the migration key is the allowed path. allow_path(&db) alone still works for the no-migrations case.
Either removes the stringified key and the hand-synced double registration. What are your thoughts @onehumandev @jjhafer?
There was a problem hiding this comment.
Option A makes more sense to me; when we register the path, we register the migrations with it as well.
I'll make that change.
There was a problem hiding this comment.
If we go with Option A, we'll still need to retain the allow_path(..) API for databases that don't require migrations. I'm still somewhat hesitant about this approach, though, since it introduces implicit behavior into the add_migrations(..) method.
That said, the builder pattern in Option B is appealing because it keeps the API surface unchanged while making it more composable. Perhaps @jjhafer may have some thoughts before we commit to either path?
There was a problem hiding this comment.
Sorry, I realized I didn't actually read option B :) That is a better option than A; I'll await @jjhafer's thoughts.
And one other question for my own edification, as it relates to this; is there a particular reason we chose to pass down and cache the raw strings as the path key instead of using PathBuf?
It's not an issue either way, but given that we are keying against a string that we are requiring to be a path, I was curious why we didn't use PathBuf, even if just for semantic reasons.
There was a problem hiding this comment.
Good question — it was deliberate for a couple of specific reasons:
- The key is string-shaped at both ends. It arrives from the frontend as a JSON string and ends up as a SQLite connection string for sqlx. Where path semantics matter — .. checks, canonicalization, the allowlist match — the code does use
PathBuf; theStringonly reappears after validation, as an opaque cache key. - Not all keys are paths. :memory: and file:…?mode=memory URIs share the same maps, and
PathBufis the wrong type for those.
There was a problem hiding this comment.
After looking at this more, I think there's a simpler way to do this, which is a single register_database method that takes the db path and an optional Migrator as a parameter. That solves the possible duplication issue, and allows creating a DB without a migrator.
I've updated this MR with this change; let me know if this is acceptable, or if another solution would be preferred. Thanks!
096fe5f to
e0b8d48
Compare
Problem: Previously, only databases that lived in `app_config_dir` could be opened with this plugin. The user passed in the path relative to `app_config_dir` to load the datbase. This meant that there was no support for databases that may live anywhere else within the app, which is a reasonable requirement. For example, some databases may need to live the `Library` folder on an iOS app so the database will be kept as part of the app backup. Solution: Databases can now only be opened by an exact, pre-registered absolute path (in-memory databases are the sole exception). Paths are registered via Builder::allow_path or, more commonly, in the new Builder::on_setup hook, which runs during plugin setup once `app` exists — so paths derived from app.path().app_data_dir() and friends can be computed and registered at startup. Registered paths are canonicalized once at setup so the equality check is symlink-safe. The resolver canonicalizes each requested path and requires an exact allowlist match. `..` segments and null bytes are always rejected. Migration registration flows through the same resolver, so a migration's key must be a registered absolute path. Why no relative paths: A relative-path default (app_config_dir) is an implicit grant — every file under that dir becomes reachable without the developer opting in. Removing it makes the set of reachable databases exactly the set the developer registered, with no ambient authority, and gives exactly one path to accessing a db. Why no registered root folders: Root allow-listing grants a whole subtree, including files created later that the developer never considered. Prefix matching is also easy to get subtly wrong against symlinks and traversal. Exact-path allow-listing is the least-privilege equivalent: explicit, auditable, and unambiguous. BREAKING CHANGE: relative paths are removed. load() now requires an absolute path registered via allow_path / on_setup. Previously-relative callers must register the absolute path and pass it from the frontend. It is the responsibility of the caller to ensure a consistent passing down of the needed path.
e0b8d48 to
0f92369
Compare
Problem:
Previously, only databases that lived in
app_config_dircould be opened with this plugin.The user passed in the path relative to
app_config_dirto load the database.This meant that there was no support for databases that may live anywhere else within the app, which is a reasonable requirement. For example, some databases may need to live the
Libraryfolder on an iOS app so the database will be kept as part of the app backup.Solution:
Databases can now only be opened by an exact, pre-registered absolute path (in-memory databases are the sole exception). Paths are registered via Builder::allow_path or, more commonly, in the new Builder::on_setup hook, which runs during plugin setup once
appexists — so paths derived from app.path().app_data_dir() and friends can be computed andregistered at startup.
Registered paths are canonicalized once at setup so the equality check is symlink-safe.
The resolver canonicalizes each requested path and requires an exact allowlist match.
..segments and null bytes are always rejected.Migration registration flows through the same resolver, so a migration's key must be a registered absolute path.
Why no relative paths:
A relative-path default (app_config_dir) is an implicit grant — every file under that dir becomes reachable without the developer opting in. Removing it makes the set of reachable databases exactly the set the developer registered, with no ambient authority, and gives exactly one path to accessing a db.
Why no registered root folders:
Root allow-listing grants a whole subtree, including files created later that the developer never considered. Prefix matching is also easy to get subtly wrong against symlinks and traversal. Exact-path allow-listing is the least-privilege equivalent: explicit, auditable, and unambiguous.
BREAKING CHANGE: relative paths are removed.
load() now requires an absolute path registered via allow_path / on_setup. Previously-relative callers must register the absolute path and pass it from the frontend.
It is the responsibility of the caller to ensure a consistent passing down of the needed path.