Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Fix UI escaping special symbols #118

Merged
merged 4 commits into from
Sep 1, 2022
Merged

Fix UI escaping special symbols #118

merged 4 commits into from
Sep 1, 2022

Conversation

feedmeapples
Copy link
Contributor

What changed?

Fixes escaping of special URL symbols

Why?

Resolves #117

How did you test it?

Verified with a workflow id that has spaces, /, %, /, \ etc https://github.com/minimaxir/big-list-of-naughty-strings

Potential risks

Is hotfix candidate?

@@ -139,3 +139,5 @@ require (
gopkg.in/validator.v2 v2.0.1 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

replace github.com/grpc-ecosystem/grpc-gateway => github.com/temporalio/grpc-gateway v1.17.0
Copy link
Member

@cretz cretz Aug 30, 2022

Choose a reason for hiding this comment

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

Can you give me a brief update of what we customize in our custom fork of grpc-gateway and the potential status of upstreaming our changes? Is it solely due to temporalio/grpc-gateway#1? I see they're on a separate major version by now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We may also need to make a note about this override for library users who are depending on Temporalite through go mod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the initial issue came from sveltejs/kit#3069

grpc-gateway v2 introduced an option WithUnescapingMode that would allow to address the escaping properly, however this was not moved into v1. See grpc-ecosystem/grpc-gateway#2629 for context and why we had to fork

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also considered upgrading to grpc-gateway v2, asked the server team if we could move to protobuf-apiv2. In short such upgrade didn't get into our timeline yet because of many moving pieces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a readme comment

Copy link
Member

Choose a reason for hiding this comment

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

That's unfortunate but understandable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@feedmeapples is there an existing issue for migrating to the v2 protobuf libraries we can link here?

Copy link
Member

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jlegrone jlegrone enabled auto-merge (squash) September 1, 2022 00:28
@jlegrone jlegrone merged commit 42dab08 into main Sep 1, 2022
@jlegrone jlegrone deleted the fix-escaping branch September 1, 2022 00:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants