fix: use encodeURIComponent to encode engineId #4371

feloy · 2023-10-17T08:58:19Z

Signed-off-by: Philippe Martin phmartin@redhat.com

What does this PR do?

Use encode/decodeURIComponent instead of encode/decodeURI to encode/decode the engineId, which can contain / characters (in ocp sandbox for example).

Screenshot/screencast of this PR

What issues does this PR fix or reference?

Fixes #3868

How to test this PR?

See steps in #3868

benoitf · 2023-10-17T09:15:55Z

packages/renderer/src/routes.ts

@@ -0,0 +1,21 @@
+export class PodDetailsRoute {


Hello, I think we should not add code in src/ folder, as it's dedicated to Pods probably it can go as new method of the pod-utils

https://github.com/containers/podman-desktop/blob/main/packages/renderer/src/lib/pod/pod-utils.ts

My idea was to add new class/methods for each route defined in App.svelte (at least the ones with parameters), which are not all related to pods, and have these methods at the same level of App.svelte

I would say we need to split "code refactoring" and "fixes"

so do not try to change all routes of App.svelte and fix the encoding at the same time.

We need separate iteration/steps
one PR = one goal (can be refactoring (no-op) or bug fixes)

I don't see value in just adding a dedicated object just for routes as routes are specific to some area (like pods, containers, images, etc and we already have utility there)

in PodList we already have the object there

I would say we need to split "code refactoring" and "fixes"

You are right. I've created a new PR with the encoding fix only. I can work on the refactoring later, when I have a better idea of the source code.

benoitf · 2023-10-17T09:32:30Z

packages/renderer/src/App.svelte

+        <Route path="{PodDetailsRoute.path()}" breadcrumb="Pod Details" let:meta navigationHint="details">
          <PodDetails
-            podName="{decodeURI(meta.params.name)}"
-            engineId="{decodeURI(meta.params.engineId)}"
-            kind="{decodeURI(meta.params.kind)}" />
+            podName="{PodDetailsRoute.getName(meta.params)}"
+            engineId="{PodDetailsRoute.getEngineId(meta.params)}"
+            kind="{PodDetailsRoute.getKind(meta.params)}" />


Maybe it will be easier to have a getRouteDetails on utility class and then just pass the argument

like

<Route path="podUtils.path" breadcrumb="Pod Details" let:meta navigationHint="details"> {@const routeDetails = podUtils.getRouteDetails(meta.params)} <PodDetails podName="{routeDetails.podName}" engineId="{routeDetails.engineId}"

benoitf · 2023-10-17T09:34:36Z

packages/renderer/src/lib/pod/PodsList.svelte

@@ -126,7 +127,15 @@ async function deleteSelectedPods() {
 }

 function openDetailsPod(pod: PodInfoUI) {
-  router.goto(`/pods/${encodeURI(pod.kind)}/${encodeURI(pod.name)}/${encodeURI(pod.engineId)}/logs`);
+  const route = PodDetailsRoute.getRoute(


we can ask podUtils object instead of adding a new PodDetailsRoute

also we can provide podInfoUi object to the method instead of providing each individual parameter

I would not want to make the (generic) Route methods coupled with the podInfoUi. For example in the following line, it would need to build such an podInfoUi object from the ContainerGroupInfoUI object and the podman constant:
https://github.com/containers/podman-desktop/blob/main/packages/renderer/src/lib/ContainerList.svelte#L349

I would prefer to simply call it as:

PodDetailsRoute.getRoute({ kind: 'podman', name: containerGroup.name, engineId: containerGroup.engineId })

but you can have two methods, one with PodInfoUI and one with meta.params so it's convenient on client side

in the example you shown https://github.com/containers/podman-desktop/blob/main/packages/renderer/src/lib/ContainerList.svelte#L349-L351 you can provide containerGrou/ContainerGroupInfoUI object instead of building a new object

benoitf

hello @feloy could you add unit tests to check that we're fixing the problem

?

feloy · 2023-10-18T15:01:24Z

hello @feloy could you add unit tests to check that we're fixing the problem

@benoitf I'm not sure how to test this.

This involves the Route, which will analyze the passed path and split it to extract the engineId.

But the Route is defined at the top level, in App.svelte, which means the test would need to instantiate the whole app.

What do you think?

benoitf

@feloy I agree it might be difficult without a refactoring to test the current design of App.svelte as it's not so modular/delegating stuff (the decode part)

But we can I think test the part on encoding like on ContainerList, PodActions, PodDetailsLogs PodList to see if the router is correctly calling with the expected route

for example if we call in PodList the openDetails we can check the parameter on router.goto and see the behaviour with encodeURI vs encodeURIComponent

… to encode engineId Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

This reverts commit 64a4028. Signed-off-by: Philippe Martin <phmartin@redhat.com>

…omponent to encode engineId" This reverts commit 9c2e239. Signed-off-by: Philippe Martin <phmartin@redhat.com>

OCP Sandobox provides an engineID with / characters, which are not encoded with encodeURI Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

Signed-off-by: Philippe Martin <phmartin@redhat.com>

benoitf

thanks @feloy

lstocchi

LGTM. Tested, works fine 🚀

feloy requested review from benoitf and a team as code owners October 17, 2023 08:58

feloy requested review from dgolovin and lstocchi and removed request for a team October 17, 2023 08:58

feloy marked this pull request as draft October 17, 2023 08:58

benoitf reviewed Oct 17, 2023

View reviewed changes

feloy changed the title ~~fix: create a PodDetailsRoute helper class and use decodeURIComponent…~~ fix: use encodeURIComponenr to encode engineId Oct 17, 2023

feloy force-pushed the bugfix-3868/encode-engine-id branch from 1e75594 to 771ef0b Compare October 17, 2023 12:29

feloy marked this pull request as ready for review October 17, 2023 12:46

benoitf reviewed Oct 17, 2023

View reviewed changes

benoitf changed the title ~~fix: use encodeURIComponenr to encode engineId~~ fix: use encodeURIComponent to encode engineId Oct 19, 2023

benoitf reviewed Oct 19, 2023

View reviewed changes

feloy added 5 commits October 19, 2023 12:12

fix: create a PodDetailsRoute helper class and use decodeURIComponent…

a744b86

… to encode engineId Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

fix: define a getParameters instead

ce2894f

Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

Revert "fix: define a getParameters instead"

da1a3df

This reverts commit 64a4028. Signed-off-by: Philippe Martin <phmartin@redhat.com>

Revert "fix: create a PodDetailsRoute helper class and use decodeURIC…

b8ea9fe

…omponent to encode engineId" This reverts commit 9c2e239. Signed-off-by: Philippe Martin <phmartin@redhat.com>

fix: use encode/decodeURIComponent for engineId

aea37d7

OCP Sandobox provides an engineID with / characters, which are not encoded with encodeURI Signed-off-by: Philippe Martin <phmartin@redhat.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>

feloy force-pushed the bugfix-3868/encode-engine-id branch from 771ef0b to 915ad42 Compare October 19, 2023 10:12

fix: unit test

c58e505

Signed-off-by: Philippe Martin <phmartin@redhat.com>

feloy force-pushed the bugfix-3868/encode-engine-id branch from 915ad42 to c58e505 Compare October 19, 2023 14:11

feloy requested a review from benoitf October 20, 2023 07:11

benoitf approved these changes Oct 20, 2023

View reviewed changes

lstocchi approved these changes Oct 20, 2023

View reviewed changes

feloy merged commit 23223c8 into containers:main Oct 20, 2023
9 checks passed

podman-desktop-bot added this to the 1.5.0 milestone Oct 20, 2023

benoitf mentioned this pull request Nov 16, 2023

fix: merged compose deploy to kube page in UI #4827

Merged

deboer-tim mentioned this pull request Nov 23, 2023

Navigation path API #4966

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use encodeURIComponent to encode engineId #4371

fix: use encodeURIComponent to encode engineId #4371

feloy commented Oct 17, 2023 •

edited

benoitf Oct 17, 2023

feloy Oct 17, 2023

benoitf Oct 17, 2023

feloy Oct 17, 2023

benoitf Oct 17, 2023

benoitf Oct 17, 2023

feloy Oct 17, 2023 •

edited

benoitf Oct 17, 2023

benoitf Oct 17, 2023 •

edited

benoitf left a comment

feloy commented Oct 18, 2023

benoitf left a comment

benoitf left a comment

lstocchi left a comment

fix: use encodeURIComponent to encode engineId #4371

fix: use encodeURIComponent to encode engineId #4371

Conversation

feloy commented Oct 17, 2023 • edited

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

benoitf Oct 17, 2023

Choose a reason for hiding this comment

feloy Oct 17, 2023

Choose a reason for hiding this comment

benoitf Oct 17, 2023

Choose a reason for hiding this comment

feloy Oct 17, 2023

Choose a reason for hiding this comment

benoitf Oct 17, 2023

Choose a reason for hiding this comment

benoitf Oct 17, 2023

Choose a reason for hiding this comment

feloy Oct 17, 2023 • edited

Choose a reason for hiding this comment

benoitf Oct 17, 2023

Choose a reason for hiding this comment

benoitf Oct 17, 2023 • edited

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

feloy commented Oct 18, 2023

benoitf left a comment

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

lstocchi left a comment

Choose a reason for hiding this comment

feloy commented Oct 17, 2023 •

edited

feloy Oct 17, 2023 •

edited

benoitf Oct 17, 2023 •

edited