Skip to content

Commit e9ced69

Browse files
authored
Merge pull request #1651 from pmcelhaney/copilot/create-failing-tests-for-bugs
Bug Hunt: issue proposals documenting 3 bugs
2 parents c56dcff + f738498 commit e9ced69

File tree

3 files changed

+119
-0
lines changed

3 files changed

+119
-0
lines changed
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
---
2+
title: "Bug: Dispatcher proxy() always throws when request body is present"
3+
parentIssue: 1650
4+
labels:
5+
- bug
6+
assignees: []
7+
milestone:
8+
---
9+
10+
The `proxy()` helper inside `Dispatcher.request()` is meant to reject non-JSON requests with a helpful error. The guard is:
11+
12+
```ts
13+
if (body !== undefined && headers.contentType !== "application/json") {
14+
throw new Error(...);
15+
}
16+
```
17+
18+
The problem is `headers.contentType` (camelCase). HTTP headers use kebab-case (`content-type`), and the `headers` object never has a `contentType` key. `headers.contentType` is always `undefined`, so `undefined !== "application/json"` is always `true`.
19+
20+
The entire condition reduces to `body !== undefined`, meaning the proxy throws for **every** request that carries a body — including valid `application/json` POST requests.
21+
22+
## Failing test
23+
24+
Added in `test/server/dispatcher.proxy.test.ts`:
25+
26+
```
27+
does not throw when proxying a POST request with a JSON body and content-type: application/json
28+
```
29+
30+
Run `yarn test` and look for that test name in the `a dispatcher passes a proxy function to the operation` describe block.
31+
32+
## Root cause
33+
34+
```ts
35+
// src/server/dispatcher.ts
36+
if (body !== undefined && headers.contentType !== "application/json") {
37+
// ^^^^^^^^^^^^^ should be headers["content-type"]
38+
```
39+
40+
## Acceptance criteria
41+
42+
- [ ] `headers.contentType` is replaced with `headers["content-type"]`
43+
- [ ] The previously failing test passes
44+
- [ ] The existing proxy test continues to pass
45+
- [ ] A POST request with a non-JSON content type is still rejected with the descriptive error
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
title: "Bug: Tools.accepts() ignores lowercase 'accept' header — always returns true in production"
3+
parentIssue: 1650
4+
labels:
5+
- bug
6+
assignees: []
7+
milestone:
8+
---
9+
10+
`Tools.accepts()` reads `this.headers.Accept` (capital A). Node.js normalises all incoming HTTP header names to **lowercase**, so when a real HTTP request is processed via Koa the header is stored under `accept` (lowercase). `this.headers.Accept` is therefore always `undefined` in production.
11+
12+
Because `undefined` triggers the early-return guard `if (acceptHeader === "" || acceptHeader === undefined) { return true; }`, the method unconditionally returns `true` — completely ignoring the client's actual Accept header.
13+
14+
## Failing test
15+
16+
Added in `test/server/tools.test.ts`:
17+
18+
```
19+
accepts('application/json') returns false when lowercase 'accept' header is 'text/plain'
20+
```
21+
22+
Run `yarn test` and look for that test name in the `tools` describe block.
23+
24+
## Root cause
25+
26+
```ts
27+
// src/server/tools.ts
28+
const acceptHeader = this.headers.Accept; // ← should be this.headers.accept
29+
```
30+
31+
## Acceptance criteria
32+
33+
- [ ] `this.headers.Accept` is replaced by a case-insensitive lookup (or lowercased key lookup) so that `accept`, `Accept`, and `ACCEPT` all work
34+
- [ ] The previously failing test passes
35+
- [ ] All other `accepts()` tests continue to pass
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
title: "Bug: Tools.accepts() fails when Accept header has spaces after commas"
3+
parentIssue: 1650
4+
labels:
5+
- bug
6+
assignees: []
7+
milestone:
8+
---
9+
10+
A well-formed HTTP Accept header commonly separates values with `", "` (comma followed by a space), e.g. `"text/html, application/json"`.
11+
12+
`Tools.accepts()` splits on `","` but does **not** trim whitespace from each part. This produces `[" application/json"]` (with a leading space) as the second element. When that is further split on `"/"`, the type becomes `" application"` rather than `"application"`, so the comparison against the requested content type always fails and `accepts()` returns `false` even when the type is listed in the Accept header.
13+
14+
## Failing test
15+
16+
Added in `test/server/tools.test.ts`:
17+
18+
```
19+
accepts('application/json') returns true when Accept header is 'text/html, application/json' (with space)
20+
```
21+
22+
Run `yarn test` and look for that test name in the `tools` describe block.
23+
24+
## Root cause
25+
26+
```ts
27+
// src/server/tools.ts
28+
const acceptTypes = String(acceptHeader).split(",");
29+
return acceptTypes.some((acceptType) => {
30+
const [type, subtype] = acceptType.split("/"); // " application" has a leading space
31+
...
32+
});
33+
```
34+
35+
## Acceptance criteria
36+
37+
- [ ] Each token is trimmed (e.g. `acceptType.trim()`) before being split on `"/"`
38+
- [ ] The previously failing test passes
39+
- [ ] All other `accepts()` tests continue to pass

0 commit comments

Comments
 (0)