Repository navigation
Reject unsafe namespace names before filesystem access - #31
raphaelfeitoza wants to merge 2 commits into
Conversation
3dc7d91 to
5bd6336
Compare
5bd6336 to
fbd6921
Compare
There was a problem hiding this comment.
The core fix looks right to me. Every NamespaceName constructor now validates, new_unchecked is gone, and each dbs/<name> join takes a checked name. I also checked our own callers. The cloud-sync-streamer names (point-of-sale-<kind>-shop-<id>-location-<id>, with the -snapshot, -shadow and -load-test-shadow suffixes) all pass the new rule, so our clients see no change.
The PR also hardens ATTACH. query_analysis.rs:138 now rejects an invalid name at parse time. On base, this path also needed attach rights and an existing namespace with allow_attach, so the check adds defense in depth. It may be worth a line in the PR body.
Important:
- Invalid names get new status codes, and the body shape now depends on the route (inline on
name.rs). - One bad migration row now stops the whole primary, which then crashloops (inline on
scheduler.rs).
Suggestions:
3. parse_snapshot_path mis-parses names that contain : (inline).
4. Rollout preflight for rows that now block startup (inline on restore()).
5. auth/authenticated.rs:35 still unwraps proxy auth (outside the diff, details below).
6. One invalid name in a JWT now rejects the whole token (outside the diff, details below).
Item 5, auth/authenticated.rs:35: from_proxy_grpc_request calls serde_json::from_str::<Authenticated>(s).unwrap(). Authenticated deserializes NamespaceName in the legacy namespace field and in the scope ns sets. JSON with a name that this PR rejects now panics there. Malformed JSON already did on base. The trigger is narrow: an old replica must forward a token, signed with our key, that carries such a name. The impact is one failed gRPC request. The fix matches what you did in admin_shell.rs:
Some(s) => serde_json::from_str::<Authenticated>(s)
.map_err(|_| Status::invalid_argument("invalid x-proxy-authorization"))?,Item 6, auth/authorized.rs:181: Scopes.namespaces is an Option<HashSet<NamespaceName>>, and the legacy id claim is a NamespaceName too. One invalid name in a token now fails the claims decode, and the server returns 401 JwtInvalid for the whole token (jwt.rs:129-134). Before, only that one scope was unusable. Please note this in ADMIN_API.md next to the namespace rules.
While you are in schema/db.rs, a nit. Lines 354-355 still unwrap the stored migration program. This is not new; base had the same unwraps inside the closure. A corrupt row panics, with_conn_async re-panics through .expect, and the server exits. .map_err(Error::CorruptedJobStatus)? and validate_migration(&mut migration)? would at least give a readable error.
33ce0e1 to
fc27a7a
Compare
|
Follow-up on the non-inline points in the review (commit |
fc27a7a to
b02733e
Compare
Why
Fix shop/issues#89265: with namespaces and the admin API enabled, a name such as
..%2F..%2Fvictimor an encoded absolute path could be decoded, joined beneath<data>/dbs, and used to create files outside that directory. Deleting the namespace could then recursively delete unrelated writable files. This PR fixes traversal through the namespace string, rather than broadening the PR to directory-ownership or lifecycle hardening.What changed
NamespaceNameto be a nonempty single path component: reject.,..,/,\, NUL, and:on Windows. Keep existing harmless names with spaces, punctuation, and Unicode; removenew_uncheckedand validate shared-schema names in replicated configuration.--meta-store-destroy-on-errorto erase the metastore. Filesystem recovery fails rather than committing a partial inventory when it finds an invalid directory name; symlinked directories are not followed.:when parsing snapshot filenames.<data>/dbs.Compatibility and rollout
This intentionally changes responses for invalid names: some routes that used to return
404now return400, while invalid create/checkpoint path parameters are rejected by the extractor (400); an invalidshared_schema_namein a create JSON body is rejected (422). Response bodies differ by route. Invalid namespace claims can also invalidate an entire JWT. Existing Shopify cloud-sync-streamer namespace names were checked in the earlier review and remain valid; other consumers should check whether they rely on the old invalid-name behavior. The admin API documentation describes the name rule and the need to inspect/repair legacy persisted names before upgrade. An invalid namespace row or shared-schema reference prevents startup rather than risking data loss; an invalid migration job is isolated rather than taking down the primary. This patch does not add general recovery for unrelated corrupt persisted data.Scope / stack
This PR targets
v0.9.30-shopify-patches. It does not defend against pre-existing filesystem aliases, symlink replacement, or concurrent directory-ownership races; those belong to stacked #36. It also does not address someone directly changing migration rows while a job is running. #36 needs rebasing after this PR's history rewrite. Merge #31 first.Validation / review request
cargo fmt --all -- --check,git diff --check, andRUSTFLAGS='-D warnings --cfg tokio_unstable' cargo check -p libsql-server --tests -qpassed onb02733e21f.Run TestsCI on this head is still pending at the time of this edit. Other completed checks, including Windows checks, passed. Please check the live CI status before approval.Please re-review the current diff, particularly the constructor/persistence boundary, migration prevalidation and isolation, non-destructive recovery, and API/rollout compatibility. Previous review comments refer to commits superseded by the focused history rewrite; human security review is still needed.