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

Widespread wrong handling of space in File->URI->File conversion?! #35

Open
jukzi opened this issue Jun 15, 2022 · 8 comments
Open

Widespread wrong handling of space in File->URI->File conversion?! #35

jukzi opened this issue Jun 15, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2022

After fixing eclipse-jdt/eclipse.jdt.debug#66 i took a look where else URLs might be decoded wrong

org.eclipse.core.runtime.URIUtil seems to be wrong even though it's javadoc explicitly says it can handle space

		File file = new File("c:/my test");
		System.out.println("file=" + file); // c:\my test OK
		URL url = file.toURI().toURL();
		System.out.println("url=" + url); // file:/c:/my%20test OK
		URL fileUrl = FileLocator.toFileURL(url);
		System.out.println("fileUrl=" + fileUrl); // file:/c:/my%20test  OK

		URI uri = org.eclipse.core.runtime.URIUtil.toURI(url);
		System.out.println("uri=" + uri); // file:/c:/my%2520test <- %25 means "%" WTF
		File toFile = org.eclipse.core.runtime.URIUtil.toFile(uri);
		System.out.println("toFile=" + toFile); // c:\my%20test <- should be c:\my test

// should be:
		File good = new File(url.toURI()); 
		System.out.println("good=" + good); // c:\my test OK

other wrong decodings new File(.*url.*getPath()) (regular pattern search) are in

  • org.eclipse.ant.internal.core.ant.InternalAntRunner.setJavaClassPath()
  • org.eclipse.ant.internal.ui.editor.utils.ProjectHelper.ProjectHandler.onStartElement(String, String, String, Attributes, AntXMLContext)
  • org.eclipse.ant.internal.ui.model.AntDefiningTaskNode.setJavaClassPath(URL[])
  • org.eclipse.core.internal.runtime.AuthorizationHandler.loadKeyring()
  • org.eclipse.core.tests.internal.runtime.PlatformURLSessionTest.createData(String)
  • org.eclipse.e4.ui.internal.workbench.swt.E4Application.getVersionFile(URL, boolean)
  • org.eclipse.ui.internal.ide.application.IDEApplication.getVersionFile(URL, boolean)
  • org.eclipse.ecf.provider.filetransfer.browse.LocalFileSystemBrowser.LocalFileSystemBrowser(IFileID, IRemoteFileSystemListener)
  • org.eclipse.ecf.provider.filetransfer.outgoing.LocalFileOutgoingFileTransfer.openStreams()
  • org.eclipse.jdt.internal.core.index.IndexLocation.createIndexLocation(URL)
  • org.eclipse.urischeme.internal.registration.FileProvider.getFilePath(URL)

Another wrong pattern is new File(.*url.*getFile())

		File urlGetFile = new File(url.getFile());
		System.out.println("urlGetFile=" + urlGetFile); // c:\my%20test <- should be c:\my test

