Skip to content

Update pathProxy.ts #6154

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

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Update pathProxy.ts #6154

merged 4 commits into from
Apr 25, 2023

Conversation

smalllady
Copy link
Contributor

@smalllady smalllady commented Apr 20, 2023

When trying to preview an HTML file with Chinese characters in its file name, the preview may fail to display correctly.

Fixes #

fix:预览包含中文字符命名的html文件,无法正常预览
@smalllady smalllady requested a review from a team as a code owner April 20, 2023 03:16
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #6154 (3d244f8) into main (c995988) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6154   +/-   ##
=======================================
  Coverage   73.16%   73.16%           
=======================================
  Files          30       30           
  Lines        1718     1718           
  Branches      380      380           
=======================================
  Hits         1257     1257           
  Misses        385      385           
  Partials       76       76           
Impacted Files Coverage Δ
src/node/routes/pathProxy.ts 82.14% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e17735...3d244f8. Read the comment docs.

@code-asher
Copy link
Member

Thank you for finding this and the fix! Just FYI I added a test and fixed the lint error, will merge once CI passes.

@code-asher code-asher enabled auto-merge (squash) April 25, 2023 19:08
@code-asher code-asher merged commit 951d8ac into coder:main Apr 25, 2023
@sha-shrestha
Copy link

This is now causing a regression wherein if you have a url with "/" in the query pattern (i.e. example.com/proxy/1234/foo?a=/b), it now encodes it to "example.com/proxy/1234/foo?a=%2Fb" which breaks backend process parser that expects a "/" as a query parameter.

@code-asher
Copy link
Member

code-asher commented Jul 18, 2023

Yeah we should remove the encodeURI around the query portion I think, as it is already encoded and is getting double-encoded. #6307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants