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.

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

test: add cache testing suite #3842

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Nov 18, 2024

This adds the test suite from https://cache-tests.fyi to test the cache interceptor.

Note these aren't compliance tests, we don't need to pass 100% of these. Arguably we shouldn't since some of the tests aren't even applicable to Undici (i.e. cdn-specific, we can skip these though). However, I do think we should try to rely on these in CI.

Current issues:

cc @mcollina @ronag

return v.replace(/\n/g, '<br/>')
}
function stripHtml (v) {
return v.replace(/<script.*?<\/script>|<!--.*?-->|<style.*?<\/style>|<.*?>/g, '')

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High test

This string may still contain
<script
, which may cause an HTML element injection vulnerability.
This string may still contain
<!--
, which may cause an HTML element injection vulnerability.
This string may still contain
<style
, which may cause an HTML element injection vulnerability.
This string may still contain
<script
, which may cause an HTML element injection vulnerability.
export const testResults = {}

export async function makeTest (test) {
const uuid = utils.token()

Check failure

Code scanning / CodeQL

Insecure randomness High test

This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

Note these aren't compliance tests, we don't need to pass 100% of these. Arguably we shouldn't since some of the tests aren't even applicable to Undici.

If these tests are targeting fetch, and fetch is a browser spec, why wouldn't some of the tests be applicable?

test/cache-interceptor/cache-tests.mjs Outdated Show resolved Hide resolved
test/cache-interceptor/cache-tests.mjs Outdated Show resolved Hide resolved
@flakey5
Copy link
Member Author

flakey5 commented Nov 19, 2024

If these tests are targeting fetch

They aren't targeting fetch. It has tests for every part of the stack that can implement caching. They're written in js w/ fetch so that they can be ran in browsers, but browsers aren't the only thing the tests cover. A lot of the behaviors do overlap, however, there are still things specific to servers or cdns.

There are also tests for things that rfc9111 obsoletes (such as using the pragma header for caching directives)

@KhafraDev
Copy link
Member

KhafraDev commented Nov 19, 2024

They aren't targeting fetch

What I mean is that the tests explicitly use fetch and it tests browser caching behavior (every http request made through browsers is done through fetch, at least in the specs). Anything that doesn't work would seem like a bug to me.

In the fetch spec, the http cache is built a layer above fetch, whereas in undici it's a layer below. For example, in multiple parts of the spec we are meant to query the cache directly (see http-network-or-cache-fetch). There are multiple situations where headers may get added to requests/responses, or the internal request is modified in some way that will cause bugs or deviate from browser behavior.

It's not a huge issue, as it's opt-in and not built specifically for fetch, but it does feel weird to not implement the caching steps in the spec, but still use fetch to test it.

A lot of the behaviors do overlap, however, there are still things specific to servers or cdns.

Is there anything specific?

There are also tests for things that rfc9111 obsoletes (such as using the pragma header for caching directives)

Is lack of pragma support intentional, because it's obsolete? Is there no plan on adding support for it?

@flakey5
Copy link
Member Author

flakey5 commented Nov 19, 2024

Is there anything specific?

They're mostly tests for CDN-Cache-Control

Is lack of pragma support intentional, because it's obsolete? Is there no plan on adding support for it?

It was intentionally left out, right now there aren't any plans to add it.

@flakey5
Copy link
Member Author

flakey5 commented Nov 19, 2024

So it does look like we can run tests that only apply to browsers, which should skip the cdn tests

@mcollina
Copy link
Member

Why did you lift the entirety of the source code of that project? What's the long term maintainability strategy?

Either:

  • we can use the npm module they publish, so just add a dependency
  • we vendor all their repo, but you also add a way to automatically update it
  • we only lift the test cases, essentially making future updates on these harder

Anyhow, I would recommend not to use fetch() for this, because it'd make things harder long term. Use undici.request().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants