kwhitley / itty-router

A little router.
MIT License
1.69k stars 77 forks source link

Issue with CORS when response is cached #242

Closed vicwilliam closed 2 months ago

vicwilliam commented 2 months ago

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

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):

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 commented 2 months ago

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

chris-laplante commented 1 month ago

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 commented 1 week ago

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.