Skip to content
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.

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

Fix failing test due to updated path-to-regexp #1424

Conversation

okimiko
Copy link
Contributor

@okimiko okimiko commented Dec 17, 2024

Workaround was found here: GHSA-9wv6-86v2-598j

Should fix/support #1412.

@acalcutt
Copy link
Collaborator

The ci tests didn't seem to run in this PR, so i tested merging this into #1401 . however it doesn't seem to fix the issue

I tested this locally and I get
Invalid regular expression: /^/(?:(?:(256|512))/)?(?:((?:(?!/|//)?).)+?)).json/?$/i: Unmatched ')'

@okimiko
Copy link
Contributor Author

okimiko commented Dec 18, 2024

I can not confirm this: I did a clean checkout and build a clean docker build-image (based on the build step in the default Dockerfile) and did not have this error in my environment.

But in fact, the runtime tests did not work because the process failed to spawn (missing X display), so I only tested my change, which worked so far. In https://github.com/maptiler/tileserver-gl/actions/runs/11783668480/job/32821321983#step:10:169 we can see the same, but there is a different path, which needs an update, too.

I just added the missing xvfb-run to npm test and can confirm the results of #1401. I will try to take care of that later.

@okimiko
Copy link
Contributor Author

okimiko commented Dec 19, 2024

I found no issues in the routes, I even reviewed the express routes with a route tester (fyi: https://forbeslindesay.github.io/express-route-tester/). But at last I had the idea to look at the result of the test and received a "invalid center", which is correct (see https://github.com/maptiler/tileserver-gl/blame/master/src/serve_rendered.js#L384-L390).

So I fixed the test results for the related queries, moved them to the "invalid requests" section and added correct ones in "different parameters".

It seems, that with the previous version of express/path-to-regexp the test passes (and a running 5.0.0 instance is in fact delivering an image), so the path handling seems to be mixed up or at least respondImage does not receive the out-of-range parameters.

@acalcutt
Copy link
Collaborator

acalcutt commented Dec 20, 2024

After more testing I see this does fix this error
image

With your fix the tests get past that issue, and actually run. However like you mention, there are still two test failing that are preventing the upgrade of express.
image
(this is run with the steps in https://github.com/maptiler/tileserver-gl?tab=readme-ov-file#getting-started-with-linux-cli ...except on windows i don't need xvfb-run )

Like you mentioned, I noticed that 'Invalid Center' also. I did test older versions and can confirm it worked when using the older express. I did spend some time looking at the regex's . I think that may be handled by https://github.com/maptiler/tileserver-gl/blob/master/src/serve_rendered.js#L751 , but it is a bit confusing. I had gotten a few suggestions from google AI but didn't have any luck with them. It would seem to get 'Invalid Center' it must not be getting the coordinates

@okimiko
Copy link
Contributor Author

okimiko commented Dec 20, 2024

Yes, serveBounds is correct and should lead to respondImage and finally to the out-of-bounds check -> "Invalid Center".

I think the issue was already in the code and just revealed by the update. You may merge your branch again, I did two fixed in the test config:

  • Set correct return code for current tests
  • Add new ones, with valid parameters for this case

I do not realy understand, why the test passed before, but I assume that something has changed in the environment or a different routing expression was used in the past (because of a weaker regex?). Anyway, this issue helped me finding the issue in #1425 ;-)

@okimiko
Copy link
Contributor Author

okimiko commented Jan 10, 2025

Obsolete due to #1429.

@okimiko okimiko closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants