Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: extract whole archive instead of subset #3604

Merged
merged 1 commit into from
Dec 6, 2023
Merged

fix: extract whole archive instead of subset #3604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions yarn-project/noir-compiler/src/compile/noir/dependencies/github-dependency-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ export class GithubDependencyResolver implements NoirDependencyResolver {

async #extractZip(dependency: NoirGitDependencyConfig, archivePath: string): Promise<string> {
const gitUrl = new URL(dependency.git);
// extract the archive to this location
const extractLocation = join('libs', safeFilename(gitUrl.pathname + '@' + (dependency.tag ?? 'HEAD')));
const tmpExtractLocation = extractLocation + '.tmp';

// where we expect to find this package after extraction
// it might already exist if the archive got unzipped previously
const packagePath = join(extractLocation, dependency.directory ?? '');

if (this.#fm.hasFileSync(packagePath)) {
Expand All @@ -82,24 +85,21 @@ export class GithubDependencyResolver implements NoirDependencyResolver {

const { entries } = await unzip(this.#fm.readFileSync(archivePath));

// extract to a temporary directory, then move it to the final location
// TODO empty the temp directory first
const tmpExtractLocation = extractLocation + '.tmp';
for (const entry of Object.values(entries)) {
if (entry.isDirectory) {
continue;
}

// remove the first path segment, because it'll be the archive name
const name = stripSegments(entry.name, 1);
if (dependency.directory && !name.startsWith(dependency.directory)) {
continue;
}
const path = join(tmpExtractLocation, name);
await this.#fm.writeFile(path, (await entry.blob()).stream());
}

if (dependency.directory) {
this.#fm.moveFileSync(join(tmpExtractLocation, dependency.directory), packagePath);
} else {
this.#fm.moveFileSync(tmpExtractLocation, packagePath);
}
this.#fm.moveFileSync(tmpExtractLocation, extractLocation);

return packagePath;
}
Expand Down
8 changes: 5 additions & 3 deletions yarn-project/noir-compiler/src/compile/noir/dependencies/local-dependency-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { NoirDependencyConfig } from '@aztec/foundation/noir';

import { resolve } from 'path';
import { isAbsolute, join } from 'path';

import { FileManager } from '../file-manager/file-manager.js';
import { NoirPackage } from '../package.js';
Expand All @@ -16,12 +16,14 @@ export class LocalDependencyResolver implements NoirDependencyResolver {
this.#fm = fm;
}

resolveDependency(pkg: NoirPackage, config: NoirDependencyConfig): Promise<NoirDependency | null> {
resolveDependency(parent: NoirPackage, config: NoirDependencyConfig): Promise<NoirDependency | null> {
if ('path' in config) {
const parentPath = parent.getPackagePath();
const dependencyPath = isAbsolute(config.path) ? config.path : join(parentPath, config.path);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda worried about someone putting /etc/passwd in their dependency list. Maybe we make it only accept relative deps?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveDependency is only used for loading stuff, right? We're not writing to the resolved directory at any time? If so, I think it's not that bad, since at most it'd exfiltrate /etc/passwd to the noir compiler, and won't overwrite anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's how it works. The Github resolver does write to the disk (to unzip the archive), but writing to the disk is strictly checked (can only write to a given subfolder).

return Promise.resolve({
// unknown version, Nargo.toml doesn't have a version field
version: undefined,
package: NoirPackage.open(resolve(pkg.getPackagePath(), config.path), this.#fm),
package: NoirPackage.open(dependencyPath, this.#fm),
});
} else {
return Promise.resolve(null);
Expand Down