fix(cors): avoid setting Access-Control-Allow-Origin if there is no matching origin

[uki00a]

The author should do the following, if applicable

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code
• Add TSDoc/JSDoc to document the code

Even if the Origin header of a request does not match a origin specified by origin option, CORS middleware always sets Access-Control-Allow-Origin header. I don't see this as a serious problem, but in some cases it may unintentionally expose a server setting.

References

• https://github.com/expressjs/cors/blob/v2.8.5/test/test.js#L251-L262 - cors package seems not to set the Access-Control-Allow-Origin if the Origin header of a request does not match allowed origins
• https://fetch.spec.whatwg.org/#cors-check - A CORS check returns failure if the Access-Control-Allow-Origin header is missing
  • Currently, CORS middleware returns a origin specified by origin option as the Access-Control-Allow-Origin if the option does not match the Origin header of a request, so changes in this PR would have no particular negative impact

[uki00a]

This was originally added in #1129, but seems to have been lost at some point 👀

[yusukebe]

Ah, one of them is not necessary. You can remove one.

[uki00a]

Thanks for the review! 🙏

I believe the addition of the following code in #1129 is to verify that duplicate headers are not set:

  app.use(
    '/api6/*',
    cors({
      origin: 'http://example.com',
    })
  )
  app.use(
    '/api6/*',
    cors({
      origin: 'http://example.com',
    })
  )

If I remove one of them, should I also remove the following test?

hono/src/middleware/cors/index.test.ts

Lines 171 to 175 in 3311664

	it('Should not return duplicate header values', async () => {
	const res = await app.request('http://localhost/api6/abc')
	expect(res.headers.get('Access-Control-Allow-Origin')).toBe('http://example.com')
	})

[yusukebe]

@uki00a

Got it! Let's leave these lines. Thank you for the explanation!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (9986b47) to head (5bf6c62).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3510      +/-   ##
==========================================
- Coverage   95.58%   95.57%   -0.01%     
==========================================
  Files         155      155              
  Lines        9357     9361       +4     
  Branches     2749     2782      +33     
==========================================
+ Hits         8944     8947       +3     
- Misses        413      414       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yusukebe]

Hi @uki00a

Thanks! I've left one comment. Please check it!

[yusukebe]

LGTM!

[yusukebe]

@uki00a

Looks good! I'll merge it and release the new version soon (maybe tomorrow). Thank you for your contribution!