LSP: Fixes URL decoding incoming file names. #13473

christianparpart · 2022-09-02T13:33:12Z

This fixes #13035.

On Windows platform, it is much more likely to hit, as we did not URL decode the incoming URL, and thus, the files (nor the base path, if specified as rootUri) could not be opened nor traversed.

Checklist

fix URL decoding/encoding
Changelog entry
tests

libsolutil/StringUtils.cpp

Marenz

I'd like to see at least one test with a full url that is also using /

Changelog.md

test/libsolutil/StringUtils.cpp

cameel

Looks good overall, but could use some small tweaks.

cameel · 2022-10-21T21:03:32Z

Changelog.md

@@ -20,6 +21,7 @@ Important Bugfixes:

 Compiler Features:
 * Code Generator: More efficient overflow checks for multiplication.
+ * Yul Optimizer: Simplify the starting offset of zero-length operations to zero.


This seems out of place in this PR.

cameel · 2022-10-21T21:08:28Z

Changelog.md

@@ -7,6 +7,7 @@ Compiler Features:
 * Commandline Interface: Add `--no-cbor-metadata` that skips CBOR metadata from getting appended at the end of the bytecode.
 * Standard JSON: Add a boolean field `settings.metadata.appendCBOR` that skips CBOR metadata from getting appended at the end of the bytecode.
 * Yul Optimizer: Allow replacing the previously hard-coded cleanup sequence by specifying custom steps after a colon delimiter (``:``) in the sequence string.
+* Language Server: Fixes URL decoding of incoming file names.


I was initially confused as to why file names would need URL-decoding. I think it would be better to say that we're decoding the file:// URLs.

Suggested change

* Language Server: Fixes URL decoding of incoming file names.

* Language Server: Properly URL-decode the `file://` URIs coming from the client and encode the URIs coming out of by the server.

I'd also say it's a bugfix rather than a feature :)

cameel · 2022-10-21T21:10:35Z

libsolutil/CMakeLists.txt

-target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system range-v3)
+target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system range-v3 fmt::fmt-header-only)


This and the URL-encoding helpers are things I'd put in a separate commit. They should stand out as its own thing in history.

cameel · 2022-10-21T21:31:18Z

libsolutil/StringUtils.h

+/// Decodes a URI with respect to %XX notation.
+/// No URI-validity verification is performed but simply the URI decoded into non-escaping characters.
+std::string decodeURI(std::string const& _uri);
+
+/// Encodes a string into a URI conform notation.
+std::string encodeURI(std::string const& _uri);


Given that these don't have to be URIs (it works for arbitrary strings), I'd call it something like urlEncode() and urlDecode() (or even urlencode() and urldecode()). I associate the term "urlencoding" specifically with this kind of encoding while "encoding an URI" could mean any encoding. I would probably look for the former I wanted to find it.

cameel · 2022-10-21T21:35:29Z

libsolutil/StringUtils.h

+/// Decodes a URI with respect to %XX notation.
+/// No URI-validity verification is performed but simply the URI decoded into non-escaping characters.
+std::string decodeURI(std::string const& _uri);
+
+/// Encodes a string into a URI conform notation.
+std::string encodeURI(std::string const& _uri);


Suggested change

/// Decodes a URI with respect to %XX notation.

/// No URI-validity verification is performed but simply the URI decoded into non-escaping characters.

std::string decodeURI(std::string const& _uri);

/// Encodes a string into a URI conform notation.

std::string encodeURI(std::string const& _uri);

/// Decodes a percent-encoded string (i.e. the %XX notation).

/// No URI-validity verification is performed but simply the URI decoded into non-escaping characters.

std::string decodeURI(std::string const& _uri);

/// Percent-encodes a string.

/// Note that `:` and `/` are preserved, which means that the function will not mangle the `protocol://`

/// prefix in URLs.

std::string encodeURI(std::string const& _uri);

cameel · 2022-10-21T21:49:42Z

test/libsolutil/StringUtils.cpp

+	BOOST_CHECK_EQUAL(util::decodeURI(""), "");
+	BOOST_CHECK_EQUAL(util::decodeURI(""), "");


Duplicate test.

cameel · 2022-10-21T21:49:53Z

test/libsolutil/StringUtils.cpp

