fix: Resolved failure to find arbitrarily-named snapshot files when using expect(...).toMatchFileSnapshot() matcher.

[zmullett]

Description

The toMatchFileSnapshot() matcher was earlier failing to return file content via the birpc later due to the resolveRaw file path resolution not adding to a cache of resolved paths. In this PR I've removed said cache, as it ostensibly wasn't used for performance improvements and was simply insisting that the resolution step was performed prior to any snapshot reads or writes.

While debugging an issue with browser code, I discovered the npm scripts for the tests weren't running (on a Mac environment). These commits correct that.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• [ new ] It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• [ ok ] Ideally, include a test that fails without this PR but passes with it.
• [ ok ] Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

• [ unrelated tests failing locally ] Run the tests with pnpm test:ci.

Documentation

• [ n/a ] If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

• [ ok ] Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[netlify]

✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name	Link
🔨 Latest commit	255bdb1
🔍 Latest deploy log	https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/6596950a52b1e30008a3cb40

[sheremet-va]

This is done not for performance reasons, but for security reasons. You should be able to access only snapshots, not just any file.

[zmullett]

Thanks for the insight @sheremet-va. Here I was attempting to fix towards the behavior documented @ https://vitest.dev/guide/snapshot.html#file-snapshots. Could you further elaborate on the potential risks? Perhaps we can arrive at a solution that supports arbitrary yet legitimate text/binary files as snapshots, but mitigates the security issues.

[zmullett]

I've been thinking about this a bit. Assuming the threat is not from the vitest repo but rather a repo that uses vitest and an unaware user executes malicious code, then @sheremet-va you're right that without guardrails, vitest could be party to exfiltration or manipulation of files anywhere in the filesystem.

My goal is to support snapshot files that have their original extension and content, rather than the .snap suffix and being encapsulated in another container. This is to allow for better DX in working with media files like images, where for example a .png file will render in an IDE or file viewer, and editor files will more readily open them.

At first I thought that extra configuration override might be the best; that by default the rules are as they were; 1. the file must be in the __snapshots__ directory (or something else if SnapshotStateOptions.resolveSnapshotPath is specified) and 2. be suffixed with .snap, but via config all this could be changed. The threat however is that the unaware user won't necessarily encounter and notice a malicious setting override.

So then I concluded that it be sufficiently safe if resolved file paths were within the project's root directory, irrespective of file extension. The threat of exfiltration and manipulation are confined to the project. A threat from a malicious commit in that project repo is mitigated.

Happy to hear your thoughts. If you think it's a good change then I can proceed with it and include security tests.

[hi-ogawa]

I'm not sure if it's intentionally left out when implementing file system restriction #3956, but from the PR introducing toMatchFileSnapshot #3116, it looks like it's intended to be used on browser mode as well.

Wouldn't it be okay to simply add SnapshotManager.resolveRawPath() to resolvedPaths like this?

  resolveRawPath(testPath: string, rawPath: string) {
    const path = isAbsolute(rawPath)
      ? rawPath
      : resolve(dirname(testPath), rawPath)
    this.resolvedPaths.add(path)  /// <-- whitelist resolved path
    return path
  }

while currently it's not added there:

vitest/packages/snapshot/src/manager.ts

Lines 37 to 41 in 70e8a38

	resolveRawPath(testPath: string, rawPath: string) {
	return isAbsolute(rawPath)
	? rawPath
	: resolve(dirname(testPath), rawPath)
	}

[zmullett]

Hi @hi-ogawa. Thanks for the references to these PRs. IIUC adding arbitrary paths to resolvedPaths would defeat its purpose in relation to #3956. IMO it would be more robust and intuitive to remove the resolvedPaths cache and check against the security logic every time. WDYT?

[hi-ogawa]

IIUC adding arbitrary paths to resolvedPaths would defeat its purpose in relation to #3956.

Yeah, I get that as well, but I'm not familiar with what's exactly intended in file system restriction #3956, so I'll wait for @sheremet-va 's response.

I just wanted to share the idea since I thought it could a simple oversight and resolveRawPath() case was forgotten.

[sheremet-va]

The current solution is just a small patch that works well for regular snapshots. The use case with toMatchFileSnapshot was not taken into account. This behavior mirrors fs.allow from Vite config.

Basically, for browser mode Vitest starts a Vite server, and anyone with access to it would be able to read any file on the file system which is a security concern. Ideally, we just need to follow the same rules here:

https://github.com/vitejs/vite/blob/56ae92c33cba6c86ab5819877c19b9ea39f7121b/packages/vite/src/node/server/middlewares/static.ts#L208

This method is actually exported from vite and we can safely use it here.

[zmullett]

@sheremet-va, I spent some time trying to shoehorn isFileServingAllowed() into readSnapshotFile(). Rather than simply returning null if file access is disallowed, I believe an error explaining the problem should be thrown and propagated.

I hit a roadblock in that for some reason an Error's content is not propagated through the RPC layer, i.e. a caught Error on the client side is missing the service-side message that would be crucial for the user to debug.

I convinced myself that birpc isn't the problem; I added an async RPC method that throws to test/bob.ts:

export async function throwError() {
  throw new Error('my error message')
}

... and demonstrated that the Error propagates OK through birpc with:

  const channel = new MessageChannel()

  const bob = createBirpc<AliceFunctions, BobFunctions>(
    { ...Bob },
    {
      post: data => channel.port1.postMessage(data),
      on: data => channel.port1.on('message', data),
    },
  )

  const alice = createBirpc<BobFunctions, AliceFunctions>(
    { ...Alice },
    {
      post: data => channel.port2.postMessage(data),
      on: data => channel.port2.on('message', data),
    },
  )

  await expect(alice.throwError())
    .rejects.toThrowError('my error message')

There's additional machinery in vitest's application of birpc, but nothing I've noticed that would cause an Error not to propagate correctly. Any ideas? Tagging birpc maintainer @antfu too!

[sheremet-va]

Error instances should be serialized: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

[zmullett]

Error instances should be serialized: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

From that documentation I interpret that Error should be serialised fine (which I confirmed works OK in the pure birpc setting). However from my observation it's not in the vitest setup. The client recognises a promise has been rejected, but seemingly has no detail. Any clues on what could be causing it?

[sheremet-va]

Error instances should be serialized: developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

From that documentation I interpret that Error should be serialised fine (which I confirmed works OK in the pure birpc setting). However from my observation it's not in the vitest setup. The client recognises a promise has been rejected, but seemingly has no detail. Any clues on what could be causing it?

Not sure, but this has been a common problem with errors. We have processError function that serializes it. But since you know what the error is, you can just throw an object:

const error = new Error('Cannot access file')
throw { message: error.message, stack: error.stack }