in places like:

  • AbstractCSSEngine.java - org.eclipse.e4.ui.css.core/src/org/eclipse/e4/ui/css/core/impl/engine (2 matches)
  • AbstractJavaModelTests.java - org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model
  • AntTestPlugin.java - org.eclipse.ant.tests.core/test plugin/org/eclipse/ant/tests/core/testplugin
  • AntUITestPlugin.java - org.eclipse.ant.tests.ui/test plugin/org/eclipse/ant/tests/ui/testplugin
  • ChooseWorkspaceData.java - org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide (4 matches)
  • ClassLoaderTools.java - org.eclipse.test/src/org/eclipse/test
  • ConfigurationActivator.java - org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator
  • ConfigurationParser.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator
  • ConfigurationParser.java - org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator
  • ConfigurationSessionTestSuite.java - org.eclipse.core.tests.harness/src/org/eclipse/core/tests/session
  • ContentDetectHelper.java - org.eclipse.ui.intro.universal/src/org/eclipse/ui/internal/intro/universal/contentdetect
  • DebugCorePlugin.java - org.eclipse.debug.examples.core/src/org/eclipse/debug/examples/core/pda
  • E4Application.java - org.eclipse.e4.ui.workbench.swt/src/org/eclipse/e4/ui/internal/workbench/swt
  • EEVMType.java - org.eclipse.jdt.launching/launching/org/eclipse/jdt/internal/launching (2 matches)
  • FeatureParser.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator
  • FeatureParser.java - org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator
  • FileSystemAccess.java - org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem
  • FileTool.java - org.eclipse.core.filebuffers.tests/src/org/eclipse/core/filebuffers/tests
  • FileTool.java - org.eclipse.search.tests/src/org/eclipse/search/tests
  • FileTool.java - org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util
  • FormatterMassiveRegressionTests.java - org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter
  • HelpBasePlugin.java - org.eclipse.help.base/src/org/eclipse/help/internal/base
  • HelpPlugin.java - org.eclipse.help/src/org/eclipse/help/internal
  • IDEApplication.java - org.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application
  • IncludedSchemaDescriptor.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/schema
  • InternalPlatform.java - org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime
  • LaunchingPlugin.java - org.eclipse.jdt.launching/launching/org/eclipse/jdt/internal/launching (3 matches)
  • LaunchTests.java - org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching
  • Main_in.java - org.eclipse.jdt.core.tests.model/workspace/Formatter/test492
  • Main_out.java - org.eclipse.jdt.core.tests.model/workspace/Formatter/test492
  • ModelTestsUtil.java - org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model
  • OpenBundleResourceHandler.java - org.eclipse.help.ui/src/org/eclipse/help/ui/internal/handlers
  • PlatformConfiguration.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator (2 matches)
  • PlatformConfiguration.java - org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator (7 matches)
  • PluginPathFinder.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core
  • SchemaDescriptor.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/schema
  • SchemaRegistry.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/schema
  • SiteEntry.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator (6 matches)
  • SiteEntry.java - org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator (8 matches)
  • SmartImportTests.java - org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer (5 matches)
  • TargetPlatformHelper.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core
  • TestsPlugin.java - org.eclipse.debug.tests/src/org/eclipse/debug/tests
  • TestsPlugin.java - org.eclipse.debug.ui.launchview.tests/src/org/eclipse/debug/ui/launchview/tests
  • ThemeEngine.java - org.eclipse.e4.ui.css.swt.theme/src/org/eclipse/e4/ui/css/swt/internal/theme
  • UpdateManagerHelperDeprecated.java - org.eclipse.pde.core/src/org/eclipse/pde/internal/core
  • URLFieldEditor.java - org.eclipse.ant.ui/Ant Tools Support/org/eclipse/ant/internal/ui/preferences
  • Utils.java - org.eclipse.test.performance.ui/src/org/eclipse/test/performance/ui

I do not know when all of this is used, does anybody know if that can be a real issue?
Can we just copy & paste the solution and trust it works?

@laeubi
Copy link
Contributor

laeubi commented Jun 15, 2022

Why do we need all this Util methods at all? Can't this be replaced with standard java?

I do not know when all of this is used, does anybody know if that can be a real issue?

Rule 1: Never use spaces in your eclipse workspace ;-)
Rule 2: Always follow rule 1

@jukzi
Copy link
Contributor Author

jukzi commented Jun 15, 2022

Rule 1: Never use spaces in your eclipse workspace ;-)

Yea, I even had to disallow non-ascii characters for our developers for Workspace and Git repos.

@iloveeclipse
Copy link
Member

Have no IDE at hand, but we not only support "file:" schema, also other schemas should be possible. So redirecting everything to "new File(url.toURI())" may not always work.

@merks
Copy link
Contributor

merks commented Jun 15, 2022

I suspect that some things may even rely on this wrong behavior, so sweeping changes and hoping/assuming all will work well is probably a bad assumption. At least on Windows, user IDs with spaces are not all that rare, but that does generally work okay despite this long list of questionable code...

@SarikaSinha
Copy link
Member

As far as I remember this was required for remote file conversions, will try to find the related comment and usage.

@HannesWell
Copy link
Member

