fix: normalize urls in stylejson payloads by achou11 · Pull Request #42 · digidem/comapeo-map-server

achou11 · 2026-04-01T21:20:18Z

Fixes #29

Mapbox style fixture is taken from here. Might consider just creating much smaller fixture that covers the properties of interest.

socket-security · 2026-04-01T21:21:13Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@maplibre/maplibre-gl-style-spec@24.7.0
	@mapbox/mapbox-gl-style-spec@14.21.0

View full report

src/lib/style.ts

achou11 · 2026-04-02T16:00:10Z

TODO: integrate implementation from https://github.com/digidem/mapbox-style-transformer instead of the one done here

achou11 · 2026-04-06T19:04:18Z

TODO: integrate implementation from https://github.com/digidem/mapbox-style-transformer instead of the one done here

@gmaclennan Haven't done this yet because the module needs to a new version released to be usable.

achou11 · 2026-04-06T19:05:19Z

src/routes/maps.ts

+		// Can use `?mapbox_access_token=...` to override the access token used
+		// for upstream mapbox style request
+		if (defaultOnlineStyleUrl.toString().startsWith(BASE_MAPBOX_API_URL)) {
+			const accessTokenFromRequest = Array.isArray(
+				request.query['mapbox_access_token'],
+			)
+				? request.query['mapbox_access_token'][0]
+				: request.query['mapbox_access_token']
+
+			// Prioritize the access token from the request
+			if (accessTokenFromRequest) {
+				defaultOnlineStyleUrl.searchParams.set(
+					'access_token',
+					accessTokenFromRequest,
+				)
+			}
+		}


I thought this could be a useful feature to support. Still undecided on that and whether the access token in the request should be scoped to the specific use case i.e. mapbox_access_token vs access_token.

I don't see the use-case for this and I think this is potentially confusing. This is only used for the defaultOnlineStyleUrl (a static option passed to the constructor) which generally must include an ?access_token= search param if it is a mapbox URL. Having this possibly overridable and therefore also optional I think confuses the API with not a lot of benefit. We won't be using this in CoMapeo, and this is a temporary measure anyway until we move away from mapbox as the default online style, so I think we should just remove this.

the situation this accounts for is when the access token you pass to the static option no longer works (because of expiry, deletion, etc) which has happened to us before 😄 being able to override it at the request level allows for workarounds in those situations without requiring new releases of the app, although that assumes that there's some client-side exposure for configuring your own access token.

but yeah, probably not worth trying to account for in this case. just trauma-based failure mode handling

I think we can address this risk a different way down the line.

Addressed via 1a13856

achou11 · 2026-04-06T19:05:39Z

src/routes/maps.ts

