cache-control always includes private even if fetch has no credentials

[jfranciscosousa]

Describe the bug
I'm filing this in as a bug simply because it's either a bug in the project or misleading documentation.

So when using the maxage attribute on the load function, SvelteKit builds the cache-control header into that page. According to the documentation: The resulting cache header will include private if user data was involved in rendering the page (either via session, or because a credentialled fetch was made in a load function).

My goal is to just cache the page that uses a fetch request with no user data. Seems that SvelteKit is forcing the private in cache-control just by using fetch.

	export async function load({ fetch }) {
		const res = await fetch('/covidData.json');

		if (res.ok) {
			const data = await res.json();

			return {
				props: {
					data: await new CovidData(data[0], data[1])
				},
				maxage: 3600
			};
		}
	}

This page will always include private in the response header.

If I don't use fetch from the load function, then it includes public.

Logs
No logs for this, but here is a screenshot of the browser response header:

To Reproduce
Just build out a simple page that makes a fetch request, then pass in a maxage to the load response. Check in the browser's dev tools that the response cache-control headers will always include private.

This is happening both on dev(svelte-kit dev) and prod (svelte-kit start).

Expected behavior
If fetch is being used without custom headers then perhaps we should not set the cache-control to private. Ideally, private should only be included if we pass in parameters to fetch, but I see that being a brittle solution. Maybe we could override it somehow, with a private: false on the load response? Or just being able to return fully custom headers on load.

Information about your SvelteKit Installation:

• The output of npx envinfo --system --npmPackages svelte,@sveltejs/kit --binaries --browsers

  System:
    OS: Linux 5.8 Pop!_OS 20.10
    CPU: (12) x64 AMD Ryzen 5 1600X Six-Core Processor
    Memory: 7.84 GB / 15.63 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 14.15.3 - ~/.asdf/installs/nodejs/14.15.3/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.9 - ~/.asdf/installs/nodejs/14.15.3/bin/npm
  Browsers:
    Firefox: 86.0.1
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.64 
    svelte: ^3.29.0 => 3.36.0 

• Browser
  Brave

• Svelte Kit Adapter
  Node, but will switch to Netlify before going into prod.

Severity
I'm just trying out SvelteKit for fun, I think it's an awesome project. But this bug basically makes the usage of this project a bit hard for me, at work and in freelancing scenarios. Without being able to cache pages that make "public" fetch requests on the load function, I'm basically forcing myself to consistently pay for serverless executions, instead of caching said page for the specified maxage. This is because no cloud provider will cache pages with private in the cache-control.

Additional context
The use case for this is a classically SSR page that contains the latest updates to Covid numbers in Portugal. I want the page to be server-rendered but the results cached by an hour so I serve that from the CDN instead of hitting the serverless function on Netlify over and over.

[jfranciscosousa]

Well well, was looking through the project code, and realized a non-credentialed request actually means, which makes sense with the regular fetch API.

kit/packages/kit/src/runtime/server/page.js

Lines 89 to 91 in a59fbd0

	if (opts.credentials !== 'omit') {
	uses_credentials = true;
	}

If I change my fetch call on load to:

await fetch('/covidData.json', { credentials: 'omit' });

The cache-control is then public.

So I guess this is working as expected, but we could use some more clarity in the docs in my opinion.

[benmccann]

I wonder if there's an opportunity to be slightly smarter here. Perhaps the check should be if (has_cookies && opts.credentials !== 'omit') instead of just if (opts.credentials !== 'omit')

[jfranciscosousa]

I think there are a lot of edge cases if this is to be inferred automatically. Headers are often used for authentication for example. post requests can contain sensitive information. We can just check for all of them though, but I fear for some edge case to be missed and people being unaware of that.

IMO, I think being able to just return private: false/true from the load function would probably be better and more intuitive, rather than setting the credentials on the fetch request. And by default let fetch requests set that cache-control to private.

[tbillington]

I'm getting private even with credentials: 'omit'. Running with adapter-node

[tbillington]

Okay... I figured out why.

My code was like this, destructing the args to Load as per the example

export const load: Load = async ({ page, fetch, session, context }) => {

However, pulling in session (even though it was unused) I think triggered this check

kit/packages/kit/src/runtime/server/page.js

Lines 270 to 273 in 42dc6b3

	get session() {
	uses_credentials = true;
	return $session;
	},

.

I've fixed it by only destructuring page & fetch but it was a bit of a headscratcher for a while.

[istarkov]

The other issue that credentials: 'omit' breaks cloudflare pages #3728
So serverside fetch can't use omit at all, making impossible to set cache-control public.

[istarkov]

Also seems like here

kit/packages/kit/src/runtime/server/page/render.js

Lines 133 to 135 in 27a9888

	const unsubscribe = session.subscribe(() => {
	if (session_tracking_active) is_private = true;
	});

is an error

IMO Framework need to instercept that subscribe was called from rendered page. And not that session was changed during rendering.

Should be something like

const tmp = session.subscribe;
session.subscribe = (v) => {
  tmp(v);
  is_private = true;
}

[Rich-Harris]

I've opened #4549 for more control over public vs private. The Cloudflare stuff should be fixed by #4548. The last item in this thread that needs attention (AFAICT) is the session tracking stuff, which was a rookie error on my end — have opened #4550, which will close this issue once merged.