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

Issue with CORS when response is cached #242

Closed
vicwilliam opened this issue Apr 28, 2024 · 3 comments
Closed

Issue with CORS when response is cached #242

vicwilliam opened this issue Apr 28, 2024 · 3 comments

Comments

@vicwilliam
Copy link

Describe the Issue

Cached responses throws error when corsify is in use. Non cached responses work well.

{
  "status": 500,
  "error": "Can't modify immutable headers."
}

Example Router Code

import { AutoRouter, cors, json } from 'itty-router';

const { preflight, corsify } = cors();

const router = AutoRouter({
	before: [preflight], // add preflight upstream
	finally: [corsify], // and corsify downstream
});
router.get('/hello', async (request, env, context) => {
	const cache = caches.default;
	const cachedResponse = await cache.match(request.url);

	if (cachedResponse) {
		console.log('Cache hit');
		return cachedResponse;
	}
	console.log('Cache miss');
	const response = new Response(JSON.stringify({ "time": Date.now() }), {
		status: 200,
		headers: { 'Content-Type': 'application/json' },
	});
	context.waitUntil(cache.put(request.url, response.clone()));

	return response;
});
export default { ...router };

Request Details

  • Method: GET
  • URL: /hello
  • Request Body: none
  • Request Headers: none

Steps to Reproduce

Steps to reproduce the behavior:

  1. Run worker in cloudflare
  2. Send request to /hello, first response is not cached
  3. Send another request, this time it should have a cached response. If it has, there will be an error.

Expected Behavior

Expected to have a cached response with no errors and cors applied when necessary, same behaviour from router-itty 4 with corsify

Actual Behavior

An error is thrown if the request is cached and the request fails.

Environment (please complete the following information):

  • Environment: Cloudflare
  • itty-router Version: 5.0.16
# cloudflare variable
main = "src/index.ts"
compatibility_date = "2024-04-23"
compatibility_flags = ["nodejs_compat"]  # Auto added by wrangler init

Workaround

A workaround. Not calling corsify if the response is cached. To check if it's a cached response, just see if a header not added by you is present in the cached response.

function customCors(response, request){
    if(response.headers.has("server"))
        return response;
    debugger
    return corsify(response.clone());
}

const router = AutoRouter<IRequest, CF>({
    base: "/",
    before: [preflight],
    catch: onError,
    finally: [customCors],
});
@kwhitley
Copy link
Owner

Love the detailed writeup! Super helpful - should be fixed in 5.0.17 :)

@chris-laplante
Copy link

If anyone is still having issues with this in 5.0.17, I found that I had to bypass corsify for redirect responses. I'm not quite sure why, but it works now. Here is my code:

const { preflight, corsify } = cors();

type CFArgs = [Env, ExecutionContext];

// Do not corsify for redirections
const wrappedCorsify = (response: Response, request?: Request): Response => {
    if (response?.status === 302) {
        return response;
    }

    return corsify(response, request);
};

const router = AutoRouter<IRequest, CFArgs>({
    before: [preflight],
    finally: [wrappedCorsify]
});

router
    .get('/authorize', (request, env) => {
            return Response.redirect(
                `https://github.com/login/oauth/authorize?client_id=${env.GITHUB_OAUTH_APP_CLIENT_ID}&scope=gist`,
                302
            );
        }
    );

@nicolasvienot
Copy link

nicolasvienot commented Jun 27, 2024

Hello, we're having the same issue in Cloudflare for almost every response with itty-router 5.0.17. Using corsify() results on a "Can't modify immutable headers." error every time a header is modified by corsify.

After investigating, the solution that worked for us was to create a new response in corsify(): new Response(...) instead of modifying the cloned response.

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

No branches or pull requests

4 participants