+					})
+				}
+			} else {
+				response?.body?.cancel() // Close the connection


is this still necessary, or even placed in the right location?

This is only necessary if you return a 302 response without consuming the upstream response body. The code as-is now always consumes the body, so this is not necessary, however if you make the suggested changes, you will still need to do this if you do not consume the body (because it will be left hanging and the connection will remain open).

Addressed via 787555b

achou11 · 2026-04-06T19:06:07Z

src/routes/maps.ts

+				if (madeChanges) {
+					return new Response(JSON.stringify(body), {
+						status: 200,
+						headers: baseHeaders,
+					})
+				} else {
+					return new Response(null, {
+						status: 302,
+						headers: {
+							...baseHeaders,
+							location: url.toString(),
+						},
+					})
+				}


I thought this could make sense but maybe it's overcomplicating things. On the bright side, means I don't have to fix the existing tests 😄

Yes, I think overcomplicating things.

Addressed via 787555b

gmaclennan

I think this over-engineers things a bit. This is only a temporary transformation of the defaultOnlineStyleUrl until we switch to using a non-mapbox style, and it currently adds quite a few extra code paths to the code that need testing and maintaining, and a new feature of making the access token optional on the defaultOnlineStyleUrl and possibly overridable from the API. This seems to be moving towards a broader long-term support of mapbox styles in this package, which I don't think is needed at this stage, and we may not ever do.

gmaclennan · 2026-04-07T13:18:36Z

src/routes/maps.ts

+		// Can use `?mapbox_access_token=...` to override the access token used
+		// for upstream mapbox style request
+		if (defaultOnlineStyleUrl.toString().startsWith(BASE_MAPBOX_API_URL)) {
+			const accessTokenFromRequest = Array.isArray(
+				request.query['mapbox_access_token'],
+			)
+				? request.query['mapbox_access_token'][0]
+				: request.query['mapbox_access_token']
+
+			// Prioritize the access token from the request
+			if (accessTokenFromRequest) {
+				defaultOnlineStyleUrl.searchParams.set(
+					'access_token',
+					accessTokenFromRequest,
+				)
+			}
+		}


I don't see the use-case for this and I think this is potentially confusing. This is only used for the defaultOnlineStyleUrl (a static option passed to the constructor) which generally must include an ?access_token= search param if it is a mapbox URL. Having this possibly overridable and therefore also optional I think confuses the API with not a lot of benefit. We won't be using this in CoMapeo, and this is a temporary measure anyway until we move away from mapbox as the default online style, so I think we should just remove this.

gmaclennan · 2026-04-07T13:23:02Z

src/routes/maps.ts

+				const body = await response.json()
+
+				const madeChanges = transformUrls(body, {
+					accessToken:
+						defaultOnlineStyleUrl.searchParams.get('access_token') || undefined,
 				})


This unnecessarily loads the body of all style requests, when we only need to do it for the default online style URL. Also I don't think we need to load the body for non-mapbox styles. Currently the "is it a mapbox style" question is done by checking whether the transormUrls() funciton returns true. I think the detection used above (does the defaultOnlineStyleUrl start with BASE_MAPBOX_API_URL) is better, and that way we don't need to load the body of requests for non-mapbox styles

make sense. based on that, then the more desirable behavior is the following?

for non-mapbox styles, there's no need to load the body and we keep the existing redirect behavior

for mapbox styles, we load the body, attempt to tranform, and provide a 200 response with the maybe-transformed body? basically, remove the conditional response type based on whether any transforms were actually made

I think the detection used above (does the defaultOnlineStyleUrl start with BASE_MAPBOX_API_URL) is better

Just noting that doing this wouldn't account for online style urls whose payloads include fields that reference mapbox using their URIs. definitely an edge case but just highlighting that.

Yes I can't think of a situation where someone would choose to host a style that references MapBox resources somewhere other than MapBox, and in any case we are only really handling the case of our current default map tiles

make sense. based on that, then the more desirable behavior is the following?

for non-mapbox styles, there's no need to load the body and we keep the existing redirect behavior

for mapbox styles, we load the body, attempt to tranform, and provide a 200 response with the maybe-transformed body? basically, remove the conditional response type based on whether any transforms were actually made

Yes

Addressed via 787555b

gmaclennan · 2026-04-07T13:23:21Z

src/routes/maps.ts

+				if (madeChanges) {
+					return new Response(JSON.stringify(body), {
+						status: 200,
+						headers: baseHeaders,
+					})
+				} else {
+					return new Response(null, {
+						status: 302,
+						headers: {
+							...baseHeaders,
+							location: url.toString(),
+						},
+					})
+				}


Yes, I think overcomplicating things.

gmaclennan · 2026-04-07T13:24:35Z

src/routes/maps.ts

+					})
+				}
+			} else {
+				response?.body?.cancel() // Close the connection


This is only necessary if you return a 302 response without consuming the upstream response body. The code as-is now always consumes the body, so this is not necessary, however if you make the suggested changes, you will still need to do this if you do not consume the body (because it will be left hanging and the connection will remain open).

achou11 · 2026-04-08T18:23:07Z

src/routes/maps.ts

+		// Can use `?mapbox_access_token=...` to override the access token used
+		// for upstream mapbox style request
+		if (defaultOnlineStyleUrl.toString().startsWith(BASE_MAPBOX_API_URL)) {
+			const accessTokenFromRequest = Array.isArray(
+				request.query['mapbox_access_token'],
+			)
+				? request.query['mapbox_access_token'][0]
+				: request.query['mapbox_access_token']
+
+			// Prioritize the access token from the request
+			if (accessTokenFromRequest) {
+				defaultOnlineStyleUrl.searchParams.set(
+					'access_token',
+					accessTokenFromRequest,
+				)
+			}
+		}


Addressed via 1a13856

achou11 · 2026-04-08T18:24:37Z

package.json

guess this file wasn't updated to be formatted according to the prettier config?

achou11 added 9 commits March 30, 2026 14:30

wip

a137aa0

update projection property

f0db8d5

update api interface

165dc8a

create separate export for applying changes inline

345f7c3

fix projection porting and add tests

14ce1bb

only expose mutative API

c375900

minor test update

a9640a0

rename function

7ae99f2

add comments about usage of decodeURI

e9f6782

achou11 commented Apr 1, 2026

View reviewed changes

src/lib/style.ts Show resolved Hide resolved

src/lib/style.ts Show resolved Hide resolved

achou11 added 2 commits April 6, 2026 13:06

update return type of normalizeStyle

83fd190

initial attempt to integrate into map server

0832194

achou11 changed the title ~~[WIP] fix: normalize urls in stylejson payloads~~ fix: normalize urls in stylejson payloads Apr 6, 2026

update implementation

108e914

achou11 marked this pull request as ready for review April 6, 2026 19:03

achou11 commented Apr 6, 2026

View reviewed changes

achou11 requested a review from gmaclennan April 6, 2026 19:07

gmaclennan requested changes Apr 7, 2026

View reviewed changes

achou11 added 2 commits April 8, 2026 14:10

remove token override feature

1a13856

only apply transform on relevant urls

787555b

achou11 commented Apr 8, 2026

View reviewed changes

achou11 requested a review from gmaclennan April 8, 2026 18:25

Conversation

achou11 commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achou11 commented Apr 2, 2026

Uh oh!

achou11 commented Apr 6, 2026

Uh oh!

achou11 Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaclennan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achou11 Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

achou11 commented Apr 1, 2026 •

edited

Loading

socket-security bot commented Apr 1, 2026 •

edited

Loading

achou11 Apr 6, 2026 •

edited

Loading

achou11 Apr 7, 2026 •

edited

Loading