+{
+	BOOST_CHECK_EQUAL(util::decodeURI(""), "");
+	BOOST_CHECK_EQUAL(util::decodeURI(""), "");
+	BOOST_CHECK_EQUAL(util::decodeURI(" ")," ");


Suggested change

BOOST_CHECK_EQUAL(util::decodeURI(" ")," ");

BOOST_CHECK_EQUAL(util::decodeURI(" "), " ");

cameel · 2022-10-21T21:52:37Z

test/libsolutil/StringUtils.cpp

+	// Decoding failure cases (there's not really a standard for that).
+	BOOST_CHECK_EQUAL(util::decodeURI("%"), "");
+	BOOST_CHECK_EQUAL(util::decodeURI("%ZZ"), "ZZ");
+	BOOST_CHECK_EQUAL(util::decodeURI("%7Ge"), "7Ge");
+	BOOST_CHECK_EQUAL(util::decodeURI("%2F%2%2F"), "/2/");
+	BOOST_CHECK_EQUAL(util::decodeURI("%1G/%7G/%FG"), "1G/7G/FG");


Wouldn't it be better to handle these as errors? Having % silently disappear from the end of a string when you pass in something not encoded by accident does not sound like the nicest behavior.

cameel · 2022-10-21T21:54:45Z

test/libsolutil/StringUtils.cpp

+	BOOST_CHECK_EQUAL(util::decodeURI("%25"), "%");
+	BOOST_CHECK_EQUAL(util::decodeURI("Hello%20World."), "Hello World.");
+	BOOST_CHECK_EQUAL(util::decodeURI("Hello World."), "Hello World.");
+	BOOST_CHECK_EQUAL(util::decodeURI("%01%02%7F"), "\x01\x02\x7F");
+	BOOST_CHECK_EQUAL(util::decodeURI("C%3A%5Creadme.md"), "C:\\readme.md");
+	BOOST_CHECK_EQUAL(util::decodeURI("C:/readme.md"), "C:/readme.md");
+
+	// Decoding failure cases (there's not really a standard for that).
+	BOOST_CHECK_EQUAL(util::decodeURI("%"), "");
+	BOOST_CHECK_EQUAL(util::decodeURI("%ZZ"), "ZZ");
+	BOOST_CHECK_EQUAL(util::decodeURI("%7Ge"), "7Ge");
+	BOOST_CHECK_EQUAL(util::decodeURI("%2F%2%2F"), "/2/");
+	BOOST_CHECK_EQUAL(util::decodeURI("%1G/%7G/%FG"), "1G/7G/FG");


Can you add a few more?

A Windows path with a file:// prefix.

%😃

%Z

cameel · 2022-10-21T21:56:44Z

test/lsp.py

@@ -747,7 +752,9 @@ def run_testcase(self, testcase: TestParser.RequestAndResponse):
            if isinstance(actualResponseJson["result"], list):
                for result in actualResponseJson["result"]:
                    if "uri" in result:
-                        result["uri"] = result["uri"].replace(self.suite.project_root_uri + "/" + self.sub_dir + "/", "")
+                        result["uri"] = decodeURI(result["uri"]).replace(
+                            self.suite.project_root_uri + "/" + self.sub_dir + "/", ""


Suggested change

self.suite.project_root_uri + "/" + self.sub_dir + "/", ""

self.suite.project_root_uri + "/" + self.sub_dir + "/",

""

github-actions · 2022-11-25T12:07:48Z

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

christianparpart marked this pull request as ready for review September 2, 2022 13:38

nikola-matic reviewed Sep 3, 2022

View reviewed changes

libsolutil/StringUtils.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the fix-lsp-uri-decoding branch from b2bb4b0 to 92d34ad Compare September 5, 2022 08:55

christianparpart mentioned this pull request Sep 5, 2022

LSP throwing an error on each and every character I type in the editor #12825

Closed

christianparpart force-pushed the fix-lsp-uri-decoding branch 2 times, most recently from b70e77c to 4d2c844 Compare September 5, 2022 15:33

aarlt reviewed Sep 5, 2022

View reviewed changes

libsolutil/StringUtils.cpp Show resolved Hide resolved

christianparpart force-pushed the fix-lsp-uri-decoding branch from 4d2c844 to a72c628 Compare September 7, 2022 11:42

Marenz suggested changes Sep 7, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

cameel added the language server label Sep 8, 2022

nikola-matic reviewed Oct 7, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

nikola-matic reviewed Oct 7, 2022

View reviewed changes

test/libsolutil/StringUtils.cpp Show resolved Hide resolved

christianparpart force-pushed the fix-lsp-uri-decoding branch 3 times, most recently from e9fc513 to b298001 Compare October 18, 2022 11:58

cameel reviewed Oct 21, 2022

View reviewed changes

cameel mentioned this pull request Oct 24, 2022

Improved output of formatNumberReadable #13277

Merged

LSP: Fixes URL decoding incoming file names.

a2de5d4

nikola-matic force-pushed the fix-lsp-uri-decoding branch from b298001 to a2de5d4 Compare November 4, 2022 13:15

NunoFilipeSantos assigned christianparpart Nov 10, 2022

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 25, 2022

NunoFilipeSantos closed this Nov 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSP: Fixes URL decoding incoming file names. #13473

LSP: Fixes URL decoding incoming file names. #13473

christianparpart commented Sep 2, 2022 •

edited

Loading

Marenz left a comment

This comment was marked as resolved.

cameel left a comment

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022 •

edited

Loading

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022

cameel Oct 21, 2022

github-actions bot commented Nov 25, 2022

	* Language Server: Fixes URL decoding of incoming file names.
	* Language Server: Properly URL-decode the `file://` URIs coming from the client and encode the URIs coming out of by the server.

		target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system range-v3)
		target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system range-v3 fmt::fmt-header-only)

		BOOST_CHECK_EQUAL(util::decodeURI(""), "");
		BOOST_CHECK_EQUAL(util::decodeURI(""), "");

	BOOST_CHECK_EQUAL(util::decodeURI(" ")," ");
	BOOST_CHECK_EQUAL(util::decodeURI(" "), " ");

	self.suite.project_root_uri + "/" + self.sub_dir + "/", ""
	self.suite.project_root_uri + "/" + self.sub_dir + "/",
	""

LSP: Fixes URL decoding incoming file names. #13473

LSP: Fixes URL decoding incoming file names. #13473

Conversation

christianparpart commented Sep 2, 2022 • edited Loading

Checklist

Marenz left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2022

christianparpart commented Sep 2, 2022 •

edited

Loading

cameel Oct 21, 2022 •

edited

Loading