From 7867b745eadc906cbe83e20a603eca7fdd096fbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl=20=C3=85stholm?= Date: Wed, 6 Nov 2024 22:15:31 +0100 Subject: [PATCH 1/5] fetch: Disallow `--save` when fetching by file path The purpose of `zig fetch` is to copy a package to the global cache to avoid needing to access the network. `path` entries in a build.zig.zon are for relative paths inside a package. It does not make sense to `--save` a package fetched by a (CWD-relative) file path. Closes #18639 --- src/Package/Fetch.zig | 18 +++++++++++++----- src/main.zig | 22 ++++++++++++++-------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 0d6cf55636b5..85a7292e1422 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -65,6 +65,9 @@ oom_flag: bool, /// If the resource pointed to by the location is not a Git-repository, this /// will be left unchanged. latest_commit: ?git.Oid, +/// If `location` was `path_or_url`, this will be set to indicate +/// whether the CLI input was identified as a file path or a URL. +actual_path_or_url_kind: enum { path, url }, // This field is used by the CLI only, untouched by this file. @@ -344,25 +347,28 @@ pub fn run(f: *Fetch) RunError!void { .remote => |remote| remote, .path_or_url => |path_or_url| { if (fs.cwd().openDir(path_or_url, .{ .iterate = true })) |dir| { + f.actual_path_or_url_kind = .path; var resource: Resource = .{ .dir = dir }; return f.runResource(path_or_url, &resource, null); } else |dir_err| { const file_err = if (dir_err == error.NotDir) e: { if (fs.cwd().openFile(path_or_url, .{})) |file| { + f.actual_path_or_url_kind = .path; var resource: Resource = .{ .file = file }; return f.runResource(path_or_url, &resource, null); } else |err| break :e err; } else dir_err; - const uri = std.Uri.parse(path_or_url) catch |uri_err| { return f.fail(0, try eb.printString( - "'{s}' could not be recognized as a file path ({s}) or an URL ({s})", + "'{s}' could not be recognized as a file path ({s}) or a URL ({s})", .{ path_or_url, @errorName(file_err), @errorName(uri_err) }, )); }; + f.actual_path_or_url_kind = .url; var server_header_buffer: [header_buffer_size]u8 = undefined; + const uri_path = try uri.path.toRawMaybeAlloc(arena); var resource = try f.initResource(uri, &server_header_buffer); - return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, null); + return f.runResource(uri_path, &resource, null); } }, }; @@ -420,11 +426,12 @@ pub fn run(f: *Fetch) RunError!void { const uri = std.Uri.parse(remote.url) catch |err| return f.fail( f.location_tok, - try eb.printString("invalid URI: {s}", .{@errorName(err)}), + try eb.printString("invalid URL: {s}", .{@errorName(err)}), ); var server_header_buffer: [header_buffer_size]u8 = undefined; + const uri_path = try uri.path.toRawMaybeAlloc(arena); var resource = try f.initResource(uri, &server_header_buffer); - return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, remote.hash); + return f.runResource(uri_path, &resource, remote.hash); } pub fn deinit(f: *Fetch) void { @@ -728,6 +735,7 @@ fn queueJobsForDeps(f: *Fetch) RunError!void { .has_build_zig = false, .oom_flag = false, .latest_commit = null, + .actual_path_or_url_kind = undefined, .module = null, }; diff --git a/src/main.zig b/src/main.zig index ed862b03a090..15b8123d0dff 100644 --- a/src/main.zig +++ b/src/main.zig @@ -5207,6 +5207,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void { .has_build_zig = true, .oom_flag = false, .latest_commit = null, + .actual_path_or_url_kind = undefined, .module = build_mod, }; @@ -7107,6 +7108,7 @@ fn cmdFetch( .has_build_zig = false, .oom_flag = false, .latest_commit = null, + .actual_path_or_url_kind = undefined, .module = null, }; @@ -7134,10 +7136,13 @@ fn cmdFetch( return cleanExit(); }, .yes, .exact => |name| name: { + if (fetch.actual_path_or_url_kind == .path) { + fatal("options '--save' and `--save-exact` cannot be used when fetching by file path", .{}); + } + if (name) |n| break :name n; - const fetched_manifest = fetch.manifest orelse - fatal("unable to determine name; fetched package has no build.zig.zon file", .{}); - break :name fetched_manifest.name; + if (fetch.manifest) |fm| break :name fm.name; + fatal("unable to determine name; fetched package has no build.zig.zon file", .{}); }, }; @@ -7163,7 +7168,8 @@ fn cmdFetch( var fixups: Ast.Fixups = .{}; defer fixups.deinit(gpa); - var saved_path_or_url = path_or_url; + var saved_url = path_or_url; + assert(fetch.actual_path_or_url_kind == .url); if (fetch.latest_commit) |latest_commit| resolved: { const latest_commit_hex = try std.fmt.allocPrint(arena, "{}", .{latest_commit}); @@ -7188,7 +7194,7 @@ fn cmdFetch( uri.fragment = .{ .raw = latest_commit_hex }; switch (save) { - .yes => saved_path_or_url = try std.fmt.allocPrint(arena, "{}", .{uri}), + .yes => saved_url = try std.fmt.allocPrint(arena, "{}", .{uri}), .no, .exact => {}, // keep the original URL } } @@ -7199,7 +7205,7 @@ fn cmdFetch( \\ .hash = "{}", \\ }} , .{ - std.zig.fmtEscapes(saved_path_or_url), + std.zig.fmtEscapes(saved_url), std.zig.fmtEscapes(&hex_digest), }); @@ -7219,7 +7225,7 @@ fn cmdFetch( if (dep.hash) |h| { switch (dep.location) { .url => |u| { - if (mem.eql(u8, h, &hex_digest) and mem.eql(u8, u, saved_path_or_url)) { + if (mem.eql(u8, h, &hex_digest) and mem.eql(u8, u, saved_url)) { std.log.info("existing dependency named '{s}' is up-to-date", .{name}); process.exit(0); } @@ -7231,7 +7237,7 @@ fn cmdFetch( const location_replace = try std.fmt.allocPrint( arena, "\"{}\"", - .{std.zig.fmtEscapes(saved_path_or_url)}, + .{std.zig.fmtEscapes(saved_url)}, ); const hash_replace = try std.fmt.allocPrint( arena, From e353694e4aa7c2973bb2f301556d319396595243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl=20=C3=85stholm?= Date: Wed, 6 Nov 2024 22:22:20 +0100 Subject: [PATCH 2/5] fetch: `--save` over existing `path` entry correctly Closes #21690 --- src/main.zig | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/main.zig b/src/main.zig index 15b8123d0dff..615be9d61ee0 100644 --- a/src/main.zig +++ b/src/main.zig @@ -7234,20 +7234,23 @@ fn cmdFetch( } } - const location_replace = try std.fmt.allocPrint( - arena, - "\"{}\"", - .{std.zig.fmtEscapes(saved_url)}, - ); - const hash_replace = try std.fmt.allocPrint( - arena, - "\"{}\"", - .{std.zig.fmtEscapes(&hex_digest)}, - ); - warn("overwriting existing dependency named '{s}'", .{name}); - try fixups.replace_nodes_with_string.put(gpa, dep.location_node, location_replace); - try fixups.replace_nodes_with_string.put(gpa, dep.hash_node, hash_replace); + if (dep.location == .url) { + const location_replace = try std.fmt.allocPrint( + arena, + "\"{}\"", + .{std.zig.fmtEscapes(saved_url)}, + ); + const hash_replace = try std.fmt.allocPrint( + arena, + "\"{}\"", + .{std.zig.fmtEscapes(&hex_digest)}, + ); + try fixups.replace_nodes_with_string.put(gpa, dep.location_node, location_replace); + try fixups.replace_nodes_with_string.put(gpa, dep.hash_node, hash_replace); + } else { + try fixups.replace_nodes_with_string.put(gpa, dep.node, new_node_init); + } } else if (manifest.dependencies.count() > 0) { // Add fixup for adding another dependency. const deps = manifest.dependencies.values(); From 2ee8bdaf75b3e4b1a8755a2dedbd92d0f14668e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl=20=C3=85stholm?= Date: Wed, 6 Nov 2024 22:24:25 +0100 Subject: [PATCH 3/5] fetch: Fix fetching by file URL This did not work at all previously because the `parent_package_root` field was `undefined`. This fix additionally validates that the file URL is a conformant local URL and decodes absolute Windows paths correctly. See RFC 8089 and https://url.spec.whatwg.org/ for relevant specs. --- src/Package/Fetch.zig | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 85a7292e1422..3e6e3ccdb922 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -922,10 +922,45 @@ fn initResource(f: *Fetch, uri: std.Uri, server_header_buffer: []u8) RunError!Re const eb = &f.error_bundle; if (ascii.eqlIgnoreCase(uri.scheme, "file")) { - const path = try uri.path.toRawMaybeAlloc(arena); - return .{ .file = f.parent_package_root.openFile(path, .{}) catch |err| { - return f.fail(f.location_tok, try eb.printString("unable to open '{}{s}': {s}", .{ - f.parent_package_root, path, @errorName(err), + var path = try uri.path.toRawMaybeAlloc(arena); + + if (uri.user != null or + uri.password != null or + uri.port != null or + path.len == 0 or path[0] != '/' or + uri.query != null) + { + return f.fail(f.location_tok, try eb.printString( + "invalid file URL: {}", + .{uri}, + )); + } + if (uri.host) |host| { + const raw_host = try host.toRawMaybeAlloc(arena); + if (raw_host.len != 0 and !ascii.eqlIgnoreCase(raw_host, "localhost")) { + return f.fail(f.location_tok, try eb.printString( + "unable to process non-local file URL: {}", + .{uri}, + )); + } + } + + // On Windows, an absolute path starting with a drive letter gains a leading + // slash when encoded as a file URL that must be stripped when decoding. + if (native_os == .windows and + path.len >= "/C:/".len and + std.ascii.isAlphabetic(path[1]) and + path[2] == ':' and + path[3] == '/') + { + path = path[1..]; + } + + assert(std.fs.path.isAbsolute(path)); + + return .{ .file = fs.cwd().openFile(path, .{}) catch |err| { + return f.fail(f.location_tok, try eb.printString("unable to open '{s}': {s}", .{ + path, @errorName(err), })); } }; } From 33d2a58c43e41b40d0487922bba1e2e4a684eea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl=20=C3=85stholm?= Date: Wed, 6 Nov 2024 22:28:57 +0100 Subject: [PATCH 4/5] fetch: Remove some redundancy from usage text --- src/main.zig | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main.zig b/src/main.zig index 615be9d61ee0..c3bb80722723 100644 --- a/src/main.zig +++ b/src/main.zig @@ -6989,10 +6989,8 @@ const usage_fetch = \\ -h, --help Print this help and exit \\ --global-cache-dir [path] Override path to global Zig cache directory \\ --debug-hash Print verbose hash information to stdout - \\ --save Add the fetched package to build.zig.zon - \\ --save=[name] Add the fetched package to build.zig.zon as name - \\ --save-exact Add the fetched package to build.zig.zon, storing the URL verbatim - \\ --save-exact=[name] Add the fetched package to build.zig.zon as name, storing the URL verbatim + \\ --save[=name] Add the fetched package to build.zig.zon + \\ --save-exact[=name] Add the fetched package to build.zig.zon, storing the URL verbatim \\ ; From 9da3e104a63b8bf8b13fa3370985ed97d05a0ca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl=20=C3=85stholm?= Date: Sun, 15 Dec 2024 16:01:39 +0100 Subject: [PATCH 5/5] fetch: Update tests --- src/Package/Fetch.zig | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 3e6e3ccdb922..64feab440c8f 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -2078,6 +2078,7 @@ test "zip" { defer fb.deinit(); try fetch.run(); + try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind); var out = try fb.packageDir(); defer out.close(); @@ -2111,6 +2112,7 @@ test "zip with one root folder" { defer fb.deinit(); try fetch.run(); + try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind); var out = try fb.packageDir(); defer out.close(); @@ -2185,6 +2187,7 @@ test "tarball with excluded duplicate paths" { "12200bafe035cbb453dd717741b66e9f9d1e6c674069d06121dafa1b2e62eb6b22da", &hex_digest, ); + try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind); const expected_files: []const []const u8 = &.{ "build.zig", @@ -2229,6 +2232,7 @@ test "tarball without root folder" { "12209f939bfdcb8b501a61bb4a43124dfa1b2848adc60eec1e4624c560357562b793", &hex_digest, ); + try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind); const expected_files: []const []const u8 = &.{ "build.zig", @@ -2267,6 +2271,7 @@ test "set executable bit based on file content" { "1220fecb4c06a9da8673c87fe8810e15785f1699212f01728eadce094d21effeeef3", &Manifest.hexDigest(fetch.actual_hash), ); + try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind); var out = try fb.packageDir(); defer out.close(); @@ -2351,6 +2356,7 @@ const TestFetchBuilder = struct { .has_build_zig = false, .oom_flag = false, .latest_commit = null, + .actual_path_or_url_kind = undefined, .module = null, };