fix(binding-http): include CORS headers for 404 responses#1496
fix(binding-http): include CORS headers for 404 responses#1496Pranav-0440 wants to merge 1 commit intoeclipse-thingweb:masterfrom
Conversation
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1495 by adding CORS headers to 404 responses returned by the defaultRoute handler. When an Origin header is present in a request to an unknown route, the server now reflects the origin in the Access-Control-Allow-Origin response header along with a Vary: Origin header.
Changes:
- Updated defaultRoute in http-server.ts to include CORS headers (Access-Control-Allow-Origin and Vary: Origin) when an Origin header is present
- Added two tests to http-server-cors-test.ts: one attempting to verify 404 CORS behavior and another testing preflight with custom headers
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/binding-http/src/http-server.ts | Added CORS header logic to defaultRoute for 404 responses |
| packages/binding-http/test/http-server-cors-test.ts | Added two tests, one for 404 CORS (though implementation is flawed) and one for preflight with custom headers |
| @test async "should handle preflight with custom headers"() { | ||
| this.thing = new ExposedThing(this.servient, { | ||
| title: "TestThingCustomHeaders", | ||
| properties: { | ||
| test: { | ||
| type: "string", | ||
| forms: [], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await this.httpServer.expose(this.thing); | ||
|
|
||
| const uri = `http://localhost:${this.httpServer.getPort()}/testthingcustomheaders/properties/test`; | ||
|
|
||
| const response = await fetch(uri, { | ||
| method: "OPTIONS", | ||
| headers: { | ||
| Origin: "http://example.com", | ||
| "Access-Control-Request-Method": "GET", | ||
| "Access-Control-Request-Headers": "Authorization, Content-Type", | ||
| }, | ||
| }); | ||
|
|
||
| expect(response.status).to.equal(200); | ||
| expect(response.headers.get("Access-Control-Allow-Headers")).to.contain("authorization"); | ||
| } |
There was a problem hiding this comment.
This test appears to be testing preflight handling with custom headers, which is unrelated to the PR's stated purpose of fixing 404 CORS behavior. While the test itself is valid and tests existing functionality, it doesn't relate to the changes in defaultRoute or 404 responses. Consider whether this test should be in a separate PR focused on preflight testing, or if it was added intentionally to improve overall CORS test coverage.
| @test async "should include CORS headers on 405 response"() { | ||
| this.thing = new ExposedThing(this.servient, { | ||
| title: "TestThing405", | ||
| properties: { | ||
| test: { | ||
| type: "string", | ||
| forms: [], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await this.httpServer.expose(this.thing); | ||
|
|
||
| const uri = `http://localhost:${this.httpServer.getPort()}/testthing405/properties/test`; | ||
|
|
||
| const response = await fetch(uri, { | ||
| method: "DELETE", // not allowed | ||
| headers: { | ||
| Origin: "http://example.com", | ||
| }, | ||
| }); | ||
|
|
||
| expect(response.status).to.equal(404); | ||
|
|
||
| // Ensure CORS header exists even on 405 | ||
| expect(response.headers.get("Access-Control-Allow-Origin")).to.exist; | ||
| } |
There was a problem hiding this comment.
This test has several issues that need to be addressed:
-
The test name and comment reference "405 response" but the test expects status 404. This is misleading because the test sends DELETE to an existing property route, which triggers defaultRoute (404) rather than a 405 response from respondUnallowedMethod.
-
The test doesn't adequately verify CORS behavior for 404 responses on truly unknown routes as described in the PR description and issue binding-http: 404 responses do not include CORS headers #1495. To properly test 404 CORS handling for the defaultRoute, use a completely non-existent route like "/nonexistentresource" instead of an existing route with an unsupported method.
-
The assertion at line 277 only checks that the CORS header exists but doesn't verify its value. It should assert that the header equals the origin sent in the request: expect(response.headers.get("Access-Control-Allow-Origin")).to.equal("http://example.com")
| const origin = req.headers.origin; | ||
| if (origin != null) { | ||
| res.setHeader("Access-Control-Allow-Origin", origin); | ||
| res.setHeader("Vary", "Origin"); | ||
| } |
There was a problem hiding this comment.
The CORS implementation in defaultRoute is inconsistent with the security-aware CORS pattern used elsewhere in the codebase. In setCorsForThing (routes/common.ts:94-105), CORS behavior depends on the security scheme: for non-nosec schemes with an origin, it reflects the origin and sets Access-Control-Allow-Credentials. In contrast, this defaultRoute implementation always reflects the origin without considering security context and never sets credentials. For consistency and security best practices, consider whether 404 responses should follow the same security-aware CORS pattern or if wildcard is more appropriate since no Thing context exists at this point.
| const origin = req.headers.origin; | |
| if (origin != null) { | |
| res.setHeader("Access-Control-Allow-Origin", origin); | |
| res.setHeader("Vary", "Origin"); | |
| } | |
| res.setHeader("Access-Control-Allow-Origin", "*"); |
Closes #1495
Description
This PR ensures that 404 responses include
Access-Control-Allow-Originwhen an
Originheader is present in the request.Previously, unknown routes returned 404 without CORS headers,
causing browsers to block cross-origin error responses.
Changes
defaultRouteinhttp-server.tsto include:Access-Control-Allow-OriginVary: OriginMotivation
Extends the CORS coverage introduced in #1486 and improves
standards compliance by ensuring consistent CORS behavior
for all HTTP responses, including 404 errors.