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

Jump to bottom

Html UI report without enable same-origin assets #6661

Closed
4 tasks done
btd opened this issue Oct 7, 2024 · 7 comments · Fixed by #6883
Closed
4 tasks done

Html UI report without enable same-origin assets #6661

btd opened this issue Oct 7, 2024 · 7 comments · Fixed by #6883
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@btd
Copy link

btd commented Oct 7, 2024

Clear and concise description of the problem

As of now, html report has such markup:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <link rel="icon" href="./favicon.ico" sizes="48x48">
    <link rel="icon" href="./favicon.svg" sizes="any" type="image/svg+xml">
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Vitest</title>
    <link rel="preconnect" href="https://fonts.googleapis.com" />
    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
    <link
      href="https://fonts.googleapis.com/css2?family=Readex+Pro:wght@300;400&display=swap"
      rel="stylesheet"
    />
    <script>
      (function () {
        const prefersDark =
          window.matchMedia &&
          window.matchMedia("(prefers-color-scheme: dark)").matches;
        const setting = localStorage.getItem("vueuse-color-scheme") || "auto";
        if (setting === "dark" || (prefersDark && setting !== "light"))
          document.documentElement.classList.toggle("dark", true);
      })();
    </script>
    <!-- !LOAD_METADATA! -->
    <script type="module" crossorigin src="./assets/index-DsWp6aFQ.js"></script>
    <link rel="stylesheet" crossorigin href="./assets/index-D5rK8X7L.css">
  </head>
  <body>
    <div id="app"></div>
  </body>
</html>

It always contain crossorigin attribute, which enables same-origin policy.

When we upload generated report for every url in our artifact storage, it returns every response with SSO checks (location header). And this breaks this report (blank page, FF shows Cross-Origin violation for same origin policy)

Suggested solution

Honestly i do not completely understand why for html report we need crossorigin checks. Nobody share them in internet and it does not look like it make it any way secure as it is completely generated.

To keep it backward compatible as i cannot imagine every possible usecase, can we allow via settings disable crossorign attribute addition?

Alternative

No response

Additional context

No response

Validations
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 8, 2024 •
edited
Loading

When we upload generated report for every url in our artifact storage, it returns every response with SSO checks (location header). And this breaks this report (blank page, FF shows Cross-Origin violation for same origin policy)

I'm not sure what the issue is exactly with that description. Adding crossorigin is the default output of Vite build (which Vitest uses to bundle @vitest/ui package) and Vitest is not doing anything specifically, so I would imagine it's a sensible default for any frontend assets. I don't think Vitest is going to introduce configuration for this.

I think the easiest way around is to strip out crossorigin from index.html during your CI pipeline, like sed -i 's/crossorigin//' index.html seems enough.

@btd
Copy link
Author

btd commented Oct 8, 2024

I would imagine it's a sensible default for any frontend assets

I would disagree there. It is optioned default. It is good for dev and when you do not use CDN (you can see for google CDN we do not have crossorigin).

Also there we are building not webapp, but report. I believe it should be as simple as possible (i based my opinion on usage of istanbul html report and rollup-plugin-visualizer maintainer experience)

Let see if others comment, otherwise i will use sed in build script.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 8, 2024 •
edited
Loading

Admittedly I have no idea about security, so I was just blaming Vite for now 😄 I didn't see any option to control this, so I thought it would be a pretty strong opinion imposed on all Vite use cases.

I also saw some folks putting Vitest's artifacts on the Internet like https://onchainkit.xyz/coverage/ (#6357), so I wasn't entirely sure about your argument. But happy to discuss here and get opinions from more.

@btd
Copy link
Author

btd commented Oct 8, 2024

I also saw some folks putting Vitest's artifacts on the Internet like https://onchainkit.xyz/coverage/

Yea, and it is istanbul report 😄

@hi-ogawa hi-ogawa added the p2-to-be-discussed Enhancement under consideration (priority) label Oct 18, 2024
@hi-ogawa hi-ogawa moved this to P2 - 3 in Team Board Oct 18, 2024
@sheremet-va sheremet-va moved this from P2 - 3 to Approved in Team Board Nov 7, 2024
@sheremet-va sheremet-va added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) enhancement: pending triage labels Nov 7, 2024
@sheremet-va
Copy link
Member

sheremet-va commented Nov 7, 2024 •
edited
Loading

The team decided that we could remove the crossorigin tag during the build. PR is welcome

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Nov 8, 2024

Reading vitejs/vite#6648 again, it looks like this is technically a browser bug. crossorigin on same origin assets should be mostly no-op and cookies should work. Nonetheless, it makes sense to do workaround on Vitest side as discussed.

@hi-ogawa hi-ogawa mentioned this issue Nov 8, 2024
Merged
6 tasks
@btd
Copy link
Author

btd commented Nov 9, 2024

Thank you @hi-ogawa @sheremet-va

@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Team Board
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ui): remove crossorigin attributes for same origin assets hi-ogawa/vitest
3 participants
@btd @hi-ogawa @sheremet-va