Skip to content

Cleanup HTTP client headers and testing code #55

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.

Sign up for GitHub

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

Open
cyrossignol wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cleanup HTTP client headers and testing code #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51238da
Clean up http client headers
cyrossignol Mar 10, 2026
faec03d
Clean up testing code in HTTP client
cyrossignol Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 44 additions & 31 deletions services/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
export type FetchConfig = Omit<RequestInit, 'headers'> & {
headers?: Record<string, string>;
};

export type HttpBody = BodyInit | object;

export class BaseHttpClientError extends Error {
response: Response;

Expand Down Expand Up @@ -25,59 +31,66 @@ export abstract class BaseHttpClient {
return this._baseUrl + rest
}

_get(url: string, config?: object): Promise<Response> {
_get(url: string, config?: FetchConfig): Promise<Response> {
return this._send(url, 'GET', undefined, config);
}

_post(url: string, body?: any, config?: object): Promise<Response> {
_post(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
return this._send(url, 'POST', body, config);
}

_put(url: string, body?: any, config?: object): Promise<Response> {
_put(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
return this._send(url, 'PUT', body, config);
}

_patch(url: string, body?: any, config?: object): Promise<Response> {
_patch(url: string, body?: HttpBody, config?: FetchConfig): Promise<Response> {
return this._send(url, 'PATCH', body, config);
}

_delete(url: string, config?: object): Promise<Response> {
_delete(url: string, config?: FetchConfig): Promise<Response> {
return this._send(url, 'DELETE', undefined, config);
}

async _sendTest(url: string, method: string, body?: any): Promise<Response> {
const response = await fetch(this.url(url), {
method,
body,
headers: {
'Accept': 'application/text',
'Authorization': this._requestHeaders.Authorization
}
});

if (!response.ok) {
throw new BaseHttpClientError(response);
}

return response;
}

async _send(url: string, method: string, body?: any, config?: object): Promise<Response> {
async _send(
url: string,
method: string,
body?: HttpBody,
config?: FetchConfig,
): Promise<Response> {
const { headers: configHeaders, ...restConfig } = config ?? {};
const mergedHeaders: Record<string, string> = {
...this._requestHeaders,
...configHeaders,
};
const requestOptions = {
method,
body,
headers: this._requestHeaders,
body: undefined as BodyInit | undefined,
headers: mergedHeaders,
signal: this._abortSignal,
...config
...restConfig,
};

if (requestOptions.headers['Content-Type'] === 'application/json'
&& !(body instanceof FormData)
&& typeof body !== 'undefined'
&& body !== null
) {
// Let the browser set Content-Type (and multipart boundary) for FormData:
if (body instanceof FormData) {
delete mergedHeaders['Content-Type'];
requestOptions.body = body;
}
else if (body !== null && body !== undefined && (
mergedHeaders['Content-Type'] === 'application/json'
|| (
typeof body === 'object'
&& !(body instanceof Blob)
&& !(body instanceof ArrayBuffer)
&& !ArrayBuffer.isView(body)
&& !(body instanceof URLSearchParams)
&& !(body instanceof ReadableStream)
)
)) {
requestOptions.body = JSON.stringify(body);
}
else {
requestOptions.body = body as BodyInit;
}

const response = await fetch(this.url(url), requestOptions);

Expand Down
51 changes: 25 additions & 26 deletions services/osm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import parseOsmChangeXml from '@osmcha/osmchange-parser';
import type { FeatureCollection, Point } from 'geojson';

import { BaseHttpClient, BaseHttpClientError } from "~/services/http";
import {
BaseHttpClient,
BaseHttpClientError,
type FetchConfig,
type HttpBody,
} from '~/services/http';
import * as xml from '~/util/xml';

import type { ICancelableClient } from '~/services/loading';
Expand All @@ -14,7 +19,6 @@ import type {
OsmElement,
OsmNode,
OsmNote,
OsmTags,
OsmWay,
} from '~/types/osm';
import type { WorkspaceId } from '~/types/workspaces';
Expand Down Expand Up @@ -207,7 +211,9 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
this.#webUrl = webUrl;
this.#tdeiClient = tdeiClient;
this.#setAuthHeader();
this._requestHeaders['Accept'] = 'text/plain';

// OSM API can return XML or JSON based on the header or file extension:
this._requestHeaders['Accept'] = '*/*';
this._requestHeaders['Content-Type'] = 'text/plain';
}

Expand All @@ -234,8 +240,8 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
display_name: this.auth.displayName
};

await this._put(`user/${this.auth.subject}`, JSON.stringify(body), {
headers: { ...this._requestHeaders, 'Content-Type': 'application/json' }
await this._put(`user/${this.auth.subject}`, body, {
headers: { 'Content-Type': 'application/json' }
});
}

Expand Down Expand Up @@ -286,7 +292,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
): Promise<OsmElement> {
const response = await this._get(`${type}/${id}/${version}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -301,7 +306,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
async getNodes(workspaceId: WorkspaceId, nodeIds: (number | string)[]): Promise<OsmNode[]> {
const response = await this._get(`nodes?nodes=${nodeIds.join(',')}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -319,7 +323,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
async getWays(workspaceId: WorkspaceId, wayIds: (number | string)[]): Promise<OsmWay[]> {
const response = await this._get(`ways?ways=${wayIds.join(',')}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -337,7 +340,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
async getWaysForNode(workspaceId: WorkspaceId, nodeId: number): Promise<OsmElement[]> {
const response = await this._get(`node/${nodeId}/ways`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -348,7 +350,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {

async listChangesets(workspaceId: WorkspaceId): Promise<OsmChangeset[]> {
const response = await this._get(`changesets.json`, {
headers: { ...this._requestHeaders, 'X-Workspace': workspaceId },
headers: { 'X-Workspace': workspaceId },
});

const changesets = (await response.json())?.changesets ?? [];
Expand All @@ -374,7 +376,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {

const response = await this._get(url, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -401,7 +402,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
{
const response = await this._get(`changeset/${changesetId}/download`, {
headers: {
...this._requestHeaders,
'Accept': 'application/xml',
'X-Workspace': workspaceId,
},
Expand All @@ -427,7 +427,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {

const body = xml.serialize(doc);
const response = await this._put('changeset/create', body, {
headers: { ...this._requestHeaders, 'X-Workspace': workspaceId },
headers: { 'X-Workspace': workspaceId },
});

return Number(await response.text());
Expand All @@ -441,7 +441,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
await this._post(`changeset/${changesetId}/upload`, changesetXml, {
headers: {
'Content-Type': 'application/xml',
'Authorization': this._requestHeaders['Authorization'],
'X-Workspace': workspaceId,
},
});
Expand All @@ -468,10 +467,7 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
body.append('text', message);

await this._post(`changeset/${changesetId}/comment`, body, {
headers: {
'Authorization': this._requestHeaders['Authorization'],
'X-Workspace': workspaceId,
},
headers: { 'X-Workspace': workspaceId },
});
}

Expand All @@ -484,7 +480,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {

const response = await this._get(`notes/search.json?${params}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId,
},
Expand All @@ -497,7 +492,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
const bboxParam = await this.getExportBbox(workspaceId);
const response = await this._get(`map.json?bbox=${bboxParam}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/json',
'X-Workspace': workspaceId
}
Expand All @@ -510,7 +504,6 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
const bboxParam = await this.getExportBbox(workspaceId);
const response = await this._get(`map?bbox=${bboxParam}`, {
headers: {
...this._requestHeaders,
'Accept': 'application/xml',
'X-Workspace': workspaceId
}
Expand All @@ -525,17 +518,23 @@ export class OsmApiClient extends BaseHttpClient implements ICancelableClient {
}
}

async _send(url: string, method: string, body?: any, config?: object): Promise<Response> {
override async _send(
url: string,
method: string,
body?: HttpBody,
config?: FetchConfig,
): Promise<Response> {
try {
await this.#tdeiClient.tryRefreshAuth();
this.#setAuthHeader();

const requestOptions = {
credentials: 'include'
}
const requestOptions: FetchConfig = {
credentials: 'include',
};

return await super._send(url, method, body, { ...requestOptions, ...config });
Comment on lines +521 to 535
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear stale Authorization header before sending requests.

On Line 529, _send reuses #setAuthHeader(), but that helper only writes when auth is complete and never clears an old token. After auth is cleared, this can keep sending a stale bearer value.

Suggested fix 
  `#setAuthHeader`() {
-    if (this.#tdeiClient.auth.complete) {
-      this._requestHeaders.Authorization = 'Bearer ' + this.#tdeiClient.auth.accessToken;
-    }
+    this._requestHeaders.Authorization = this.#tdeiClient.auth.complete
+      ? `Bearer ${this.#tdeiClient.auth.accessToken}`
+      : '';
  }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/osm.ts` around lines 521 - 535, The _send method can reuse a stale
Authorization header because `#setAuthHeader` only adds a header when auth exists
but never removes an old one; update the code so that before calling
`#setAuthHeader` (or inside `#setAuthHeader`) you explicitly remove any existing
Authorization header when there is no current token — for example, clear
this.headers['authorization'] (or call a new clearAuthHeader helper) if
`#tdeiClient.tryRefreshAuth` indicates auth is not present, then only set the
header when a fresh token is available; make this change in the _send flow (and
optionally in `#setAuthHeader`) to ensure stale bearer values are not sent.

} catch (e: any) {
}
catch (e: unknown) {
if (e instanceof BaseHttpClientError) {
throw new OsmApiClientError(e.response);
}
Expand Down
Loading
Loading