Why do we need all this Util methods at all? Can't this be replaced with standard java?

I think it would definitely be better to work towards using standard Java API for that.
Replacing File.toURL() by File.toURI().toURL() is a similar topic that can be done at many locations in the Eclipse code base.

I do not know when all of this is used, does anybody know if that can be a real issue?

Rule 1: Never use spaces in your eclipse workspace ;-) Rule 2: Always follow rule 1

The 90s are calling and want their problems back. 😄

I suspect that some things may even rely on this wrong behavior, so sweeping changes and hoping/assuming all will work well is probably a bad assumption. At least on Windows, user IDs with spaces are not all that rare, but that does generally work okay despite this long list of questionable code...

Although I'm for fixing this, I share that concern. What we can do is to check how the produced URL is used. If it is only used by java API (e.g. a stream is opened from it) it should be save to fix it. If it is for example converted to a String and then later parsed again, we have to ensure that the parsing code can handle a correctly encoded URL. So maybe for such cases we have to ensure that both correct and incorrectly encoded URLs can be handled, then wait some time (one or two years?) and the 'fix' the remaining parts to produce correct URLs.

@jukzi
Copy link
Contributor Author

jukzi commented Oct 6, 2022

relates to https://bugs.eclipse.org/bugs/show_bug.cgi?id=145096#c4 FileLocator.resolve(URL) returns invalid file URL (spaces not escaped)
a long known bug where they did not change encoding to not break clients that might rely on wrong encoding

@HannesWell
Copy link
Member

smile

I suspect that some things may even rely on this wrong behavior, so sweeping changes and hoping/assuming all will work well is probably a bad assumption. At least on Windows, user IDs with spaces are not all that rare, but that does generally work okay despite this long list of questionable code...

Although I'm for fixing this, I share that concern. What we can do is to check how the produced URL is used. If it is only used by java API (e.g. a stream is opened from it) it should be save to fix it. If it is for example converted to a String and then later parsed again, we have to ensure that the parsing code can handle a correctly encoded URL. So maybe for such cases we have to ensure that both correct and incorrectly encoded URLs can be handled, then wait some time (one or two years?) and the 'fix' the remaining parts to produce correct URLs.

Do we have any plan to fix this issue? What do you think about the suggested approach?

In the FileLocator we could work-around the compatibility problem by adding new methods like toFile/Path() that returns a File/Path directly. Isn't that what most users want to have any way?
For FileLocator.resolve() we could find an alternative, for example FileLocator.toNativeURL() or just FileLocator.resolve() that explicitly states that it encodes URLs correctly.
The existing wrong behaving methods could then be deprecated and removed after the usual deprecation period (or if necessary after an extended period).

JavaJoeS pushed a commit to JavaJoeS/eclipse.platform that referenced this issue Jan 29, 2024
…platform#35)

See original bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=514333
that was only partly fixed.

The original fix added -Dosgi.dataAreaRequiresExplicitInit=true system
property and changed org.eclipse.core.internal.runtime.DataArea in the
way that it doesn't allow to initialize itself if the workspace location
was not explicitly specified yet - either via -data or via workspace
selection prompt.

But the fix in DataArea does not work if bad client code calls
Platform.getLocation() *before* calling Plugin.getStateLocation(),
because Platform.getLocation() doesn't run into changed DataArea code at
all.

Exact this happened in commit
eclipse-platform/eclipse.platform.resources@a8a8d82
where workspace init order was slightly changed: original code
initialized LocalMetaArea first (which then failed as supposed in
assertLocationInitialized() if workspace was not set yet), and after
that initialized WorkspaceRoot (which uses Platform.getLocation() after
the check). Changed code initialized WorkspaceRoot first, and because
this uses Platform.getLocation(), the code silently initializes default
workspace location even if -Dosgi.dataAreaRequiresExplicitInit=true is
set.

The current fix makes sure that if the system flag is given, we disallow
silent workspace location initialization by adding similar check in
Platform.getLocation().

Fixes issue
eclipse-platform/eclipse.platform.runtime#35
@jukzi jukzi added the bug Something isn't working label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants